[userspace LL] vregion related syscalls - #11108
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Zephyr userspace support for SOF “vregion” allocations to enable userspace LL + DP integration (part of #10945), including syscall plumbing and module-adapter wiring to map vregion memory into the LL userspace memory domain.
Changes:
- Introduces Zephyr syscall handlers/marshalling for vregion alloc/free/get/put/set_interim.
- Refactors Zephyr vregion implementation entrypoints to
z_impl_*to back the new syscalls. - Extends module-adapter allocation flow to create/map/unmap vregions for DP modules and plumbs vregion start/size through
mod_alloc_ctx.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| zephyr/syscall/vregion.c | New syscall verification + marshalling includes for vregion APIs. |
| zephyr/lib/vregion.c | Switches vregion APIs to z_impl_* entrypoints and adjusts symbol exports accordingly. |
| zephyr/Kconfig | Adds SOF_USERSPACE_INTERFACE_VREGION and selects it from SOF_USERSPACE_LL. |
| zephyr/include/rtos/alloc.h | Extends mod_alloc_ctx with vregion start/size metadata for domain mapping. |
| zephyr/CMakeLists.txt | Adds syscall header generation and builds the new vregion syscall source. |
| src/include/sof/lib/vregion.h | Marks vregion APIs as __syscall and includes generated syscall header. |
| src/include/sof/audio/module_adapter/module/generic.h | Exposes module-adapter vregion map/unmap as syscalls for full Zephyr app. |
| src/audio/module_adapter/module_adapter.c | Implements vregion creation + mem-domain partition mapping and adds syscall verifiers. |
| src/audio/buffers/comp_buffer.c | Routes vregion-backed buffer free through the new shared vregion-free helper. |
Suppressed comments (2)
src/audio/module_adapter/module_adapter.c:134
- module_adapter_vreg_free() decrements the vregion refcount (and may free the vregion pages) before removing the user mem-domain partitions. If vregion_put() frees the pages, the user partition remains until module_adapter_vreg_unmap() runs, creating a window where freed (and potentially reallocated) pages stay user-accessible. Consider unmapping first and freeing the vregion atomically in kernel code when the refcount reaches 0.
void module_adapter_vreg_free(struct mod_alloc_ctx *alloc)
{
if (vregion_put(alloc->vreg))
return;
module_adapter_vreg_unmap(alloc);
sof_heap_free(alloc->heap, alloc);
src/audio/module_adapter/module_adapter.c:255
- The syscall verifier for module_adapter_vreg_unmap() only checks that the alloc struct is readable. In userspace-LL configurations alloc lives in user-writable memory, so a caller can forge vreg_start/vreg_size and attempt to remove arbitrary partitions from zephyr_ll_mem_domain(). Add validation that alloc refers to an expected allocation and that (vreg_start,vreg_size) match the vregion’s actual mem_info (or avoid taking alloc from user-space entirely).
void z_vrfy_module_adapter_vreg_unmap(const struct mod_alloc_ctx *alloc)
{
K_OOPS(K_SYSCALL_MEMORY_READ(alloc, sizeof(*alloc)));
z_impl_module_adapter_vreg_unmap(alloc);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4f46949 to
296ef8b
Compare
kv2019i
left a comment
There was a problem hiding this comment.
Please check inline, concern with the syscall verify function.
| void z_vrfy_module_adapter_vreg_unmap(const struct mod_alloc_ctx *alloc) | ||
| { | ||
| K_OOPS(K_SYSCALL_MEMORY_READ(alloc, sizeof(*alloc))); | ||
| z_impl_module_adapter_vreg_unmap(alloc); |
There was a problem hiding this comment.
Hmm, this is not safe, user-space can pass arbitrary alloc->vreg and this is passed unchecked to kernel code.
There was a problem hiding this comment.
@kv2019i added an owner, please re-check
There was a problem hiding this comment.
This is better, but as "alloc" is untrusted memory, "alloc->vreg" can point to a made-up vreg object (with owner set to point to alloc by the user-space thread). I think another layer is needed to look up "vreg" is an actual kernel vreg object, and not a pointer to some random user created object.
6f6c216 to
c223bae
Compare
c5e4761 to
88da26b
Compare
Add two syscall functions to allocate and map, and to unmap vregion for userspace modules. For now only used for DP modules. Add module_adapter.c to cmocka builds for the new module_adapter_vreg_free() function, which is now called in comp_buffer.c. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
scheduler_dp_task_init() currently only runs in privileged mode, add a comment and a check for that. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The entire user_access_to_mailbox() function is already under an #ifdef CONFIG_SOF_USERSPACE_LL condition. Remove an additional identical check inside the function. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Make vregion_alloc(), vregion_alloc_coherent(), vregion_alloc_align(), vregion_alloc_coherent_align(), and vregion_free() available as Zephyr system calls for user-space threads. Add K_SYSCALL_MEMORY_WRITE verification to all syscall handlers to validate the calling thread has access to the vregion's managed memory area. Add CONFIG_SOF_USERSPACE_INTERFACE_VREGION Kconfig option to control the feature. It is auto-selected by SOF_USERSPACE_LL when SOF_VREGIONS is enabled. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Extract common syscall verification code into a function. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
vregion_get(), vregion_put() and vregion_set_interim() should also be callable from the userspace. Make them syscalls. Also remove redundant symbol exporting since the vregion API shouldn't be used directly by LLEXT modules. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
kv2019i
left a comment
There was a problem hiding this comment.
The new check in vregion_verify() could be enough, please see comments inline. At least I'd add a comment how this protects against invalid/fabricated "vr" objects.
| void z_vrfy_module_adapter_vreg_unmap(const struct mod_alloc_ctx *alloc) | ||
| { | ||
| K_OOPS(K_SYSCALL_MEMORY_READ(alloc, sizeof(*alloc))); | ||
| z_impl_module_adapter_vreg_unmap(alloc); |
There was a problem hiding this comment.
This is better, but as "alloc" is untrusted memory, "alloc->vreg" can point to a made-up vreg object (with owner set to point to alloc by the user-space thread). I think another layer is needed to look up "vreg" is an actual kernel vreg object, and not a pointer to some random user created object.
| size_t vr_size = 0; | ||
| uintptr_t vr_start; | ||
|
|
||
| vregion_mem_info(vr, &vr_size, &vr_start); |
There was a problem hiding this comment.
I think here too we'd need to verify "vr" is a valid kernel vr object.
| size_t vr_size = 0; | ||
| uintptr_t vr_start; | ||
| if (vregion_verify(vr)) | ||
| return z_impl_vregion_alloc(vr, size); |
There was a problem hiding this comment.
Commit description says you put common code into vregion_verify(), but it actually does more checks than before, so this is not just about putting it in common function.
| return false; | ||
|
|
||
| /* vregion instances must not be accessible to the userspace. */ | ||
| K_OOPS(!K_SYSCALL_MEMORY_READ(vr, sizeof(*vr))); |
There was a problem hiding this comment.
Ok this could be potentially enough. I still wonder if this is secure enough. but this is definitely a fast check to make, versus looking up a list/array of all kernel vr objects.
vregion system calls, needed when integrating userspace LL and DP
part of #10945