igo_nr: bound host-supplied control and config data by lgirdwood · Pull Request #10909 · thesofproject/sof

lgirdwood · 2026-06-15T09:45:13Z

Hardening of host-supplied (IPC3 bytes-control / config blob) data in igo_nr:

reject an out-of-range element count in the SWITCH get handler before the
loop (out-of-bounds write to the reply otherwise)
validate the config blob is at least the size of the config struct before
copying it (out-of-bounds read otherwise)
compute the processing circular-buffer end pointers in sample elements
rather than bytes, so the wrap check is correct

No functional change for valid configurations.

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>

Copilot

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_end in sample elements (not bytes) to make wrap checks correct.
Reject out-of-range SOF_CTRL_CMD_SWITCH num_elems to prevent reply OOB writes.
Validate config blob size returned from comp_get_data_blob() before copying into cd->config.

lgirdwood · 2026-06-15T10:10:03Z

+	x_end = x_start + x_size / sizeof(*x_start);
+	y_end = y_start + y_size / sizeof(*y_start);


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.

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.

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.

lgirdwood · 2026-06-15T10:10:04Z

 	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);


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.)

if we have a pre-existing check that does not validate old data, then we should add this check too.

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>

lyakh · 2026-06-16T06:13:23Z

 		return comp_data_blob_get_cmd(cd->model_handler, cdata, fragment_size);
 	case SOF_CTRL_CMD_SWITCH:
+		if (cdata->num_elems > SOF_IPC_MAX_CHANNELS) {
+			comp_err(dev, "igo_nr_get_config() error: num_elems %u out of range",


please remove the function name from the log

lyakh · 2026-06-16T06:14:16Z

-	struct sof_igo_nr_config *p_config = comp_get_data_blob(cd->model_handler, NULL, NULL);
+	size_t config_size = 0;
+	struct sof_igo_nr_config *p_config = comp_get_data_blob(cd->model_handler,
+							       &config_size, NULL);


off by 1 indentation. comp_get_data_blob() already (IMHO dubiously) sets *size to 0 immediately on entry, so actually config_size initialisation is redundant. But yes, I think a better approach for passed by reference output parameters is to only set them when the function executes successfully. Unfortunately SOF doesn't have conventions for that.

lyakh · 2026-06-16T06:22:27Z

 	comp_info(dev, "entry");
-	igo_nr_check_config_validity(dev, cd);

 	if (p_config) {


maybe a good opportunity to convert this to if (!p_config) return; instead of adding more code under the if

lyakh · 2026-06-16T06:24:32Z

 	struct comp_dev *dev = mod->dev;

 	comp_info(dev, "entry");
-	igo_nr_check_config_validity(dev, cd);


wow, this was a "great" validity check...

lyakh · 2026-06-16T06:25:42Z

+		 * igo_nr_check_config_validity() re-reads the new blob
+		 */
+		if (igo_nr_check_config_validity(dev, cd) < 0) {
+			comp_err(dev, "New config failed validation, not applied");


igo_nr_check_config_validity() is already quite verbose when failing

lyakh · 2026-06-16T06:28:48Z

+	 * already in samples and need no byte-to-element conversion
+	 */
+	ret = source_get_data_s16(source, request_size, (int16_t const **)&x,
+				  (int16_t const **)&x_start, &x_size);


I think also with pointer-to-pointer upgrading to const doesn't need an explicit type-cast

lyakh · 2026-06-16T06:30:50Z

+	 * already in samples and need no byte-to-element conversion
+	 */
+	ret = source_get_data_s32(source, request_size, (int32_t const **)&x,
+				  (int32_t const **)&x_start, &x_size);


ditto const

lyakh · 2026-06-16T06:31:06Z

+	 * already in samples and need no byte-to-element conversion
+	 */
+	ret = source_get_data_s32(source, request_size, (int32_t const **)&x,
+				  (int32_t const **)&x_start, &x_size);


Copilot AI review requested due to automatic review settings June 15, 2026 09:45

lgirdwood requested review from dbaluta, kv2019i, lbetlej, mmaka1 and plbossart as code owners June 15, 2026 09:45

Copilot AI reviewed Jun 15, 2026

View reviewed changes

lgirdwood force-pushed the fix-igo-nr branch from 53d2844 to 04c0ef2 Compare June 15, 2026 10:09

Copilot started reviewing on behalf of lgirdwood June 15, 2026 10:16 View session

lrgirdwo added 2 commits June 15, 2026 12:57

lgirdwood force-pushed the fix-igo-nr branch from 04c0ef2 to 7f5aebc Compare June 15, 2026 11:58

lyakh requested changes Jun 16, 2026

View reviewed changes

		x_end = x_start + x_size / sizeof(*x_start);
		y_end = y_start + y_size / sizeof(*y_start);

Conversation

lgirdwood commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lgirdwood commented Jun 15, 2026 •

edited

Loading