copier: validate host-supplied gateway config and queue indices#10908
copier: validate host-supplied gateway config and queue indices#10908lgirdwood wants to merge 5 commits into
Conversation
The gateway configuration length from the init payload was multiplied and used as a copy length from the mailbox without checking it against the actual payload size. Reject a configuration that would read past the init payload. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The multi-gateway count from the gateway config was used to compute sizes and walk the mapping array before being bounded. Reject a count above the supported maximum before any arithmetic. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The sink queue id was used directly to index the output format array when updating parameters, unlike the other call sites which bound it. Skip a sink whose queue id exceeds the output pin count. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Hardens the copier against malformed/host-controlled IPC4 configuration fields to prevent out-of-bounds reads/writes in DSP code paths.
Changes:
- Add length validation for IPC gateway config blobs before dereferencing/copying.
- Validate ALH multi-gateway count before sizing/walking gateway mappings.
- Bound-check host-controlled queue-derived indices and require exact-sized attenuation payloads.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/audio/copier/copier_ipcgtw.c | Validate IPC gateway config length covers required headers/blobs before access |
| src/audio/copier/copier_generic.c | Bound-check queue-derived index before indexing output format array |
| src/audio/copier/copier_dai.c | Validate ALH gateway count before computing config size / walking mappings |
| src/audio/copier/copier.c | Bound total config copy size to mailbox payload and tighten attenuation payload sizing |
| int j; | ||
| j = IPC4_SRC_QUEUE_ID(buf_get_id(sink)); | ||
|
|
||
| /* src_queue id is host-controlled; bound it before indexing out_fmt[] */ | ||
| if (j >= IPC4_COPIER_MODULE_OUTPUT_PINS_COUNT) { | ||
| comp_err(dev, "invalid src_queue id %d", j); | ||
| continue; | ||
| } | ||
|
|
||
| ipc4_update_buffer_format(sink, &cd->out_fmt[j]); |
There was a problem hiding this comment.
IPC4_SRC_QUEUE_ID(x) is ((x) & 0xffff), so j is always in [0, 0xffff] and can't be negative — the >= IPC4_COPIER_MODULE_OUTPUT_PINS_COUNT check is sufficient. This also matches the existing guarded call sites (e.g. copier.c bind handling) which use the same int + >= pattern, so I've kept it consistent.
| if (alh_blob->alh_cfg.count > IPC4_ALH_MAX_NUMBER_OF_GTW) { | ||
| comp_err(mod->dev, "Invalid ALH count: %u", | ||
| alh_blob->alh_cfg.count); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
mod is in scope: the function opens with struct processing_module *mod = comp_mod(dev); and the pre-existing code right above ("No gateway config found in blob!") already logs via comp_err(mod->dev, ...). So this compiles and uses the same device pointer as the surrounding code — no build break.
| if (data_offset != sizeof(uint32_t)) { | ||
| comp_err(dev, "attenuation data size %d is incorrect", data_offset); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
Fixed — changed the format specifier to %u (data_offset is uint32_t).
| if ((size_t)gtw_cfg->config_length * sizeof(uint32_t) < | ||
| sizeof(struct ipc4_gateway_config_data) + | ||
| sizeof(struct ipc4_ipc_gateway_config_blob)) { | ||
| comp_err(dev, "ipc4_gateway_config_data too small: %u", | ||
| gtw_cfg->config_length); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
Fixed — rewrote it to compare dword counts instead of scaling config_length by 4: config_length < DIV_ROUND_UP(sizeof(header) + sizeof(blob), sizeof(uint32_t)). The right-hand side is a small compile-time constant, so there's no multiplication to overflow.
The attenuation setter only rejected oversized payloads, then dereferenced the data as a 32-bit value; a payload shorter than that read past the mailbox. Require exactly a 32-bit payload. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The IPC gateway path read a config blob from the gateway data without checking the declared config length covered it, over-reading the mailbox tail. Reject a config length too small for the gateway config header and blob. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Hardening of host-supplied (IPC4) fields in the copier so a compromised host
cannot drive out-of-bounds access in the DSP. Each commit is independent:
copying it out of the mailbox
gateway config
No functional change for valid configurations.