From 84ca6c52db02e5e377abd03eeb61bf5f80534fd7 Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Wed, 21 Oct 2026 16:17:08 +0300 Subject: [PATCH 1/2] vregion: do not read back create_map results from user memory z_impl_vregion_create_map() wrote the region start and size to the caller's buffers and then read them back to build the memory partition. As a system call these buffers are user memory, so another thread could change them in between and get arbitrary memory mapped into the LL domain. Read the requested size once, build the partitions from kernel locals and write the results only at the end. Also set vr->domain after the first partition is added, so a failure adding the second one unmaps the first. Signed-off-by: Kai Vehmanen --- zephyr/lib/vregion.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/zephyr/lib/vregion.c b/zephyr/lib/vregion.c index d28565e0b0d9..0bda4d5d6572 100644 --- a/zephyr/lib/vregion.c +++ b/zephyr/lib/vregion.c @@ -194,25 +194,36 @@ struct vregion *z_impl_vregion_get(struct vregion *vr) struct vregion *z_impl_vregion_create_map(uintptr_t *vreg_start, size_t *vreg_size) { - if (!vreg_start || !vreg_size || !*vreg_size) + if (!vreg_start || !vreg_size) return NULL; - struct vregion *vr = vregion_create(*vreg_size); + /* + * Called as a system call, both pointers refer to user memory that + * another thread can modify at any time. Read the size once and only + * write the results back at the end, never read them back. + */ + size_t size = *vreg_size; + + if (!size) + return NULL; + + struct vregion *vr = vregion_create(size); + uintptr_t start; if (!vr) return NULL; -#if CONFIG_USERSPACE && CONFIG_SOF_USERSPACE_LL - vregion_mem_info(vr, vreg_size, vreg_start); + vregion_mem_info(vr, &size, &start); +#if CONFIG_USERSPACE && CONFIG_SOF_USERSPACE_LL /* * In the userspace LL case allocations are also performed by the * userspace IPC thread, which is also the one, executing this syscall */ struct k_mem_domain *domain = zephyr_ll_mem_domain(); struct k_mem_partition part = { - .start = *vreg_start, - .size = *vreg_size, + .start = start, + .size = size, .attr = K_MEM_PARTITION_P_RW_U_RW | XTENSA_MMU_CACHED_WB, }; int ret = k_mem_domain_add_partition(domain, &part); @@ -222,6 +233,9 @@ struct vregion *z_impl_vregion_create_map(uintptr_t *vreg_start, size_t *vreg_si return NULL; } + /* from here on vregion_unmap() must remove what was added */ + vr->domain = domain; + part.start = (uintptr_t)sys_cache_uncached_ptr_get((void *)part.start); part.attr = K_MEM_PARTITION_P_RW_U_RW; @@ -230,10 +244,11 @@ struct vregion *z_impl_vregion_create_map(uintptr_t *vreg_start, size_t *vreg_si vregion_put(vr); return NULL; } - - vr->domain = domain; #endif + *vreg_start = start; + *vreg_size = size; + return vr; } From 6e7f2dc781cf1524366d2bf33bfb6be6c68c6bc0 Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Wed, 21 Oct 2026 16:17:09 +0300 Subject: [PATCH 2/2] vregion: validate syscall handles against a kernel registry vregion_verify() accepted any pointer to memory the caller cannot read, as long as the two words at its start looked like a user-writable range. A forged handle let the kernel lock, count, initialize and free kernel memory of the caller's choice. A concurrent vregion_put() could also free a vregion between validation and use. Keep live vregions in a kernel-only list. vregion_verify_pin() looks the handle up and pins it under the list lock, and the validators unpin it after the call. Pins are counted separately from references, so a vregion_put() call of another thread can not consume them, and a vregion is destroyed only when both its references and its pins are gone. vregion_put() removes the last reference and the list entry atomically with respect to lookups. The vregion_get() and vregion_put() validators take or drop the caller's reference under the lock, and a put without a reference left is rejected instead of underflowing the count. Signed-off-by: Kai Vehmanen --- src/include/sof/lib/vregion.h | 46 ++++++++- zephyr/lib/vregion.c | 169 ++++++++++++++++++++++++++++------ zephyr/syscall/vregion.c | 64 ++++++++++--- 3 files changed, 231 insertions(+), 48 deletions(-) diff --git a/src/include/sof/lib/vregion.h b/src/include/sof/lib/vregion.h index fb5ae60e8c67..eab7e42a36ff 100644 --- a/src/include/sof/lib/vregion.h +++ b/src/include/sof/lib/vregion.h @@ -10,6 +10,7 @@ #ifndef __SOF_LIB_VREGION_H__ #define __SOF_LIB_VREGION_H__ +#include #include #include #include @@ -172,11 +173,45 @@ void vregion_info(struct vregion *vr); void vregion_mem_info(struct vregion *vr, size_t *size, uintptr_t *start); /** - * @brief Validate that a virtual region handle is genuine and isolated from user space. + * @brief Validate a virtual region handle passed by a user thread and pin it. + * + * For system call validators only. Kills the calling thread if vr is not a + * live vregion, or if the caller has no access to the region memory. On + * success vr stays allocated until vregion_unpin(), even if all its + * references are dropped meanwhile. A pin is not a reference: it is counted + * separately, so vregion_put() calls of other threads can not consume it. + * * @param[in] vr Pointer to virtual region instance to verify. - * @return True if handle is valid and secure, false otherwise. + * @return True if vr was pinned, false if vr is NULL. + */ +bool vregion_verify_pin(struct vregion *vr); + +/** + * @brief Release a pin taken with vregion_verify_pin(). + * + * Frees vr if this was the last pin and no references are left. + * + * @param[in] vr Pinned virtual region instance. + */ +void vregion_unpin(struct vregion *vr); + +/** + * @brief Take a reference to a pinned virtual region. + * + * @param[in] vr Pinned virtual region instance. + * @return False if vr has no references left, so it can not be revived. + */ +bool vregion_pinned_get(struct vregion *vr); + +/** + * @brief Drop a reference to a pinned virtual region. + * + * Never frees vr: that is left to vregion_unpin(). + * + * @param[in] vr Pinned virtual region instance. + * @return References left, or -EINVAL if vr had none to drop. */ -bool vregion_verify(struct vregion *vr); +int vregion_pinned_put(struct vregion *vr); #include @@ -227,7 +262,10 @@ static inline void vregion_mem_info(struct vregion *vr, size_t *size, uintptr_t if (size) *size = 0; } -static inline bool vregion_verify(struct vregion *vr) {return false;} +static inline bool vregion_verify_pin(struct vregion *vr) { return false; } +static inline void vregion_unpin(struct vregion *vr) {} +static inline bool vregion_pinned_get(struct vregion *vr) { return false; } +static inline int vregion_pinned_put(struct vregion *vr) { return -EINVAL; } #endif /* CONFIG_SOF_VREGIONS */ diff --git a/zephyr/lib/vregion.c b/zephyr/lib/vregion.c index 0bda4d5d6572..846f6d24356e 100644 --- a/zephyr/lib/vregion.c +++ b/zephyr/lib/vregion.c @@ -5,9 +5,12 @@ * Author: Liam Girdwood */ +#include +#include #include #include #include +#include #include #include #include @@ -84,12 +87,15 @@ struct interim_heap { * TODO: Add support to flag which heaps should have their contexts saved and restored. */ struct vregion { + sys_dnode_t node; /* entry in vregion_list */ + /* region context */ uint8_t *base; /* base address of entire region */ size_t size; /* size of whole region in bytes */ unsigned int pages; /* size of whole region in pages */ - struct k_mutex lock; /* protect vregion heaps and use-count */ - unsigned int use_count; + struct k_mutex lock; /* protect vregion heaps, use-count and pins */ + unsigned int use_count; /* references held by kernel and user code */ + unsigned int pins; /* system calls in progress, see vregion_verify_pin() */ struct k_mem_domain *domain; /* current allocation mode */ @@ -102,6 +108,15 @@ struct vregion { struct vlinear_heap lifetime; /* lifetime linear heap */ }; +/* + * Kernel-only list of live vregions. System call validators look up the + * vregion pointers passed by user threads here, so that a forged pointer + * to some other kernel-only memory is never treated as a vregion. + * Lock order: vregion_list_lock, then vr->lock. + */ +static sys_dlist_t vregion_list = SYS_DLIST_STATIC_INIT(&vregion_list); +static K_MUTEX_DEFINE(vregion_list_lock); + /** * @brief Create a new virtual region instance. * @@ -172,6 +187,11 @@ __cold struct vregion *vregion_create(size_t memsize) k_mutex_init(&vr->lock); /* The creator is the first user */ vr->use_count = 1; + vr->pins = 0; + + k_mutex_lock(&vregion_list_lock, K_FOREVER); + sys_dlist_append(&vregion_list, &vr->node); + k_mutex_unlock(&vregion_list_lock); /* log the new vregion */ LOG_INF("new at base %p size %#zx pages %u metadata at %p", @@ -273,6 +293,36 @@ static void vregion_unmap(struct vregion *vr) #endif } +/* Free vr, once it has no references and no system call pins left */ +static void vregion_destroy(struct vregion *vr) +{ + /* log the vregion being destroyed */ + LOG_DBG("destroy %p size %#zx pages %u", (void *)vr->base, vr->size, vr->pages); + LOG_DBG(" lifetime used %zu free count %d", vr->lifetime.used, vr->lifetime.free_count); + vpage_free(vr->base); + + vregion_unmap(vr); + rfree(vr); +} + +/* + * Drop one reference. Caller holds vregion_list_lock and vr->lock, taken in + * that order, so that vregion_verify_pin() can not find the instance after + * its last reference has been dropped. + * + * @return remaining references, or -EINVAL if there was none to drop + */ +static int vregion_put_locked(struct vregion *vr) +{ + if (!vr->use_count) + return -EINVAL; + + if (!--vr->use_count) + sys_dlist_remove(&vr->node); + + return vr->use_count; +} + /** * @brief Decrement virtual region's user count or destroy it. * @@ -281,29 +331,29 @@ static void vregion_unmap(struct vregion *vr) */ struct vregion *z_impl_vregion_put(struct vregion *vr) { - unsigned int use_count; + bool destroy; + int ret; if (!vr) return NULL; + k_mutex_lock(&vregion_list_lock, K_FOREVER); k_mutex_lock(&vr->lock, K_FOREVER); - use_count = --vr->use_count; + ret = vregion_put_locked(vr); + /* a system call in progress destroys it in vregion_unpin() instead */ + destroy = !ret && !vr->pins; k_mutex_unlock(&vr->lock); + k_mutex_unlock(&vregion_list_lock); - if (use_count) - return vr; - - /* Last user: nobody else can access the instance. */ - - /* log the vregion being destroyed */ - LOG_DBG("destroy %p size %#zx pages %u", (void *)vr->base, vr->size, vr->pages); - LOG_DBG(" lifetime used %zu free count %d", vr->lifetime.used, vr->lifetime.free_count); - vpage_free(vr->base); + if (ret < 0) { + LOG_ERR("put %p without a reference", (void *)vr); + return NULL; + } - vregion_unmap(vr); - rfree(vr); + if (destroy) + vregion_destroy(vr); - return NULL; + return ret ? vr : NULL; } /** @@ -590,28 +640,87 @@ void vregion_mem_info(struct vregion *vr, size_t *size, uintptr_t *start) #if CONFIG_SOF_VREGIONS && CONFIG_USERSPACE #include -bool vregion_verify(struct vregion *vr) +bool vregion_verify_pin(struct vregion *vr) { + struct vregion *it; + bool found = false; + if (!vr) return false; + k_mutex_lock(&vregion_list_lock, K_FOREVER); + SYS_DLIST_FOR_EACH_CONTAINER(&vregion_list, it, node) { + if (it == vr) { + found = true; + break; + } + } + + if (found) { + /* + * The list lock keeps vregion_put() from freeing vr meanwhile. + * Count the pin separately from references: a pin must not be + * consumed by a vregion_put() call of another user thread. + */ + k_mutex_lock(&vr->lock, K_FOREVER); + vr->pins++; + k_mutex_unlock(&vr->lock); + } + k_mutex_unlock(&vregion_list_lock); + + /* not a live vregion: a forged pointer */ + K_OOPS(!found); + /* - * vregion instances must not be accessible to the userspace. - * - * Don't use K_SYSCALL_MEMORY_READ() here: it logs an "access denied" - * error whenever the region is inaccessible, which is exactly the - * expected (good) case for a kernel-only vregion. Omit false - * error messages by using arch_buffer_validate() directly. + * The caller must have the region memory mapped. vr->base and + * vr->size are kernel-only and constant over the vregion lifetime. */ - K_OOPS(arch_buffer_validate((void *)vr, sizeof(*vr), 0) == 0); + if (K_SYSCALL_MEMORY_WRITE(vr->base, vr->size)) { + vregion_unpin(vr); + K_OOPS(true); + } - size_t vr_size = 0; - uintptr_t vr_start; + return true; +} - vregion_mem_info(vr, &vr_size, &vr_start); - if (vr_size) - K_OOPS(K_SYSCALL_MEMORY_WRITE((void *)vr_start, vr_size)); +void vregion_unpin(struct vregion *vr) +{ + bool destroy; - return true; + k_mutex_lock(&vr->lock, K_FOREVER); + destroy = !--vr->pins && !vr->use_count; + k_mutex_unlock(&vr->lock); + + /* the last reference was dropped while pinned, and it is off the list */ + if (destroy) + vregion_destroy(vr); +} + +bool vregion_pinned_get(struct vregion *vr) +{ + bool ok; + + k_mutex_lock(&vr->lock, K_FOREVER); + /* do not revive a vregion whose last reference is already gone */ + ok = vr->use_count && vr->use_count < UINT_MAX; + if (ok) + vr->use_count++; + k_mutex_unlock(&vr->lock); + + return ok; +} + +int vregion_pinned_put(struct vregion *vr) +{ + int ret; + + k_mutex_lock(&vregion_list_lock, K_FOREVER); + k_mutex_lock(&vr->lock, K_FOREVER); + ret = vregion_put_locked(vr); + k_mutex_unlock(&vr->lock); + k_mutex_unlock(&vregion_list_lock); + + /* destroying is left to vregion_unpin(), the caller still holds a pin */ + return ret; } #endif diff --git a/zephyr/syscall/vregion.c b/zephyr/syscall/vregion.c index dd02f854cf82..8593530149d8 100644 --- a/zephyr/syscall/vregion.c +++ b/zephyr/syscall/vregion.c @@ -6,45 +6,79 @@ #include #include +/* + * Every validator pins the vregion with vregion_verify_pin() for the + * duration of the call, so a concurrent vregion_put() from another thread + * cannot free the vregion while the implementation uses it. The pin is + * counted separately from references, so such a vregion_put() can not + * consume it either. + */ + static inline void *z_vrfy_vregion_alloc_align(struct vregion *vr, size_t size, size_t alignment) { - if (vregion_verify(vr)) - return z_impl_vregion_alloc_align(vr, size, alignment); + void *ptr = NULL; + + if (vregion_verify_pin(vr)) { + ptr = z_impl_vregion_alloc_align(vr, size, alignment); + vregion_unpin(vr); + } - return NULL; + return ptr; } #include static inline void *z_vrfy_vregion_alloc_coherent_align(struct vregion *vr, size_t size, size_t alignment) { - if (vregion_verify(vr)) - return z_impl_vregion_alloc_coherent_align(vr, size, alignment); + void *ptr = NULL; + + if (vregion_verify_pin(vr)) { + ptr = z_impl_vregion_alloc_coherent_align(vr, size, alignment); + vregion_unpin(vr); + } - return NULL; + return ptr; } #include static inline void z_vrfy_vregion_free(struct vregion *vr, void *ptr) { - if (vregion_verify(vr)) + if (vregion_verify_pin(vr)) { z_impl_vregion_free(vr, ptr); + vregion_unpin(vr); + } } #include struct vregion *z_vrfy_vregion_get(struct vregion *vr) { - if (vregion_verify(vr)) - return z_impl_vregion_get(vr); - return NULL; + bool ok; + + if (!vregion_verify_pin(vr)) + return NULL; + + ok = vregion_pinned_get(vr); + vregion_unpin(vr); + + return ok ? vr : NULL; } #include struct vregion *z_vrfy_vregion_put(struct vregion *vr) { - if (vregion_verify(vr)) - return z_impl_vregion_put(vr); - return NULL; + int ret; + + if (!vregion_verify_pin(vr)) + return NULL; + + /* drop the caller's reference, freeing is left to the unpin */ + ret = vregion_pinned_put(vr); + vregion_unpin(vr); + + /* releasing a reference the caller does not hold */ + K_OOPS(ret < 0); + + return ret ? vr : NULL; } #include @@ -58,7 +92,9 @@ struct vregion *z_vrfy_vregion_create_map(uintptr_t *vreg_start, size_t *vreg_si void z_vrfy_vregion_set_interim(struct vregion *vr) { - if (vregion_verify(vr)) + if (vregion_verify_pin(vr)) { z_impl_vregion_set_interim(vr); + vregion_unpin(vr); + } } #include