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 :-)
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 <[email protected]>
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. |
| struct comp_dev *dev = mod->dev; | ||
| uint32_t size = config->size; | ||
| size_t required_size; | ||
| int32_t num_assigned_sinks; | ||
| int32_t num_lr4s; |
| 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; | ||
| } |
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).