From 63430cf203177aff237ece9f775a67c6a225b25e Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Tue, 10 Nov 2026 19:46:48 +0200 Subject: [PATCH 1/7] ipc: keep send-queue state in kernel-only memory With CONFIG_SOF_USERSPACE_LL, struct ipc is in a partition that the LL and IPC user threads can write. The send queue head, its spinlock, the D3 preparation flag and the completion task mask are only used by kernel code, but a user thread could link a forged message into the queue, corrupt the lock or make the kernel skip or delay the host reply. Move these fields to a kernel-only struct ipc_kstate when CONFIG_SOF_USERSPACE_LL is set, and access them through ipc_ks(). The field names are the same as in struct ipc, which keeps them when CONFIG_SOF_USERSPACE_LL is not set, so other builds are unchanged. Signed-off-by: Kai Vehmanen --- src/include/sof/ipc/common.h | 45 +++++++++++++--- src/ipc/ipc-common.c | 80 +++++++++++++++------------- src/kernel/idc/zephyr_idc.c | 6 +-- src/kernel/ipc/ipc-zephyr.c | 22 ++++---- src/kernel/ipc/ipc3/handler.c | 16 +++--- src/kernel/ipc/ipc3/helper.c | 6 +-- src/kernel/ipc/ipc4/handler-kernel.c | 13 ++--- zephyr/lib/pm_runtime.c | 2 +- 8 files changed, 116 insertions(+), 74 deletions(-) diff --git a/src/include/sof/ipc/common.h b/src/include/sof/ipc/common.h index 696cb553661d..bb770ed3681f 100644 --- a/src/include/sof/ipc/common.h +++ b/src/include/sof/ipc/common.h @@ -98,16 +98,49 @@ struct ipc_user { uint8_t init_drv_data[160] __aligned(4); }; -struct ipc { - struct k_spinlock lock; /* locking mechanism */ - void *comp_data; +#ifdef CONFIG_SOF_USERSPACE_LL +/** + * Kernel-only IPC state. + * + * With CONFIG_SOF_USERSPACE_LL, struct ipc is in a partition that the LL and + * IPC user threads can write. The send queue, its lock and the completion + * state are used by kernel code only, so they are kept here instead. The + * field names match the struct ipc fields used without + * CONFIG_SOF_USERSPACE_LL, so code accesses both through ipc_ks(). + */ +struct ipc_kstate { + struct k_spinlock lock; /* locking mechanism */ + int pm_prepare_D3; /* do we need to prepare for D3 */ + struct list_item msg_list; /* queue of messages to be sent */ + uint32_t task_mask; /* tasks to be completed by this IPC */ +}; + +/** Kernel context only. */ +struct ipc_kstate *ipc_kstate_get(void); + +struct ipc; - /* PM */ - int pm_prepare_D3; /* do we need to prepare for D3 */ +static inline struct ipc_kstate *ipc_ks(struct ipc *ipc) +{ + (void)ipc; + + return ipc_kstate_get(); +} +#else +#define ipc_ks(ipc) (ipc) +#endif +struct ipc { +#ifndef CONFIG_SOF_USERSPACE_LL + /* in struct ipc_kstate with CONFIG_SOF_USERSPACE_LL, use ipc_ks() */ + struct k_spinlock lock; /* locking mechanism */ + int pm_prepare_D3; /* do we need to prepare for D3 */ struct list_item msg_list; /* queue of messages to be sent */ - bool is_notification_pending; /* notification is being sent to host */ uint32_t task_mask; /* tasks to be completed by this IPC */ +#endif + void *comp_data; + + bool is_notification_pending; /* notification is being sent to host */ unsigned int core; /* core, processing the IPC */ struct list_item comp_list; /* list of component devices */ diff --git a/src/ipc/ipc-common.c b/src/ipc/ipc-common.c index d24c20a130b5..f051d51f4e73 100644 --- a/src/ipc/ipc-common.c +++ b/src/ipc/ipc-common.c @@ -61,10 +61,18 @@ K_APPMEM_PARTITION_DEFINE(ipc_context_part); K_APP_BMEM(ipc_context_part) static struct ipc ipc_context; +/* kernel-only, see struct ipc_kstate */ +static struct ipc_kstate ipc_kstate; + struct ipc *ipc_get(void) { return &ipc_context; } + +struct ipc_kstate *ipc_kstate_get(void) +{ + return &ipc_kstate; +} #endif struct ipc_msg *ipc_msg_w_ext_init(struct k_heap *heap, uint32_t header, @@ -124,9 +132,9 @@ int ipc_process_on_core(uint32_t core, bool blocking) k_spinlock_key_t key; ipc->core = core; - key = k_spin_lock(&ipc->lock); - ipc->task_mask |= IPC_TASK_SECONDARY_CORE; - k_spin_unlock(&ipc->lock, key); + key = k_spin_lock(&ipc_ks(ipc)->lock); + ipc_ks(ipc)->task_mask |= IPC_TASK_SECONDARY_CORE; + k_spin_unlock(&ipc_ks(ipc)->lock, key); } /* send IDC message */ @@ -205,16 +213,16 @@ void ipc_send_queued_msg(void) struct ipc_msg *msg; k_spinlock_key_t key; - key = k_spin_lock(&ipc->lock); + key = k_spin_lock(&ipc_ks(ipc)->lock); - if (ipc->pm_prepare_D3) + if (ipc_ks(ipc)->pm_prepare_D3) goto out; /* any messages to send ? */ - if (list_is_empty(&ipc->msg_list)) + if (list_is_empty(&ipc_ks(ipc)->msg_list)) goto out; - msg = list_first_item(&ipc->msg_list, struct ipc_msg, + msg = list_first_item(&ipc_ks(ipc)->msg_list, struct ipc_msg, list); if (ipc_platform_send_msg(msg) == 0) { @@ -229,7 +237,7 @@ void ipc_send_queued_msg(void) #endif } out: - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); } #ifdef __ZEPHYR__ @@ -268,7 +276,7 @@ __cold void ipc_msg_send_direct(struct ipc_msg *msg, void *data) assert_can_be_cold(); - key = k_spin_lock(&ipc->lock); + key = k_spin_lock(&ipc_ks(ipc)->lock); /* copy mailbox data to message if not already copied */ if (data && msg->tx_size > 0 && msg->tx_size <= SOF_IPC_MSG_MAX_SIZE && @@ -279,7 +287,7 @@ __cold void ipc_msg_send_direct(struct ipc_msg *msg, void *data) ipc_platform_send_msg_direct(msg); - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); } void z_impl_ipc_msg_send(struct ipc_msg *msg, void *data, bool high_priority) @@ -288,7 +296,7 @@ void z_impl_ipc_msg_send(struct ipc_msg *msg, void *data, bool high_priority) k_spinlock_key_t key; int ret; - key = k_spin_lock(&ipc->lock); + key = k_spin_lock(&ipc_ks(ipc)->lock); /* copy mailbox data to message if not already copied */ if (data && (msg->tx_size > 0 && msg->tx_size <= SOF_IPC_MSG_MAX_SIZE) && @@ -309,7 +317,7 @@ void z_impl_ipc_msg_send(struct ipc_msg *msg, void *data, bool high_priority) if (high_priority) { ret = ipc_platform_send_msg(msg); if (!ret) { - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); return; } } @@ -317,14 +325,14 @@ void z_impl_ipc_msg_send(struct ipc_msg *msg, void *data, bool high_priority) /* add to queue unless already there */ if (list_is_empty(&msg->list)) { if (high_priority) - list_item_prepend(&msg->list, &ipc->msg_list); + list_item_prepend(&msg->list, &ipc_ks(ipc)->msg_list); else - list_item_append(&msg->list, &ipc->msg_list); + list_item_append(&msg->list, &ipc_ks(ipc)->msg_list); } schedule_ipc_worker(); - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); } EXPORT_SYMBOL(ipc_msg_send); @@ -357,9 +365,9 @@ void z_impl_ipc_msg_list_remove(struct ipc_msg *msg) struct ipc *ipc = ipc_get(); k_spinlock_key_t key; - key = k_spin_lock(&ipc->lock); + key = k_spin_lock(&ipc_ks(ipc)->lock); list_item_del(&msg->list); - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); } #ifdef CONFIG_USERSPACE @@ -380,7 +388,7 @@ void z_vrfy_ipc_msg_list_remove(struct ipc_msg *msg) if (list_is_empty(&msg->list)) return; - list_for_item_safe(mlist, _mlist, &ipc->msg_list) { + list_for_item_safe(mlist, _mlist, &ipc_ks(ipc)->msg_list) { if (mlist == &msg->list) { found = true; break; @@ -400,12 +408,12 @@ static void ipc_work_handler(struct k_work *work) ipc_send_queued_msg(); - key = k_spin_lock(&ipc->lock); + key = k_spin_lock(&ipc_ks(ipc)->lock); - if (!list_is_empty(&ipc->msg_list) && !ipc->pm_prepare_D3) + if (!list_is_empty(&ipc_ks(ipc)->msg_list) && !ipc_ks(ipc)->pm_prepare_D3) schedule_ipc_worker(); - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); } #endif @@ -473,9 +481,9 @@ int ipc_user_forward_cmd(uint32_t primary, uint32_t extension, unsigned int core pdata->ipc = ipc; /* Prevent host completion until user thread finishes */ - key = k_spin_lock(&ipc->lock); - ipc->task_mask |= IPC_TASK_IN_THREAD; - k_spin_unlock(&ipc->lock, key); + key = k_spin_lock(&ipc_ks(ipc)->lock); + ipc_ks(ipc)->task_mask |= IPC_TASK_IN_THREAD; + k_spin_unlock(&ipc_ks(ipc)->lock, key); /* Wake the user thread */ k_event_set(ipc_user_kevent[core], IPC_USER_EVENT_CMD); @@ -488,10 +496,10 @@ int ipc_user_forward_cmd(uint32_t primary, uint32_t extension, unsigned int core } /* Clear the task mask bit and check for completion */ - key = k_spin_lock(&ipc->lock); - ipc->task_mask &= ~IPC_TASK_IN_THREAD; + key = k_spin_lock(&ipc_ks(ipc)->lock); + ipc_ks(ipc)->task_mask &= ~IPC_TASK_IN_THREAD; ipc_complete_cmd(ipc); - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); return !ret ? pdata->result : ret; } @@ -835,8 +843,8 @@ __cold int ipc_init(struct sof *sof) } memset(ipc->comp_data, 0, SOF_IPC_MSG_MAX_SIZE); - k_spinlock_init(&ipc->lock); - list_init(&ipc->msg_list); + k_spinlock_init(&ipc_ks(ipc)->lock); + list_init(&ipc_ks(ipc)->msg_list); list_init(&ipc->comp_list); #ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS @@ -882,7 +890,7 @@ __cold int ipc_init(struct sof *sof) return platform_ipc_init(ipc); } -/* Locking: call with ipc->lock held and interrupts disabled */ +/* Locking: call with ipc_ks(ipc)->lock held and interrupts disabled */ void ipc_complete_cmd(struct ipc *ipc) { /* @@ -893,11 +901,11 @@ void ipc_complete_cmd(struct ipc *ipc) * is important to only signal the host that the IPC processing has * completed after *all* tasks have completed. Therefore only the last * context should do that. We accomplish this by setting IPC_TASK_* bits - * in ipc->task_mask for each used IPC context and by clearing them when + * in ipc_ks(ipc)->task_mask for each used IPC context and by clearing them when * each of those contexts completes. Only when the mask is 0 we can * signal the host. */ - if (ipc->task_mask) + if (ipc_ks(ipc)->task_mask) return; ipc_platform_complete_cmd(ipc); @@ -915,10 +923,10 @@ static void ipc_complete_task(void *data) struct ipc *ipc = data; k_spinlock_key_t key; - key = k_spin_lock(&ipc->lock); - ipc->task_mask &= ~IPC_TASK_INLINE; + key = k_spin_lock(&ipc_ks(ipc)->lock); + ipc_ks(ipc)->task_mask &= ~IPC_TASK_INLINE; ipc_complete_cmd(ipc); - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); #if CONFIG_GDBSTUB if (ipc_enter_gdb) { ipc_enter_gdb = false; @@ -941,7 +949,7 @@ static enum task_state ipc_do_cmd(void *data) * 32-bit writes are atomic and at the moment no IPC processing is * taking place, so, no need for a lock. */ - ipc->task_mask = IPC_TASK_INLINE; + ipc_ks(ipc)->task_mask = IPC_TASK_INLINE; return ipc_platform_do_cmd(ipc); } diff --git a/src/kernel/idc/zephyr_idc.c b/src/kernel/idc/zephyr_idc.c index bba2d32a5ffd..e40d953598b3 100644 --- a/src/kernel/idc/zephyr_idc.c +++ b/src/kernel/idc/zephyr_idc.c @@ -109,10 +109,10 @@ static void idc_handler(struct k_p4wq_work *work) case IDC_MSG_IPC: idc_cmd(&idc->received_msg); /* Signal the host */ - key = k_spin_lock(&ipc->lock); - ipc->task_mask &= ~IPC_TASK_SECONDARY_CORE; + key = k_spin_lock(&ipc_ks(ipc)->lock); + ipc_ks(ipc)->task_mask &= ~IPC_TASK_SECONDARY_CORE; ipc_complete_cmd(ipc); - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); } } diff --git a/src/kernel/ipc/ipc-zephyr.c b/src/kernel/ipc/ipc-zephyr.c index 5661c72bb269..7b3aaff03c88 100644 --- a/src/kernel/ipc/ipc-zephyr.c +++ b/src/kernel/ipc/ipc-zephyr.c @@ -86,7 +86,7 @@ static void sof_ipc_receive_cb(const void *data, size_t len, void *priv) __ASSERT(len == sizeof(uint32_t) * 2, "Unexpected IPC message length: %zu", len); __ASSERT(data, "IPC data pointer is NULL"); - key = k_spin_lock(&ipc->lock); + key = k_spin_lock(&ipc_ks(ipc)->lock); g_last_data = msg[0]; g_last_ext_data = msg[1]; @@ -96,7 +96,7 @@ static void sof_ipc_receive_cb(const void *data, size_t len, void *priv) #endif ipc_schedule_process(ipc); - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); } #if defined(CONFIG_PM_DEVICE) && defined(CONFIG_IPC_SERVICE_BACKEND_INTEL_ADSP_HOST_IPC) @@ -113,19 +113,19 @@ static int ipc_device_suspend_handler(const struct device *dev, void *arg) int ret = 0; - if (!(ipc->task_mask & IPC_TASK_POWERDOWN)) { + if (!(ipc_ks(ipc)->task_mask & IPC_TASK_POWERDOWN)) { tr_err(&ipc_tr, "ipc task mask not set to IPC_TASK_POWERDOWN. Current value: %u", - ipc->task_mask); + ipc_ks(ipc)->task_mask); ret = -ENOMSG; } - if (!ipc->pm_prepare_D3) { + if (!ipc_ks(ipc)->pm_prepare_D3) { tr_err(&ipc_tr, "power state D3 not requested"); ret = -EBADMSG; } - if (!list_is_empty(&ipc->msg_list)) { + if (!list_is_empty(&ipc_ks(ipc)->msg_list)) { struct list_item *slist; struct ipc_msg *msg; /* The only possible message that can be added during power transition procedure is @@ -134,7 +134,7 @@ static int ipc_device_suspend_handler(const struct device *dev, void *arg) const uint32_t exp_hdr = SOF_IPC4_NOTIF_HEADER(SOF_IPC4_NOTIFY_LOG_BUFFER_STATUS); bool only_buff_status = true; - list_for_item(slist, &ipc->msg_list) { + list_for_item(slist, &ipc_ks(ipc)->msg_list) { msg = container_of(slist, struct ipc_msg, list); if (msg->header != exp_hdr) @@ -167,8 +167,8 @@ static int ipc_device_resume_handler(const struct device *dev, void *arg) struct ipc *ipc = (struct ipc *)arg; ipc_set_drvdata(ipc, NULL); - ipc->task_mask = 0; - ipc->pm_prepare_D3 = false; + ipc_ks(ipc)->task_mask = 0; + ipc_ks(ipc)->pm_prepare_D3 = false; /* schedule task */ #if CONFIG_TWB_IPC_TASK @@ -236,8 +236,8 @@ enum task_state ipc_platform_do_cmd(struct ipc *ipc) dbg_path_hot_stop_watching(); - if (ipc->task_mask & IPC_TASK_POWERDOWN || - ipc_get()->pm_prepare_D3) { + if (ipc_ks(ipc)->task_mask & IPC_TASK_POWERDOWN || + ipc_ks(ipc_get())->pm_prepare_D3) { #if defined(CONFIG_PM) /** * @note For primary core this function diff --git a/src/kernel/ipc/ipc3/handler.c b/src/kernel/ipc/ipc3/handler.c index 38694e10984d..8f63b4156c34 100644 --- a/src/kernel/ipc/ipc3/handler.c +++ b/src/kernel/ipc/ipc3/handler.c @@ -503,15 +503,15 @@ static int ipc_stream_trigger(uint32_t header) if (pipeline_is_timer_driven(pcm_dev->cd->pipeline)) { k_spinlock_key_t key; - key = k_spin_lock(&ipc->lock); - ipc->task_mask |= IPC_TASK_IN_THREAD; - k_spin_unlock(&ipc->lock, key); + key = k_spin_lock(&ipc_ks(ipc)->lock); + ipc_ks(ipc)->task_mask |= IPC_TASK_IN_THREAD; + k_spin_unlock(&ipc_ks(ipc)->lock, key); ret = pipeline_trigger(pcm_dev->cd->pipeline, pcm_dev->cd, cmd); if (ret <= 0) { - key = k_spin_lock(&ipc->lock); - ipc->task_mask &= ~IPC_TASK_IN_THREAD; - k_spin_unlock(&ipc->lock, key); + key = k_spin_lock(&ipc_ks(ipc)->lock); + ipc_ks(ipc)->task_mask &= ~IPC_TASK_IN_THREAD; + k_spin_unlock(&ipc_ks(ipc)->lock, key); } } else { ret = pipeline_trigger_run(pcm_dev->cd->pipeline, pcm_dev->cd, cmd); @@ -696,7 +696,7 @@ static int ipc_pm_context_save(uint32_t header) /* write the context to the host driver */ //mailbox_hostbox_write(0, pm_ctx, sizeof(*pm_ctx)); #endif /* !defined(CONFIG_LIBRARY) && !defined(CONFIG_ZEPHYR_POSIX) */ - ipc_get()->pm_prepare_D3 = 1; + ipc_ks(ipc_get())->pm_prepare_D3 = 1; return 0; } @@ -707,7 +707,7 @@ static int ipc_pm_context_restore(uint32_t header) tr_info(&ipc_tr, "ipc: pm -> restore"); - ipc_get()->pm_prepare_D3 = 0; + ipc_ks(ipc_get())->pm_prepare_D3 = 0; /* restore context placeholder */ //mailbox_hostbox_write(0, pm_ctx, sizeof(*pm_ctx)); diff --git a/src/kernel/ipc/ipc3/helper.c b/src/kernel/ipc/ipc3/helper.c index 2fb7b52e1f4a..cd69a5eac3f8 100644 --- a/src/kernel/ipc/ipc3/helper.c +++ b/src/kernel/ipc/ipc3/helper.c @@ -734,8 +734,8 @@ void z_impl_ipc_msg_reply(struct sof_ipc_reply *reply) mailbox_hostbox_write(0, reply, reply->hdr.size); - key = k_spin_lock(&ipc->lock); - ipc->task_mask &= ~IPC_TASK_IN_THREAD; + key = k_spin_lock(&ipc_ks(ipc)->lock); + ipc_ks(ipc)->task_mask &= ~IPC_TASK_IN_THREAD; ipc_complete_cmd(ipc); - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); } diff --git a/src/kernel/ipc/ipc4/handler-kernel.c b/src/kernel/ipc/ipc4/handler-kernel.c index 29b25bdf5d7d..173202edb2d1 100644 --- a/src/kernel/ipc/ipc4/handler-kernel.c +++ b/src/kernel/ipc/ipc4/handler-kernel.c @@ -437,7 +437,7 @@ __cold static int ipc4_module_process_dx(struct ipc4_message_request *ipc4) #endif #if defined(CONFIG_PM) - ipc_get()->task_mask |= IPC_TASK_POWERDOWN; + ipc_ks(ipc_get())->task_mask |= IPC_TASK_POWERDOWN; #endif /* do platform specific suspending */ platform_context_save(sof_get()); @@ -447,7 +447,7 @@ __cold static int ipc4_module_process_dx(struct ipc4_message_request *ipc4) platform_timer_stop(timer_get()); #endif #if defined(CONFIG_PM) - ipc_get()->pm_prepare_D3 = 1; + ipc_ks(ipc_get())->pm_prepare_D3 = 1; #endif } @@ -559,10 +559,10 @@ static bool is_notification_queued(struct ipc_msg *msg) k_spinlock_key_t key; bool queued = false; - key = k_spin_lock(&ipc->lock); + key = k_spin_lock(&ipc_ks(ipc)->lock); if (!list_is_empty(&msg->list)) queued = true; - k_spin_unlock(&ipc->lock, key); + k_spin_unlock(&ipc_ks(ipc)->lock, key); return queued; } @@ -704,7 +704,8 @@ void ipc_cmd(struct ipc_cmd_hdr *_hdr) */ /* Reply prepared by secondary core */ - if ((ipc->task_mask & IPC_TASK_SECONDARY_CORE) && cpu_is_primary(cpu_get_id())) + if ((ipc_ks(ipc)->task_mask & IPC_TASK_SECONDARY_CORE) && + cpu_is_primary(cpu_get_id())) return; /* Reply has been prepared by secondary core */ if (msg_reply.header != in->primary.dat) @@ -714,7 +715,7 @@ void ipc_cmd(struct ipc_cmd_hdr *_hdr) * The reply is going to be sent as part of the power down * sequence */ - if (ipc->task_mask & IPC_TASK_POWERDOWN) + if (ipc_ks(ipc)->task_mask & IPC_TASK_POWERDOWN) return; if (ipc_wait_for_compound_msg() != 0) { diff --git a/zephyr/lib/pm_runtime.c b/zephyr/lib/pm_runtime.c index 120bb63d7e14..dd238b9b9708 100644 --- a/zephyr/lib/pm_runtime.c +++ b/zephyr/lib/pm_runtime.c @@ -46,7 +46,7 @@ const struct pm_state_info *pm_policy_next_state(uint8_t cpu, int32_t ticks) continue; /* skipping when some ipc task is not finished */ - if (ipc_get()->task_mask || !ipc_platform_poll_is_host_ready()) + if (ipc_ks(ipc_get())->task_mask || !ipc_platform_poll_is_host_ready()) continue; } From 990453a08fccab5f7e27a56c8cbeb4b31cbf5cca Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Tue, 10 Nov 2026 19:47:27 +0200 Subject: [PATCH 2/7] ipc: move IPC processing task into kernel-only state The IPC processing task is run by the kernel EDF or TWB scheduler, which calls through its ops and reads its scheduling state. With CONFIG_SOF_USERSPACE_LL it is embedded in struct ipc, which LL and IPC user threads can write, so a user thread could redirect kernel execution through it. Move it to struct ipc_kstate when CONFIG_SOF_USERSPACE_LL is set. Signed-off-by: Kai Vehmanen --- src/include/sof/ipc/common.h | 19 ++++++++++++++----- src/ipc/ipc-common.c | 4 ++-- src/kernel/ipc/ipc-zephyr.c | 8 ++++---- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/include/sof/ipc/common.h b/src/include/sof/ipc/common.h index bb770ed3681f..296eacb9baad 100644 --- a/src/include/sof/ipc/common.h +++ b/src/include/sof/ipc/common.h @@ -103,16 +103,23 @@ struct ipc_user { * Kernel-only IPC state. * * With CONFIG_SOF_USERSPACE_LL, struct ipc is in a partition that the LL and - * IPC user threads can write. The send queue, its lock and the completion - * state are used by kernel code only, so they are kept here instead. The - * field names match the struct ipc fields used without - * CONFIG_SOF_USERSPACE_LL, so code accesses both through ipc_ks(). + * IPC user threads can write. The send queue, its lock, the completion + * state and the IPC processing task are used by kernel code only, so they + * are kept here instead. The field names match the struct ipc fields used + * without CONFIG_SOF_USERSPACE_LL, so code accesses both through ipc_ks(). */ struct ipc_kstate { struct k_spinlock lock; /* locking mechanism */ int pm_prepare_D3; /* do we need to prepare for D3 */ struct list_item msg_list; /* queue of messages to be sent */ uint32_t task_mask; /* tasks to be completed by this IPC */ + + /* processing task, run by the kernel scheduler */ +#if CONFIG_TWB_IPC_TASK + struct task *ipc_task; +#else + struct task ipc_task; +#endif }; /** Kernel context only. */ @@ -145,12 +152,14 @@ struct ipc { struct list_item comp_list; /* list of component devices */ - /* processing task */ +#ifndef CONFIG_SOF_USERSPACE_LL + /* processing task, in struct ipc_kstate with CONFIG_SOF_USERSPACE_LL */ #if CONFIG_TWB_IPC_TASK struct task *ipc_task; #else struct task ipc_task; #endif +#endif #ifdef CONFIG_SOF_USERSPACE_LL struct ipc_user *ipc_user_pdata; diff --git a/src/ipc/ipc-common.c b/src/ipc/ipc-common.c index f051d51f4e73..870e22e68db4 100644 --- a/src/ipc/ipc-common.c +++ b/src/ipc/ipc-common.c @@ -420,9 +420,9 @@ static void ipc_work_handler(struct k_work *work) void ipc_schedule_process(struct ipc *ipc) { #if CONFIG_TWB_IPC_TASK - schedule_task(ipc->ipc_task, 0, IPC_PERIOD_USEC); + schedule_task(ipc_ks(ipc)->ipc_task, 0, IPC_PERIOD_USEC); #else - schedule_task(&ipc->ipc_task, 0, IPC_PERIOD_USEC); + schedule_task(&ipc_ks(ipc)->ipc_task, 0, IPC_PERIOD_USEC); #endif } diff --git a/src/kernel/ipc/ipc-zephyr.c b/src/kernel/ipc/ipc-zephyr.c index 7b3aaff03c88..e9f452fa3577 100644 --- a/src/kernel/ipc/ipc-zephyr.c +++ b/src/kernel/ipc/ipc-zephyr.c @@ -172,11 +172,11 @@ static int ipc_device_resume_handler(const struct device *dev, void *arg) /* schedule task */ #if CONFIG_TWB_IPC_TASK - scheduler_twb_task_init(&ipc->ipc_task, SOF_UUID(zipc_task_uuid), + scheduler_twb_task_init(&ipc_ks(ipc)->ipc_task, SOF_UUID(zipc_task_uuid), &ipc_task_ops, ipc, 0, "IPC", ZEPHYR_TWB_STACK_SIZE, CONFIG_TWB_THREAD_MEDIUM_PRIORITY, ZEPHYR_TWB_BUDGET_MAX / 2); #else - schedule_task_init_edf(&ipc->ipc_task, SOF_UUID(zipc_task_uuid), + schedule_task_init_edf(&ipc_ks(ipc)->ipc_task, SOF_UUID(zipc_task_uuid), &ipc_task_ops, ipc, 0, 0); #endif return 0; @@ -306,11 +306,11 @@ int platform_ipc_init(struct ipc *ipc) /* schedule task */ #if CONFIG_TWB_IPC_TASK - scheduler_twb_task_init(&ipc->ipc_task, SOF_UUID(zipc_task_uuid), + scheduler_twb_task_init(&ipc_ks(ipc)->ipc_task, SOF_UUID(zipc_task_uuid), &ipc_task_ops, ipc, 0, "IPC", ZEPHYR_TWB_STACK_SIZE, CONFIG_TWB_THREAD_MEDIUM_PRIORITY, ZEPHYR_TWB_BUDGET_MAX / 2); #else - schedule_task_init_edf(&ipc->ipc_task, SOF_UUID(zipc_task_uuid), + schedule_task_init_edf(&ipc_ks(ipc)->ipc_task, SOF_UUID(zipc_task_uuid), &ipc_task_ops, ipc, 0, 0); #endif /* configure interrupt - work is done internally by Zephyr API */ From 402a2d80c326c863806b9bc6e0ac2216182f80f8 Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Tue, 10 Nov 2026 19:50:21 +0200 Subject: [PATCH 3/7] ipc4: use a kernel copy of the IPC data buffer pointer With CONFIG_SOF_USERSPACE_LL, ipc->comp_data is in user-writable struct ipc. Kernel code builds replies from this buffer and passes it to ipc_msg_send(), which copies it into the reply message when the two pointers differ, so a user thread could redirect a kernel memory copy or make the kernel send other memory to the host. The reply to a forwarded LARGE_CONFIG_GET also took its payload pointer and size from struct ipc_user, which user threads can write. Keep a copy of the buffer pointer in struct ipc_kstate and use it in the kernel paths. For forwarded LARGE_CONFIG_GET replies, always reply from this buffer, where the IPC user thread writes the reply, and check the size read from struct ipc_user once against the message and mailbox limits. Signed-off-by: Kai Vehmanen --- src/include/sof/ipc/common.h | 4 ++++ src/ipc/ipc-common.c | 3 +++ src/ipc/ipc4/handler-user.c | 25 +++++++++++++++++++------ src/kernel/idc/idc.c | 2 +- src/kernel/ipc/ipc4/handler-kernel.c | 4 ++-- 5 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/include/sof/ipc/common.h b/src/include/sof/ipc/common.h index 296eacb9baad..fa43adea963c 100644 --- a/src/include/sof/ipc/common.h +++ b/src/include/sof/ipc/common.h @@ -107,9 +107,13 @@ struct ipc_user { * state and the IPC processing task are used by kernel code only, so they * are kept here instead. The field names match the struct ipc fields used * without CONFIG_SOF_USERSPACE_LL, so code accesses both through ipc_ks(). + * + * comp_data stays in struct ipc for the IPC user thread, but kernel code + * must use the copy here, ipc_ks(ipc)->comp_data, to find the buffer. */ struct ipc_kstate { struct k_spinlock lock; /* locking mechanism */ + void *comp_data; /* kernel copy of ipc->comp_data */ int pm_prepare_D3; /* do we need to prepare for D3 */ struct list_item msg_list; /* queue of messages to be sent */ uint32_t task_mask; /* tasks to be completed by this IPC */ diff --git a/src/ipc/ipc-common.c b/src/ipc/ipc-common.c index 870e22e68db4..8831c656dd72 100644 --- a/src/ipc/ipc-common.c +++ b/src/ipc/ipc-common.c @@ -842,6 +842,9 @@ __cold int ipc_init(struct sof *sof) sof_panic(SOF_IPC_PANIC_IPC); } memset(ipc->comp_data, 0, SOF_IPC_MSG_MAX_SIZE); +#ifdef CONFIG_SOF_USERSPACE_LL + ipc_ks(ipc)->comp_data = ipc->comp_data; +#endif k_spinlock_init(&ipc_ks(ipc)->lock); list_init(&ipc_ks(ipc)->msg_list); diff --git a/src/ipc/ipc4/handler-user.c b/src/ipc/ipc4/handler-user.c index 0af33fe7fd1c..8d71b6db7cf6 100644 --- a/src/ipc/ipc4/handler-user.c +++ b/src/ipc/ipc4/handler-user.c @@ -762,13 +762,13 @@ __cold static int ipc4_process_ipcgtw_cmd(struct ipc4_message_request *ipc4) uint32_t reply_size = 0; int err; - err = copier_ipcgtw_process((const struct ipc4_ipcgtw_cmd *)ipc4, ipc->comp_data, + err = copier_ipcgtw_process((const struct ipc4_ipcgtw_cmd *)ipc4, ipc_ks(ipc)->comp_data, &reply_size); /* reply size is returned in header extension dword */ msg_reply->extension = reply_size; if (reply_size > 0) { - msg_reply->tx_data = ipc->comp_data; + msg_reply->tx_data = ipc_ks(ipc)->comp_data; msg_reply->tx_size = reply_size; } @@ -1329,7 +1329,7 @@ __cold static int ipc4_get_large_config_module_instance(struct ipc4_message_requ struct ipc4_module_large_config_reply reply; const struct ipc4_module_large_config *config = (const struct ipc4_module_large_config *)ipc4; - char *data = ipc_get()->comp_data; + char *data = ipc_ks(ipc_get())->comp_data; const struct comp_driver *drv; struct comp_dev *dev = NULL; uint32_t data_offset; @@ -1640,7 +1640,7 @@ __cold static int ipc4_set_large_config_module_instance(struct ipc4_message_requ } else { #if CONFIG_LIBRARY struct ipc *ipc = ipc_get(); - const char *data = (const char *)ipc->comp_data + sizeof(*config); + const char *data = (const char *)ipc_ks(ipc)->comp_data + sizeof(*config); #else const char *data = (const char *)MAILBOX_HOSTBOX_BASE; #endif @@ -1784,10 +1784,23 @@ __cold int ipc4_user_process_module_message(struct ipc4_message_request *ipc4, ipc4_user_target_core_module(ipc4)); if (!ret) { struct ipc_user *pdata = ipc_user_get(); + /* + * pdata is writable by user threads: read the + * size once and always reply from the kernel + * copy of the IPC data buffer, which is where + * the user thread wrote the reply. + */ + uint32_t size = pdata->reply_tx_size; + + if (size > SOF_IPC_MSG_MAX_SIZE || size > MAILBOX_DSPBOX_SIZE) { + ipc_cmd_err(&ipc_tr, "reply size %u too large", size); + ret = IPC4_INVALID_CONFIG_DATA_STRUCT; + break; + } msg_reply->extension = pdata->reply_ext; - msg_reply->tx_size = pdata->reply_tx_size; - msg_reply->tx_data = pdata->reply_tx_data; + msg_reply->tx_size = size; + msg_reply->tx_data = ipc_ks(ipc_get())->comp_data; } } else { /* Base firmware (module_id==0): keep in kernel — diff --git a/src/kernel/idc/idc.c b/src/kernel/idc/idc.c index 8fdecb8ba173..5bdc8477f0bf 100644 --- a/src/kernel/idc/idc.c +++ b/src/kernel/idc/idc.c @@ -81,7 +81,7 @@ static void idc_ipc(void) { struct ipc *ipc = ipc_get(); - ipc_cmd(ipc->comp_data); + ipc_cmd(ipc_ks(ipc)->comp_data); } static int idc_ipc4_bind(uint32_t comp_id) diff --git a/src/kernel/ipc/ipc4/handler-kernel.c b/src/kernel/ipc/ipc4/handler-kernel.c index 173202edb2d1..c980c94d2d15 100644 --- a/src/kernel/ipc/ipc4/handler-kernel.c +++ b/src/kernel/ipc/ipc4/handler-kernel.c @@ -108,7 +108,7 @@ static inline struct ipc4_message_request *ipc4_get_message_request(void) static inline void ipc4_send_reply(struct ipc4_message_reply *reply) { struct ipc *ipc = ipc_get(); - char *data = ipc->comp_data; + char *data = ipc_ks(ipc)->comp_data; ipc_msg_send(&msg_reply, data, true); } @@ -481,7 +481,7 @@ __cold static int ipc4_process_module_message(struct ipc4_message_request *ipc4) __cold struct ipc_cmd_hdr *mailbox_validate(void) { - struct ipc_cmd_hdr *hdr = ipc_get()->comp_data; + struct ipc_cmd_hdr *hdr = ipc_ks(ipc_get())->comp_data; assert_can_be_cold(); From 0f84e4dfb1555808aa066e1fd9ba988ab902e227 Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Tue, 10 Nov 2026 20:00:21 +0200 Subject: [PATCH 4/7] ipc: queue user-originated messages through kernel-owned slots The ipc_msg_send system call linked the struct ipc_msg of the calling user thread into the kernel send queue. The send work queue then called msg->callback in supervisor mode and read the header, size and payload pointer from user memory, all of which the thread can change at any time. A DP module thread could use this to run any kernel code or to corrupt the kernel queue. The validator also faulted a thread that sent a message again while it was still queued, which modules such as tdfb and mfcc do. Copy the header and payload once into a kernel-owned slot instead and queue the slot. The slot is found again by the address of the user message and the memory domain of the sender, neither of which is dereferenced. Sending a message that is still queued updates the queued copy, which matches the in-place update done for kernel messages. ipc_msg_list_remove now removes the matching slot, and does nothing if there is none. User messages are limited to CONFIG_SOF_IPC_USER_MSG_MAX_SIZE bytes, and a message is dropped if all CONFIG_SOF_IPC_USER_MSG_SLOTS slots are in use. Add ipc_user_msg_purge() for dropping the queued messages of a memory domain before it is reused. Signed-off-by: Kai Vehmanen --- src/include/sof/ipc/msg.h | 16 ++++ src/ipc/ipc-common.c | 169 +++++++++++++++++++++++++++++++------- zephyr/Kconfig | 20 +++++ 3 files changed, 175 insertions(+), 30 deletions(-) diff --git a/src/include/sof/ipc/msg.h b/src/include/sof/ipc/msg.h index 943faf6df813..a9ae763519d8 100644 --- a/src/include/sof/ipc/msg.h +++ b/src/include/sof/ipc/msg.h @@ -113,6 +113,22 @@ void z_impl_ipc_msg_send(struct ipc_msg *msg, void *data, #define ipc_msg_send z_impl_ipc_msg_send #endif +#ifdef CONFIG_USERSPACE +struct k_mem_domain; + +/** + * \brief Drop queued messages sent by user threads of a memory domain. + * + * Messages sent by user threads are queued as kernel copies, found by the + * address of the user message and the memory domain of the sender. Call + * this before a memory domain is reused, so that a message still queued + * from its previous threads is not taken as theirs. Kernel context only. + * + * @param domain The memory domain. + */ +void ipc_user_msg_purge(struct k_mem_domain *domain); +#endif + /** * \brief Send an IPC message directly for emergency. * @param msg The IPC message to be freed. diff --git a/src/ipc/ipc-common.c b/src/ipc/ipc-common.c index 8831c656dd72..da0695a6b098 100644 --- a/src/ipc/ipc-common.c +++ b/src/ipc/ipc-common.c @@ -263,7 +263,8 @@ static void schedule_ipc_worker(void) * note: in XTOS builds, this is handled in * task_main_primary_core() */ -#ifdef __ZEPHYR__ +#if defined(__ZEPHYR__) && !CONFIG_SOF_BOOT_TEST_STANDALONE + /* with SOF_BOOT_TEST_STANDALONE, ipc_init() does not set up the work queue */ k_work_schedule_for_queue(&ipc_send_wq, &ipc_send_work, K_USEC(IPC_PERIOD_USEC)); #endif } @@ -337,25 +338,141 @@ void z_impl_ipc_msg_send(struct ipc_msg *msg, void *data, bool high_priority) EXPORT_SYMBOL(ipc_msg_send); #ifdef CONFIG_USERSPACE +/* + * Messages from user threads are never linked into the kernel send queue + * and their callbacks are never called. Each send copies the header and the + * payload once into a kernel-owned slot, which is queued in their place. + * The slot is found again by the address of the user message and the + * memory domain of the sender. Neither is dereferenced, so the user message + * may change or go away at any time without affecting the kernel. + */ +struct ipc_user_msg_slot { + struct ipc_msg msg; /* in ipc_ks(ipc)->msg_list if key is set */ + const struct ipc_msg *key; /* user message, NULL if slot is free */ + struct k_mem_domain *domain; /* memory domain of the sender */ + uint32_t payload[DIV_ROUND_UP(CONFIG_SOF_IPC_USER_MSG_MAX_SIZE, sizeof(uint32_t))]; +}; + +BUILD_ASSERT(CONFIG_SOF_IPC_USER_MSG_MAX_SIZE <= SOF_IPC_MSG_MAX_SIZE, + "user IPC messages must fit in the IPC mailbox"); + +static struct ipc_user_msg_slot ipc_user_msg_slots[CONFIG_SOF_IPC_USER_MSG_SLOTS]; + +static struct k_mem_domain *ipc_user_msg_domain(void) +{ + return k_current_get()->mem_domain_info.mem_domain; +} + +static void ipc_user_msg_slot_free(struct ipc_user_msg_slot *slot) +{ + slot->key = NULL; + slot->domain = NULL; +} + +/* Called by ipc_send_queued_msg() with the IPC lock held, after sending */ +static void ipc_user_msg_sent(struct ipc_msg *msg) +{ + ipc_user_msg_slot_free(container_of(msg, struct ipc_user_msg_slot, msg)); +} + +/* Call with the IPC lock held */ +static struct ipc_user_msg_slot *ipc_user_msg_find(const struct ipc_msg *key, + struct k_mem_domain *domain) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(ipc_user_msg_slots); i++) + if (ipc_user_msg_slots[i].key == key && + ipc_user_msg_slots[i].domain == domain) + return &ipc_user_msg_slots[i]; + + return NULL; +} + +static void ipc_user_msg_queue(const struct ipc_msg *key, const struct ipc_msg *kmsg, + const void *payload, bool high_priority) +{ + struct k_mem_domain *domain = ipc_user_msg_domain(); + struct ipc *ipc = ipc_get(); + struct ipc_user_msg_slot *slot; + k_spinlock_key_t lock_key; + + lock_key = k_spin_lock(&ipc_ks(ipc)->lock); + + /* a message that is still queued is updated in place, not queued again */ + slot = ipc_user_msg_find(key, domain); + if (!slot) { + slot = ipc_user_msg_find(NULL, NULL); + if (!slot) { + k_spin_unlock(&ipc_ks(ipc)->lock, lock_key); + LOG_WRN("no free slot, dropping user IPC message %#x", kmsg->header); + return; + } + + slot->key = key; + slot->domain = domain; + if (high_priority) + list_item_prepend(&slot->msg.list, &ipc_ks(ipc)->msg_list); + else + list_item_append(&slot->msg.list, &ipc_ks(ipc)->msg_list); + } + + slot->msg.header = kmsg->header; + slot->msg.extension = kmsg->extension; + slot->msg.tx_size = kmsg->tx_size; + slot->msg.tx_data = slot->payload; + slot->msg.callback = ipc_user_msg_sent; + memcpy(slot->payload, payload, kmsg->tx_size); + + schedule_ipc_worker(); + + k_spin_unlock(&ipc_ks(ipc)->lock, lock_key); +} + +void ipc_user_msg_purge(struct k_mem_domain *domain) +{ + struct ipc *ipc = ipc_get(); + k_spinlock_key_t key; + int i; + + key = k_spin_lock(&ipc_ks(ipc)->lock); + + for (i = 0; i < ARRAY_SIZE(ipc_user_msg_slots); i++) { + struct ipc_user_msg_slot *slot = &ipc_user_msg_slots[i]; + + if (slot->key && slot->domain == domain) { + list_item_del(&slot->msg.list); + ipc_user_msg_slot_free(slot); + } + } + + k_spin_unlock(&ipc_ks(ipc)->lock, key); +} + void z_vrfy_ipc_msg_send(struct ipc_msg *msg, void *data, bool high_priority) { /* No caller check: DP modules send notifications, see mod_ipc_msg_init() */ - K_OOPS(K_SYSCALL_MEMORY_WRITE(msg, sizeof(*msg))); + uint32_t payload[DIV_ROUND_UP(CONFIG_SOF_IPC_USER_MSG_MAX_SIZE, sizeof(uint32_t))]; + struct ipc_msg kmsg; + + /* only the header fields are used, list and callback are ignored */ + K_OOPS(k_usermode_from_copy(&kmsg, msg, sizeof(kmsg))); - K_OOPS(K_SYSCALL_VERIFY(msg->tx_size <= SOF_IPC_MSG_MAX_SIZE)); + K_OOPS(K_SYSCALL_VERIFY(kmsg.tx_size <= CONFIG_SOF_IPC_USER_MSG_MAX_SIZE)); - K_OOPS(K_SYSCALL_VERIFY(list_is_empty(&msg->list))); + if (kmsg.tx_size) { + /* as in z_impl_ipc_msg_send(), data replaces the message payload */ + const void *src = data ? data : kmsg.tx_data; - if (msg->tx_size > 0) { - K_OOPS(K_SYSCALL_VERIFY(msg->tx_data != NULL)); - K_OOPS(K_SYSCALL_MEMORY_WRITE(msg->tx_data, msg->tx_size)); - } + K_OOPS(K_SYSCALL_VERIFY(src != NULL)); + K_OOPS(k_usermode_from_copy(payload, src, kmsg.tx_size)); - /* If data != NULL and tx_size > 0, verify the data buffer */ - if (data && msg->tx_size > 0) - K_OOPS(K_SYSCALL_MEMORY_READ(data, msg->tx_size)); + /* keep the message payload up to date, as z_impl_ipc_msg_send() does */ + if (data && data != kmsg.tx_data) + K_OOPS(k_usermode_to_copy(kmsg.tx_data, payload, kmsg.tx_size)); + } - z_impl_ipc_msg_send(msg, data, high_priority); + ipc_user_msg_queue(msg, &kmsg, payload, high_priority); } #include #endif /* CONFIG_USERSPACE */ @@ -375,27 +492,19 @@ void z_vrfy_ipc_msg_list_remove(struct ipc_msg *msg) { /* No caller check: called by mod_ipc_msg_free() in DP module threads */ struct ipc *ipc = ipc_get(); - struct list_item *mlist, *_mlist; - bool found = false; - - K_OOPS(K_SYSCALL_MEMORY_WRITE(msg, sizeof(*msg))); + struct ipc_user_msg_slot *slot; + k_spinlock_key_t key; - /* - * special case: empty list was passed. we can't trust where - * list->prev points to, so do not pass to - * z_impl_ipc_msg_list_remove(), but handle here - */ - if (list_is_empty(&msg->list)) - return; + /* msg is only used as a key, removing a message that is not queued is a no-op */ + key = k_spin_lock(&ipc_ks(ipc)->lock); - list_for_item_safe(mlist, _mlist, &ipc_ks(ipc)->msg_list) { - if (mlist == &msg->list) { - found = true; - break; - } + slot = ipc_user_msg_find(msg, ipc_user_msg_domain()); + if (slot) { + list_item_del(&slot->msg.list); + ipc_user_msg_slot_free(slot); } - K_OOPS(K_SYSCALL_VERIFY(found)); - z_impl_ipc_msg_list_remove(msg); + + k_spin_unlock(&ipc_ks(ipc)->lock, key); } #include #endif /* CONFIG_USERSPACE */ diff --git a/zephyr/Kconfig b/zephyr/Kconfig index fdefb6182e3f..efe1a3cb86d0 100644 --- a/zephyr/Kconfig +++ b/zephyr/Kconfig @@ -361,6 +361,26 @@ config STACK_SIZE_IPC_TX help IPC sender work-queue thread stack size. Keep a power of 2. +config SOF_IPC_USER_MSG_SLOTS + int "Number of IPC messages from user threads that can be queued" + default 16 + depends on USERSPACE + help + IPC messages sent by user threads, for example module + notifications, are copied to kernel-owned slots before they + are queued for sending to the host. A message sent while all + slots are in use is dropped. + +config SOF_IPC_USER_MSG_MAX_SIZE + int "Maximum payload size of an IPC message from a user thread" + default 64 + depends on USERSPACE + help + Maximum payload size in bytes of an IPC message sent by a + user thread. Each of the SOF_IPC_USER_MSG_SLOTS slots reserves + this much kernel memory. Sending a larger message faults the + sending thread. + config SOF_IPC_USER_THREAD_STACK_SIZE int "IPC user thread stack size" default 8192 From 66222bbe14ad15e5a2586831bec36dd986eeecca Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Tue, 10 Nov 2026 20:00:48 +0200 Subject: [PATCH 5/7] ipc: drop queued user IPC messages when freeing a module domain DP memory domains and userspace proxy domains are reused for new modules. Drop the messages that the threads of a freed domain still have queued, so that they are not sent on behalf of a module that no longer exists, and not matched to messages of the next module using the domain. Signed-off-by: Kai Vehmanen --- src/kernel/audio/module_adapter/library/userspace_proxy.c | 3 +++ src/schedule/zephyr_dp_schedule_application.c | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/src/kernel/audio/module_adapter/library/userspace_proxy.c b/src/kernel/audio/module_adapter/library/userspace_proxy.c index a861c9670cc5..c7b667abfa2f 100644 --- a/src/kernel/audio/module_adapter/library/userspace_proxy.c +++ b/src/kernel/audio/module_adapter/library/userspace_proxy.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -494,6 +495,8 @@ void userspace_proxy_destroy(const struct comp_driver *drv, struct userspace_con { tr_dbg(&userspace_proxy_tr, "userspace proxy destroy"); user_work_item_free(user_ctx, drv->user_heap); + /* drop messages still queued by the module, before the domain is reused */ + ipc_user_msg_purge(user_ctx->comp_dom); rfree(user_ctx->comp_dom); k_heap_free(drv->user_heap, user_ctx); } diff --git a/src/schedule/zephyr_dp_schedule_application.c b/src/schedule/zephyr_dp_schedule_application.c index 6c9dafc1db28..c2075c353110 100644 --- a/src/schedule/zephyr_dp_schedule_application.c +++ b/src/schedule/zephyr_dp_schedule_application.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -560,6 +561,9 @@ static void scheduler_dp_domain_free(struct processing_module *pmod, struct k_me } } + /* drop messages still queued by the module, before the domain is reused */ + ipc_user_msg_purge(mdom); + /* All partitions removed, the domain can be freed now */ pmod->mdom = NULL; k_mem_domain_deinit(mdom); From 8ed8e2448ac3e7976fa41ac9d6f736d8569f2265 Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Tue, 10 Nov 2026 20:24:48 +0200 Subject: [PATCH 6/7] zephyr: test: userspace: add tests for user IPC message queueing Check that the kernel queues its own copy of a message sent by a user thread: changing the user message after sending it, or forging its list pointers and callback, has no effect on the queued message, a message sent again while queued is updated in place, and messages are removed only by their own memory domain. Also check that oversized messages and kernel payload addresses are rejected, and that the number of queued messages is limited to the number of slots. Signed-off-by: Kai Vehmanen --- zephyr/test/CMakeLists.txt | 1 + zephyr/test/userspace/test_ipc_msg_queue.c | 342 +++++++++++++++++++++ 2 files changed, 343 insertions(+) create mode 100644 zephyr/test/userspace/test_ipc_msg_queue.c diff --git a/zephyr/test/CMakeLists.txt b/zephyr/test/CMakeLists.txt index e64b61314da4..ff0265ea1f7e 100644 --- a/zephyr/test/CMakeLists.txt +++ b/zephyr/test/CMakeLists.txt @@ -31,6 +31,7 @@ endif() if(CONFIG_SOF_BOOT_TEST_STANDALONE AND CONFIG_SOF_USERSPACE_LL) zephyr_library_sources(userspace/test_ll_task.c) + zephyr_library_sources(userspace/test_ipc_msg_queue.c) endif() if(CONFIG_SOF_BOOT_TEST_STANDALONE AND CONFIG_SOF_USERSPACE_SYSCALL_CALLER_CHECK AND diff --git a/zephyr/test/userspace/test_ipc_msg_queue.c b/zephyr/test/userspace/test_ipc_msg_queue.c new file mode 100644 index 000000000000..43bfcab66113 --- /dev/null +++ b/zephyr/test/userspace/test_ipc_msg_queue.c @@ -0,0 +1,342 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Copyright(c) 2026 Intel Corporation. + */ + +/* + * Tests for IPC messages queued by user threads: the kernel queues its own + * copy of the message, so later changes to the user message, forged list + * pointers or callbacks have no effect on the kernel send queue. + * + * IPC is not started in standalone boot tests, so queued messages are not + * sent. The tests inspect the kernel send queue directly and drop the test + * messages with ipc_user_msg_purge(). + */ + +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#include + +LOG_MODULE_DECLARE(sof_boot_test, LOG_LEVEL_DBG); + +#define USER_STACKSIZE 2048 + +/* not a valid IPC4 header, never sent: IPC is not started in these tests */ +#define TEST_HEADER 0x5ec0de00 +#define TEST_MSGS (CONFIG_SOF_IPC_USER_MSG_SLOTS + 2) + +static struct k_thread user_thread; +static K_THREAD_STACK_DEFINE(user_stack, USER_STACKSIZE); + +/* + * Messages are sent from two memory domains: a domain of the test, and the + * LL domain, to which the test partition is added. MMU page tables are + * scarce, so the second domain is not a new one. + */ +static struct k_mem_domain domain_a; +static struct k_mem_domain *domain_b; + +/* kernel-only data, used as forged pointers */ +static uint32_t kernel_only_word[16]; + +static void planted_callback(struct ipc_msg *msg) +{ + kernel_only_word[0] = 0xdead; +} + +/* user data, accessible to both test domains */ +K_APPMEM_PARTITION_DEFINE(ipc_msg_test_part); +K_APP_BMEM(ipc_msg_test_part) static struct ipc_msg test_msgs[TEST_MSGS]; +/* large enough for MSG_SEND_OVERSIZE, so that only the size check can fault */ +K_APP_BMEM(ipc_msg_test_part) static uint32_t + test_payload[DIV_ROUND_UP(CONFIG_SOF_IPC_USER_MSG_MAX_SIZE + 1, sizeof(uint32_t))]; +K_APP_BMEM(ipc_msg_test_part) static uint32_t test_data[4]; + +enum msg_test_case { + /* send, then change the message and forge its list and callback */ + MSG_SEND_THEN_CHANGE, + /* send a message with forged list and callback fields */ + MSG_SEND_FORGED_FIELDS, + /* send with a separate data buffer */ + MSG_SEND_DATA, + /* send the same message twice */ + MSG_SEND_TWICE, + /* send a payload larger than CONFIG_SOF_IPC_USER_MSG_MAX_SIZE */ + MSG_SEND_OVERSIZE, + /* send with a kernel address as the payload */ + MSG_SEND_KERNEL_PAYLOAD, + /* send more messages than there are slots */ + MSG_SEND_MANY, + /* remove test_msgs[0] */ + MSG_REMOVE, +}; + +static void msg_set(struct ipc_msg *msg, uint32_t header, uint32_t size) +{ + memset(msg, 0, sizeof(*msg)); + msg->header = header; + msg->tx_size = size; + msg->tx_data = test_payload; + list_init(&msg->list); +} + +static void user_function(void *p1, void *p2, void *p3) +{ + enum msg_test_case test_case = (enum msg_test_case)(uintptr_t)p1; + struct ipc_msg *msg = &test_msgs[0]; + int i; + + zassert_true(k_is_user_context(), "isn't user"); + + switch (test_case) { + case MSG_SEND_THEN_CHANGE: + msg_set(msg, TEST_HEADER, 8); + test_payload[0] = 1; + test_payload[1] = 2; + ipc_msg_send(msg, NULL, false); + + /* none of this may affect the queued message */ + test_payload[0] = 9; + test_payload[1] = 9; + msg->header = TEST_HEADER + 1; + msg->tx_size = 4096; + msg->tx_data = kernel_only_word; + msg->list.next = (struct list_item *)kernel_only_word; + msg->list.prev = (struct list_item *)kernel_only_word; + msg->callback = planted_callback; + return; + case MSG_SEND_FORGED_FIELDS: + msg_set(msg, TEST_HEADER, 4); + test_payload[0] = 3; + msg->list.next = (struct list_item *)kernel_only_word; + msg->list.prev = (struct list_item *)kernel_only_word; + msg->callback = planted_callback; + ipc_msg_send(msg, NULL, false); + return; + case MSG_SEND_DATA: + msg_set(msg, TEST_HEADER, 4); + test_payload[0] = 0; + test_data[0] = 4; + ipc_msg_send(msg, test_data, false); + /* the message payload is updated from data, as for kernel messages */ + zassert_equal(test_payload[0], 4); + return; + case MSG_SEND_TWICE: + msg_set(msg, TEST_HEADER, 4); + test_payload[0] = 5; + ipc_msg_send(msg, NULL, false); + test_payload[0] = 6; + ipc_msg_send(msg, NULL, false); + return; + case MSG_SEND_OVERSIZE: + msg_set(msg, TEST_HEADER, CONFIG_SOF_IPC_USER_MSG_MAX_SIZE + 1); + ipc_msg_send(msg, NULL, false); + break; + case MSG_SEND_KERNEL_PAYLOAD: + msg_set(msg, TEST_HEADER, 4); + msg->tx_data = kernel_only_word; + ipc_msg_send(msg, NULL, false); + break; + case MSG_SEND_MANY: + for (i = 0; i < TEST_MSGS; i++) { + msg_set(&test_msgs[i], TEST_HEADER, 0); + ipc_msg_send(&test_msgs[i], NULL, false); + } + return; + case MSG_REMOVE: + ipc_msg_list_remove(msg); + return; + default: + zassert_unreachable("unknown test case"); + } + + zassert_unreachable("syscall security check did not fault"); +} + +static void run_case(enum msg_test_case test_case, struct k_mem_domain *domain, + bool expect_fault) +{ + int ret; + + k_thread_create(&user_thread, user_stack, USER_STACKSIZE, + user_function, (void *)(uintptr_t)test_case, NULL, NULL, + -1, K_USER, K_FOREVER); + k_mem_domain_add_thread(domain, &user_thread); + + if (expect_fault) + sof_boot_test_set_fault_valid(&user_thread); + + k_thread_start(&user_thread); + ret = k_thread_join(&user_thread, K_FOREVER); + + sof_boot_test_set_fault_valid(NULL); + + zassert_equal(ret, 0, "user thread join failed: %d", ret); +} + +/* + * Count the queued test messages and copy the first one, with up to + * sizeof(test_payload) bytes of its payload. + */ +static int queued_test_msgs(struct ipc_msg *copy, uint32_t *payload) +{ + struct ipc_kstate *ks = ipc_ks(ipc_get()); + struct list_item *item; + k_spinlock_key_t key; + int count = 0; + + key = k_spin_lock(&ks->lock); + + list_for_item(item, &ks->msg_list) { + struct ipc_msg *msg = container_of(item, struct ipc_msg, list); + + if (msg->header != TEST_HEADER) + continue; + + if (!count++ && copy) { + *copy = *msg; + memcpy(payload, msg->tx_data, MIN(msg->tx_size, sizeof(test_payload))); + } + } + + k_spin_unlock(&ks->lock, key); + + return count; +} + +static void check_one_queued(uint32_t size, uint32_t first_word) +{ + uint32_t payload[ARRAY_SIZE(test_payload)] = { 0 }; + struct ipc_msg copy; + + zassert_equal(queued_test_msgs(©, payload), 1, "expected one queued message"); + zassert_equal(copy.tx_size, size); + zassert_equal(payload[0], first_word); + zassert_not_equal(copy.tx_data, test_payload, "kernel queued the user payload"); + zassert_not_equal(copy.callback, planted_callback, "kernel uses the user callback"); +} + +static void *ipc_msg_queue_setup(void) +{ + struct k_mem_partition *parts[] = { &ipc_msg_test_part }; + + zassert_equal(k_mem_domain_init(&domain_a, ARRAY_SIZE(parts), parts), 0); + + domain_b = zephyr_ll_mem_domain(); + zassert_equal(k_mem_domain_add_partition(domain_b, &ipc_msg_test_part), 0); + + return NULL; +} + +static void ipc_msg_queue_teardown(void *fixture) +{ + zassert_equal(k_mem_domain_remove_partition(domain_b, &ipc_msg_test_part), 0); + zassert_equal(k_mem_domain_deinit(&domain_a), 0); +} + +static void ipc_msg_queue_after(void *fixture) +{ + ipc_user_msg_purge(&domain_a); + ipc_user_msg_purge(domain_b); + zassert_equal(queued_test_msgs(NULL, NULL), 0); + zassert_equal(kernel_only_word[0], 0, "kernel-only data was written"); +} + +ZTEST(userspace_ipc_msg_queue, send_copies_once) +{ + uint32_t payload[ARRAY_SIZE(test_payload)] = { 0 }; + struct ipc_msg copy; + + run_case(MSG_SEND_THEN_CHANGE, &domain_a, false); + + zassert_equal(queued_test_msgs(©, payload), 1); + zassert_equal(copy.tx_size, 8); + zassert_equal(payload[0], 1); + zassert_equal(payload[1], 2); + zassert_not_equal(copy.callback, planted_callback); +} + +ZTEST(userspace_ipc_msg_queue, forged_list_and_callback_ignored) +{ + run_case(MSG_SEND_FORGED_FIELDS, &domain_a, false); + check_one_queued(4, 3); +} + +ZTEST(userspace_ipc_msg_queue, send_with_data) +{ + run_case(MSG_SEND_DATA, &domain_a, false); + check_one_queued(4, 4); +} + +ZTEST(userspace_ipc_msg_queue, resend_updates_queued_copy) +{ + run_case(MSG_SEND_TWICE, &domain_a, false); + check_one_queued(4, 6); +} + +ZTEST(userspace_ipc_msg_queue, oversize_rejected) +{ + run_case(MSG_SEND_OVERSIZE, &domain_a, true); + zassert_equal(queued_test_msgs(NULL, NULL), 0); +} + +ZTEST(userspace_ipc_msg_queue, kernel_payload_rejected) +{ + run_case(MSG_SEND_KERNEL_PAYLOAD, &domain_a, true); + zassert_equal(queued_test_msgs(NULL, NULL), 0); +} + +ZTEST(userspace_ipc_msg_queue, slots_limit_queue) +{ + run_case(MSG_SEND_MANY, &domain_a, false); + zassert_equal(queued_test_msgs(NULL, NULL), CONFIG_SOF_IPC_USER_MSG_SLOTS); +} + +ZTEST(userspace_ipc_msg_queue, remove_by_owner_only) +{ + run_case(MSG_SEND_FORGED_FIELDS, &domain_a, false); + + /* same message address, other domain: not removed */ + run_case(MSG_REMOVE, domain_b, false); + zassert_equal(queued_test_msgs(NULL, NULL), 1); + + run_case(MSG_REMOVE, &domain_a, false); + zassert_equal(queued_test_msgs(NULL, NULL), 0); + + /* removing a message that is not queued is a no-op */ + run_case(MSG_REMOVE, &domain_a, false); +} + +ZTEST(userspace_ipc_msg_queue, purge_by_domain) +{ + run_case(MSG_SEND_FORGED_FIELDS, &domain_a, false); + run_case(MSG_SEND_FORGED_FIELDS, domain_b, false); + zassert_equal(queued_test_msgs(NULL, NULL), 2); + + ipc_user_msg_purge(&domain_a); + zassert_equal(queued_test_msgs(NULL, NULL), 1); +} + +ZTEST_SUITE(userspace_ipc_msg_queue, sof_boot_test_standalone_predicate, ipc_msg_queue_setup, + NULL, ipc_msg_queue_after, ipc_msg_queue_teardown); + +/** + * SOF main has booted up and IPC handling is stopped. + * Run test suites with ztest_run_all. + */ +static int run_tests(void) +{ + ztest_run_test_suite(userspace_ipc_msg_queue, false, 1, 1, NULL); + return 0; +} + +SYS_INIT(run_tests, APPLICATION, 99); From a891d2217c8e33a0b332952d3a52b78aca8ec075 Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Tue, 10 Nov 2026 20:44:15 +0200 Subject: [PATCH 7/7] userspace: doc: document queueing of IPC messages from user threads Describe how messages sent by user threads are copied to kernel-owned slots, how they are matched for re-send and removal, and that the IPC send queue state is kernel-only with CONFIG_SOF_USERSPACE_LL. Signed-off-by: Kai Vehmanen --- src/include/sof/userspace/README.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/include/sof/userspace/README.md b/src/include/sof/userspace/README.md index d8efa662aa2d..289be02f7048 100644 --- a/src/include/sof/userspace/README.md +++ b/src/include/sof/userspace/README.md @@ -150,6 +150,16 @@ sending notifications: modules queue messages from `mod_ipc_msg_init()` with processing errors with `send_resource_notif()`. DP modules have no access to the other IPC syscalls or to heap, vregion, DMA, DAI or scheduler syscalls. +A message sent by a user thread is never linked into the kernel send queue. `ipc_msg_send()` +copies its header and payload (at most `CONFIG_SOF_IPC_USER_MSG_MAX_SIZE` bytes) once into +one of `CONFIG_SOF_IPC_USER_MSG_SLOTS` kernel-owned slots and queues the slot. The slot is +found again by the address of the user message and the memory domain of the sender, so +sending a queued message again updates the queued copy, and `ipc_msg_list_remove()` only +removes messages of the caller's own domain. `msg->list` and `msg->callback` of a user +message are not used. With `CONFIG_SOF_USERSPACE_LL`, the send queue, its lock and the IPC +processing task are kept in kernel-only `struct ipc_kstate`, not in the user-writable +`struct ipc`; kernel code accesses them with `ipc_ks()`. + The `zephyr/syscall/*.c` files hold the validators in one place per subsystem; the rows marked **scattered** keep their `z_vrfy_` next to the kernel logic and are the migration targets.