audio: crossover: Improve robustness for invalid configuration#10904
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Improves robustness of the crossover module by validating runtime-updated configuration blobs before applying them, and strengthening config validation to prevent out-of-range or undersized blobs from reaching setup/init paths.
Changes:
- Re-validate IPC-updated config blobs in
crossover_process_audio_stream()before callingcrossover_setup(). - Extend
crossover_validate_config()to verify blob header size matches framework-reported size. - Add minimum payload-size checks for required LR4 biquad coefficients.
| if (comp_is_new_data_blob_available(cd->model_handler)) { | ||
| cd->config = comp_get_data_blob(cd->model_handler, NULL, NULL); | ||
| cd->config = comp_get_data_blob(cd->model_handler, &cfg_size, NULL); | ||
| if (!cd->config || !cfg_size || | ||
| crossover_validate_config(mod, cd->config, cfg_size) < 0) { | ||
| comp_err(dev, "invalid runtime config blob"); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
This opinion appears for every module. I think we should address it at framework level to let a component validate what comp_is_new_data_blob_available() offers before copying it with comp_get_data_blob(),
singalsu
force-pushed
the
crossover_updates
branch
from
d18eee6 to
7731015
Compare
|
Note: I can't test this in a device at the moment but tested it in sof-testbench4, e.g. With a chirp e.g. left up, right down the effect of filtering is very visible in spectrogram. |
singalsu
force-pushed
the
crossover_updates
branch
from
7731015 to
55695c8
Compare
singalsu
force-pushed
the
crossover_updates
branch
from
55695c8 to
f073d83
Compare
|
The quickbuild fail for MTL (only, other platforms OK) Test HDA passthrough compress data is not related to this change. |
singalsu
force-pushed
the
crossover_updates
branch
from
f073d83 to
4ebae2c
Compare
| return ret; | ||
| } | ||
|
|
||
| if (cd->config) { |
There was a problem hiding this comment.
ok, I should've commented on this earlier, so, you don't need to do a new version just for that, but if you do end up doing a new iteration - I'd join these two if (cd->config) branches and removed int from int ret below to not overshadow the new ret variable that you've added globally in this function. If you don't redo this PR, we can fix it as a follow-up.
There was a problem hiding this comment.
Oops - I've fixed it now. There was also one wrong comp_err() trace print that I fixed.
|
The quickbuild test fail basic hda link random dma can't be related to this. |
crossover_validate_config() was only run on the initial blob in crossover_prepare(). Runtime IPC updates fetched in crossover_process_audio_stream() were applied without revalidation, so a bad blob could drive crossover_setup() with an out-of-range num_sinks. Validate the new blob before crossover_setup() runs. Also extend the validator to cross-check config->size against the size reported by the framework and to require enough payload for the LR4 biquads that crossover_init_coef_ch() reads (1 pair for 2-way, 3 pairs for 3-way and 4-way). Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
singalsu
force-pushed
the
crossover_updates
branch
from
4ebae2c to
7251254
Compare
Agree, cross over not tested by internal CI. |
crossover_validate_config() was only run on the initial blob in crossover_prepare(). Runtime IPC updates fetched in crossover_process_audio_stream() were applied without revalidation, so a bad blob could drive crossover_setup() with an out-of-range num_sinks. Validate the new blob before crossover_setup() runs.
Also extend the validator to cross-check config->size against the size reported by the framework and to require enough payload for the LR4 biquads that crossover_init_coef_ch() reads (1 pair for 2-way, 3 pairs for 3-way and 4-way).