Repository navigation
Conversation
The IPC4 fuzzer (simple-IPC-fuzz_sh) aborts with an AddressSanitizer stack-overflow report in the ipc_send_wq thread, inside vfprintf() under posix_print_trace(). That report is a red herring: the real failure is an assert, and the recursive assert -> k_panic -> assert loop that follows it is what eventually exhausts the 8MB pthread stack native_sim gives the thread. With -jobs>1 the harness passes -close_fd_mask=1, so printk (which goes to stdout) is discarded and only the secondary ASan report on stderr survives into the CI log. The assert is: ASSERTION FAIL [!dsp_write_err] @ sof/src/include/sof/lib/mailbox.h:52 reached from ipc_send_queued_msg() -> ipc_platform_send_msg() -> mailbox_dspbox_write(0, msg->tx_data, 918016), i.e. a reply claiming ~900KB of payload for a 4KB mailbox. ipc4_get_large_config_module_instance() seeds data_offset from config->extension.r.data_off_size, a 20-bit host-controlled field, and then passes it to drv->ops.get_large_config() as an in/out parameter. A module whose .get_configuration produces no data (template_get_config() is the one the fuzzer found, but any IPC3-oriented stub behaves the same) returns success without writing *data_offset_size, so the host-supplied value survives unchanged and is published as msg_reply->tx_size. The payload itself lives in ipc->comp_data, which is only SOF_IPC_MSG_MAX_SIZE bytes. Only the VENDOR_CONFIG_PARAM branch bounds data_off_size today, and it does so for the inbound hostbox read, not for the reply. With asserts compiled out, memcpy_s() rejects the copy but the return value is discarded and dcache_writeback_region() is still asked to write back the out-of-range length, so the caller sends a reply header advertising a size that was never copied. Bound the reply size against the space actually left in the reply buffer before it is published, and fail the command with IPC4_INVALID_CONFIG_DATA_LEN otherwise. The bound is computed from data rather than from SOF_IPC_MSG_MAX_SIZE directly because the non-vendor branch advances data by sizeof(reply) under CONFIG_LIBRARY. The same fix is applied to ipc4_process_large_config_get(), the CONFIG_SOF_USERSPACE_LL variant, which has the identical flaw. Reproduced and verified on native_sim/native/64 with the crash artifact from the failing CI run (thesofproject/sof PR 11193, job simple-IPC-fuzz_sh (4)): it aborts before the change and exits 0 after it. A subsequent 420s/8-job fuzz run over a ~10k-input corpus produced no new artifacts, and an intel_adsp/ace30/ptl cross-build is clean. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@lgirdwood is this the PR you mentioned in #11237 (comment)? These changes address a different issue, I also came across it in my nightly fuzzer runs. The fix is good. I would probably just remove the comments. They seem unnecessary. |
There was a problem hiding this comment.
🟢 Approval recommended
The bounds checks correctly account for payload offsets and cover both affected reply paths.
0 open findings
What changed in this PR
Bounds IPC4 large-config reply sizes to prevent oversized mailbox writes from host-controlled values.
Changes:
- Validates reply payload size against remaining IPC buffer capacity.
- Applies validation to kernel and userspace LL paths.
| File | Description |
|---|---|
src/ipc/ipc4/handler-user.c |
Rejects oversized large-config replies with IPC4_INVALID_CONFIG_DATA_LEN. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Thanks @tmleman - I'd forgotten about this as last two weeks have been very busy, and thanks for confirming its not the same fix as your PR. Now made non draft. Comments good for me as descriptive, yes Claude is very verbose on the commit message, but I've found deep diving the fuzzer integration time consuming so any context is helpful. |
|
SOFCI TEST |
PR 11239: test resultsRun date: 2026-10-10 10:54 UTC Tested commit: 46016cb03e290bf1f482179f53de04fa9acf3cd1 |
What this fixes
The
simple-IPC-fuzz_shjob has been failing intermittently on unrelated PRs(#11193, #11208, #11214, #11227 among them). The CI log only ever shows this:
That report is a red herring. The real failure is a Zephyr assert; the
assert -> k_panic -> z_spin_lock_validassert loop that follows it is whateventually exhausts the 8MB pthread stack native_sim gives the
ipc_send_wqthread. The actual assert text goes to stdout, which
fuzz.shcloses via-close_fd_mask=1whenever-jobs > 1, so only the secondary ASan report onstderr reaches the log.
Replaying the crash artifact locally shows the real failure immediately:
A reply claiming ~900KB of payload for a 4KB mailbox.
Root cause
ipc4_get_large_config_module_instance()seedsdata_offsetfromconfig->extension.r.data_off_size— a 20-bit host-controlled field — andpasses it to
drv->ops.get_large_config()as an in/out parameter. A modulewhose
.get_configurationproduces no data returns success without writing*data_offset_size, so the host-supplied value survives unchanged and ispublished as
msg_reply->tx_size. The payload itself lives inipc->comp_data, which is onlySOF_IPC_MSG_MAX_SIZEbytes.template_get_config()is the stub the fuzzer happened to find — it isexplicitly documented as "Not used in IPC4 systems" and just returns 0 — but
any IPC3-oriented
.get_configurationbehaves the same way.Only the
VENDOR_CONFIG_PARAMbranch boundsdata_off_sizetoday (added ind94306c), and it does so for the inbound hostbox read, not for the reply.
With asserts compiled out this is not merely cosmetic:
memcpy_s()rejectsthe copy but its return value is discarded,
dcache_writeback_region()isstill asked to write back the out-of-range length, and the caller goes on to
send a reply header advertising a size that was never copied.
The change
Bound the reply size against the space actually left in the reply buffer
before it is published, and fail the command with
IPC4_INVALID_CONFIG_DATA_LENotherwise. The bound is computed fromdatarather than from
SOF_IPC_MSG_MAX_SIZEdirectly because the non-vendor branchadvances
databysizeof(reply)underCONFIG_LIBRARY.The same fix is applied to
ipc4_process_large_config_get(), theCONFIG_SOF_USERSPACE_LLvariant, which has the identical flaw (and alsolacks the vendor-path hostbox bound its sibling has — not addressed here).
Verification
Built for
native_sim/native/64and replayed the crash artifacts from twoindependent failing CI runs:
crash-cd99ca2d...(PR #11193, jobsimple-IPC-fuzz_sh (4))ASSERTION FAIL [!dsp_write_err], rc=1crash-15eb8d66...(PR #11208, jobsimple-IPC-fuzz_sh (4))ASSERTION FAIL [!dsp_write_err], rc=1Two independently-found inputs hitting the identical assert is what makes me
fairly confident this is the single root cause behind the recent intermittent
failures, rather than one of several.
Under gdb the new branch is confirmed to be the one taken:
data_offset = 918016,data_max = 4096,ret = 121(
IPC4_INVALID_CONFIG_DATA_LEN).Also:
intel_adsp/ace30/ptlcross-build clean.Left alone deliberately
Two things make any fuzz assert much harder to diagnose than it should be.
Both look worth separate patches, and I would rather not fold them into a
fix:
ipc_send_queued_msg()holdsipc->lockwhenthe assert fires, and the fatal path then trips
z_spin_lock_valid()repeatedly, so any fuzz assert degenerates into an 8MB stack overflow
instead of stopping at the assert.
-close_fd_mask=1. With-jobs > 1this discards the one line thatidentifies the bug.
Marked draft pending CI.
🤖 Generated with Claude Code