audio: module_adapter: bound large_config fragment reassembly to buffer size - #11155
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new fragment bounds failure path returns without cleaning up the in-progress reassembly buffer, which can leave the module stuck “busy” and leak memory until teardown.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens IPC4 large configuration (“SET_LARGE_CONFIG”) fragment reassembly in the module adapter to prevent host-controlled out-of-bounds writes by ensuring the reassembly buffer size and copy bounds remain consistent throughout a multi-fragment transfer.
Changes:
- Reject a new FIRST fragment with
-EBUSYwhen a reassembly is already in progress (prevents overwritingmd->new_cfg_sizemid-stream). - In the reassembly path, only commit
md->new_cfg_sizeafter successful allocation ofmd->runtime_params. - Add explicit offset/size bounds checking for intermediate/last fragments before copying into the reassembly buffer.
File summaries
| File | Description |
|---|---|
| src/audio/module_adapter/module/generic.c | Defers new_cfg_size commit until after allocation and adds fragment offset/size bounds validation. |
| src/audio/module_adapter/module_adapter_ipc4.c | Prevents FIRST fragment from restarting reassembly while an earlier one is still buffered. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| offset = data_offset_size; | ||
| if (offset > md->new_cfg_size || | ||
| fragment_size > md->new_cfg_size - offset) { | ||
| comp_err(dev, "fragment (offset %zu, size %zu) exceeds config buffer %zu", | ||
| offset, fragment_size, md->new_cfg_size); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
I think this is good to add, so we can recover to a sane state.
There was a problem hiding this comment.
I was thinking about this even before the commit and honestly I don't know. Copilot suggested using free only during middle/last, but during the first message it is possible to request a smaller buffer and attempt to copy a larger chunk of data (which will be correctly rejected but will lead to the same state).
I was wondering whether to reject IPCs where the config size is smaller than or equal to what fits in the mailbox but arrives with the FIRST bit set without FINAL.
If the host sends an incorrect IPC sequence that fails at any point, I would probably leave the teardown on the host side. This does not protect us against a memory leak at all. The host can send such messages to multiple modules, all with the FIRST bit set allocating memory, and nothing will stop it. The BUSY status is returned only if it concerns a specific module instance.
It might be worth taking a closer look at this IPC later, thoroughly describing the possible flows, documenting what is valid and what is not, and how the FW should behave. I think this flow may contain more ambiguities and gaps.
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Looks good. The one copilot comment on rolling back to known state seems like a good addition.
| offset = data_offset_size; | ||
| if (offset > md->new_cfg_size || | ||
| fragment_size > md->new_cfg_size - offset) { | ||
| comp_err(dev, "fragment (offset %zu, size %zu) exceeds config buffer %zu", | ||
| offset, fragment_size, md->new_cfg_size); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
I think this is good to add, so we can recover to a sane state.
…er size md->new_cfg_size, used to bound every SET_LARGE_CONFIG reassembly copy in module_set_configuration, is written in two places: - module_set_large_config, sets it unconditionally on a FIRST fragment - module_set_configuration, sets it while allocating md->runtime_params to that size Nothing keeps the two in sync when a second FIRST fragment arrives mid-reassembly. module_set_large_config allows to overwrite new_cfg_size with a new (larger) value while runtime_params still holds the previous, smaller allocation. A following MIDDLE/LAST fragment then passes the memcpy_s bound check and writes host-controlled mailbox data out of bounds of the heap buffer. Fix it in the reassembly path: - module_set_large_config: reject a FIRST fragment with -EBUSY when a reassembly is already in progress, before overwriting new_cfg_size. - module_set_configuration: commit new_cfg_size only after runtime_params is allocated, so the two always match, and reject an intermediate/last fragment whose host-supplied offset or size does not fit in the buffer, which otherwise underflows the memcpy_s destination bound. Closes thesofproject#11153 Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
md->new_cfg_size, used to bound every SET_LARGE_CONFIG reassembly copy in module_set_configuration, is written in two places:
Nothing keeps the two in sync when a second FIRST fragment arrives mid-reassembly. module_set_large_config allows to overwrite new_cfg_size with a new (larger) value while runtime_params still holds the previous, smaller allocation.
A following MIDDLE/LAST fragment then passes the memcpy_s bound check and writes host-controlled mailbox data out of bounds of the heap buffer.
Fix it in the reassembly path:
Closes #11153