igo_nr: bound host-supplied control and config data#10909
Conversation
The switch get handler looped over a host-supplied element count while writing the reply channel array and reading per-channel state. Reject counts larger than the maximum channel count before the loop. 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 igo_nr against malformed host-supplied IPC3 control/config data by fixing circular buffer end computations, bounding SWITCH element counts, and validating config blob sizes before copying.
Changes:
- Compute
x_end/y_endin sample elements (not bytes) to make wrap checks correct. - Reject out-of-range
SOF_CTRL_CMD_SWITCHnum_elemsto prevent reply OOB writes. - Validate config blob size returned from
comp_get_data_blob()before copying intocd->config.
| x_end = x_start + x_size / sizeof(*x_start); | ||
| y_end = y_start + y_size / sizeof(*y_start); |
There was a problem hiding this comment.
The buffer sizes returned by source_get_data()/sink_get_buffer() are whole numbers of samples (the streams are sample-aligned), so the division is exact in practice. And even if it weren't, truncating down makes the end pointer smaller, i.e. the wrap/bound check becomes stricter — it can never place the end past the real buffer, so there's no OOB risk. I kept it simple rather than adding a partial-sample check that can't trigger.
There was a problem hiding this comment.
Maybe its better to actually use the sample sizes derivatives of source_get_data e.g. source_get_data_s32 and so on, this way could avoid the uncertainty.
There was a problem hiding this comment.
Done — switched all three capture functions to the sample-count accessors (source_get_data_s16/_s32 and sink_get_buffer_s16/_s32). Those return the buffer size already in samples, so the end pointers are now x_end = x_start + x_size with no byte-to-element division at all. Folded into the "compute circular buffer ends in samples not bytes" commit.
| igo_nr_check_config_validity(dev, cd); | ||
|
|
||
| if (p_config) { | ||
| if (config_size < sizeof(cd->config)) { | ||
| comp_err(dev, "New config too small: %zu < %zu", | ||
| config_size, sizeof(cd->config)); | ||
| return; | ||
| } | ||
| comp_info(dev, "New config detected."); | ||
| cd->config = *p_config; | ||
| igo_nr_print_config(mod); |
There was a problem hiding this comment.
igo_nr_check_config_validity() doesn't look at the old cd->config — it re-reads the incoming blob via comp_get_data_blob(cd->model_handler, ...) and validates that (the same p_config about to be applied), so it does check the new config. (Its return value isn't acted on today, and active_channel_idx is separately bounded at its use site — but that's pre-existing and outside this size-validation change.)
There was a problem hiding this comment.
if we have a pre-existing check that does not validate old data, then we should add this check too.
There was a problem hiding this comment.
Agreed — done. igo_nr_set_igo_params() now acts on the result: it calls igo_nr_check_config_validity() on the incoming blob and skips applying the config (returns without copying into cd->config) if validation fails. Folded into the "validate config blob size before copy" commit.
A new configuration blob was copied into the component state as a fixed-size structure without checking the blob was large enough, reading past the allocation for a short blob. Request the blob size and reject one smaller than the configuration structure. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The processing buffers' end pointers were computed by adding a byte count to sample-typed pointers, placing the end past the real buffer and letting the wrap check pass too late. Convert the byte sizes to element counts when computing the ends. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Hardening of host-supplied (IPC3 bytes-control / config blob) data in igo_nr:
loop (out-of-bounds write to the reply otherwise)
copying it (out-of-bounds read otherwise)
rather than bytes, so the wrap check is correct
No functional change for valid configurations.