module_adapter: validate config sizes and isolate per-instance state#10920
module_adapter: validate config sizes and isolate per-instance state#10920lgirdwood wants to merge 3 commits into
Conversation
lgirdwood
requested review from
dbaluta,
kv2019i,
lbetlej,
mmaka1,
plbossart and
ranj063
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Hardens module_adapter and codec wrappers by validating configuration buffer sizes before dereferencing and by isolating fragmented transfer reassembly state per component instance to avoid cross-component corruption.
Changes:
- Added per-instance
runtime_params_sizetoprocessing_moduleto track fragmented runtime-params transfer size. - Added bounds checks in Waves TLV parsing and Cadence init parsing to prevent out-of-bounds reads.
- Updated IPC3 module_adapter get/set params path to use per-instance reassembly size instead of a static.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/include/module/module/base.h | Adds per-instance field for fragmented transfer reassembly sizing. |
| src/audio/module_adapter/module_adapter_ipc3.c | Switches from static reassembly size to per-instance state. |
| src/audio/module_adapter/module/waves/waves.c | Adds header-size bounds check before reading TLV header fields. |
| src/audio/module_adapter/module/cadence_ipc4.c | Validates init payload is large enough before reading direction word. |
| init_bytes = (uint8_t *)ext_data->module_data; | ||
| cd->direction = *(uint32_t *)(init_bytes + sizeof(struct snd_codec)); |
There was a problem hiding this comment.
Fixed — the direction word is now read with memcpy into a local uint32_t instead of dereferencing a possibly-unaligned uint32_t *. (Host and DSP are both little-endian for these IPC fields, so no byte-swap is needed.)
| if (index + header_size > cfg->size) { | ||
| comp_err(dev, "module_param header exceeds cfg buffer size: %d", | ||
| cfg->size); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
Fixed — the message now logs the index, header size and config size with matching specifiers: index %u (header %u) exceeds cfg size %zu (cfg->size is size_t).
| * across all module_adapter components and corrupted by interleaved | ||
| * fragmented transfers | ||
| */ | ||
| uint32_t *size = &mod->runtime_params_size; |
There was a problem hiding this comment.
Fixed — dropped the size pointer alias and use mod->runtime_params_size directly in the reassembly arithmetic.
| /* total size of a fragmented runtime-params (get/set) transfer, kept | ||
| * per instance so concurrent transfers to different components do not | ||
| * corrupt each other's reassembly state | ||
| */ | ||
| uint32_t runtime_params_size; |
There was a problem hiding this comment.
Good point — moved the new field to the end of struct processing_module (after the existing private members) so the offsets of all pre-existing fields are unchanged.
| /* direction follows the codec params in init data; make sure the | ||
| * payload is large enough to hold it before dereferencing | ||
| */ | ||
| if (size < (int)(sizeof(struct snd_codec) + sizeof(uint32_t))) { |
There was a problem hiding this comment.
this should be checked right in the beginning of this block, e.g. after line 247 above.
There was a problem hiding this comment.
Done — moved the size check to the top of the block (right after the variable declarations, before the allocation), so a too-small payload is rejected up front.
The init path read the direction word at a fixed offset past the codec params without checking the payload was large enough, reading past the mailbox. Require the payload to cover the field. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The config parse loop read a parameter header before confirming a whole header remained in the blob, reading past it near the end. Bound the read by the remaining size. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The reassembly size for fragmented runtime-params transfers was a file-scope static shared by every component, so interleaved transfers to different components corrupted each other's state. Move it into per-instance module state. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
lgirdwood
force-pushed
the
fix-module-adapter
branch
from
0c0e552 to
64cadca
Compare
Hardening of the module_adapter and a couple of codec wrappers:
direction field past the codec params
file-scope static into per-instance state, so interleaved transfers to
different components can't corrupt each other
No functional change for valid configurations.