audio: crossover: Improve robustness for invalid configuration#10904
audio: crossover: Improve robustness for invalid configuration#10904singalsu wants to merge 1 commit into
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(),
| 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) { |
There was a problem hiding this comment.
is this actually possible and necessary? Shouldn't and wouldn't we catch any invalid configuration inside the configuration routine?
There was a problem hiding this comment.
I can try to restructure the code that way and see if it makes sense
| /* Initialize Crossover */ | ||
| if (cd->config && | ||
| (!data_size || crossover_validate_config(mod, cd->config) < 0)) { | ||
| (!data_size || crossover_validate_config(mod, cd->config, data_size) < 0)) { |
There was a problem hiding this comment.
propagating the error from crossover_validate_config() would be cleaner but it anyway always returns -EINVAL in case of failure :-)
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
| * reported new_size must cover at least the fixed header. | ||
| */ | ||
| if (new_size < sizeof(*config)) { | ||
| comp_err(dev, "size u smaller than header %u", (uint32_t)new_size); |
There was a problem hiding this comment.
The topology default limits the blob size to max 1 kB. The 64 bit size is overkill here.
(I recall the xt-xcc build for cAVS does not support %zu, so I've been avoiding it. But it seems we have lost for other reasons too the old toolchain support with Zephyr.)
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
55695c8 to
f073d83
Compare
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).