audio: tdfb: register IPC-time blob validator#10944
Conversation
| if (!cd->config || cd->config_size < sizeof(*cd->config) || | ||
| cd->config_size > SOF_TDFB_MAX_SIZE) { | ||
| comp_err(dev, "invalid configuration blob, size %zu", cd->config_size); | ||
| if (!cd->config || tdfb_check_blob_size(dev, cd->config_size) < 0) |
There was a problem hiding this comment.
I thought this should remove processing-time blob checks?
There was a problem hiding this comment.
Yes, this check is a duplicate.
|
|
||
| static int tdfb_init_coef(struct processing_module *mod, int source_nch, | ||
| int sink_nch) | ||
| static int tdfb_check_blob_size(struct comp_dev *dev, size_t size) |
There was a problem hiding this comment.
like in other PRs static bool ...
There was a problem hiding this comment.
The function content can be moved to tdfb_validator().
| * validate_config() call runs. | ||
| */ | ||
| cd->source_channels = source_channels; | ||
| cd->sink_channels = sink_channels; |
There was a problem hiding this comment.
usually it's better to only modify persistent data when you're sure the change is valid... Passing these new values as parameters to validation functions would be safer
There was a problem hiding this comment.
Yes, but the validator callback (call from model handler) needs this internal state. Also, the information set here is correct even if the blob wouldn't be valid. I started to change this but it turned out difficult.
singalsu
force-pushed
the
model_handler_validator_tdfb
branch
from
746ca74 to
62ae3c2
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an IPC-time configuration blob validator to the TDFB (time-domain fixed beamformer) component so malformed or stream-mismatching runtime updates are rejected before they can replace the currently working configuration, preventing out-of-bounds walks during coefficient setup.
Changes:
- Added
tdfb_validator()/tdfb_validate_config()to byte-bound walk the FIR sections and validate trailer layout againstconfig->size. - Enforced stream/channel compatibility checks during validation using cached source/sink channel counts.
- Removed now-redundant structural/sanity checks from
tdfb_init_coef()and installed/uninstalled the validator inprepare()/reset().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/audio/tdfb/tdfb.c | Adds a new blob validator, integrates it into prepare/reset and runtime update handling, and removes redundant in-setup validation. |
| src/audio/tdfb/tdfb_comp.h | Caches source/sink channel counts in component state for use by the validator. |
| /* The blob must match the running stream. Skip these checks when no | ||
| * stream is bound yet (cached channel counts are zero before prepare). | ||
| */ |
There was a problem hiding this comment.
OK, there is a runtime check for out of bounds read but a fail with such corrupt blob would fail the capture use case.
| if (fir_delay_size(coef_data) <= 0) { | ||
| comp_err(dev, "FIR %d invalid length %u", | ||
| i, coef_data->length); | ||
| return -EINVAL; | ||
| } |
Hook a tdfb blob validator into the model handler so a corrupted or mismatching run-time configuration update is rejected before it can replace the working blob. Capture then continues with the previously set filters instead of being interrupted by bad re-configuration. The TDFB blob is variable size and the per-filter walk in tdfb_init_coef() was not bounded against the IPC payload, so a bad length field could push tdfb_filter_seek() past the buffer. The validator now walks every FIR section and the trailing arrays with byte-bounded steps, requires the total layout to exactly match config->size, and rejects blobs whose num_output_channels or input_channel_select[] entries do not fit the channel counts of the running stream. Each filter_angles[].filter_index is also range- checked against the total filter count so a bad index in any angle entry is caught at ingress, not only when that angle happens to be selected at runtime. The same walk is reused at prepare time on the initial topology blob; a malformed initial blob is discarded (cd->config cleared) so the runtime path will not dereference it. With ingress fully validated, the redundant sanity checks inside tdfb_init_coef() are dropped. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
singalsu
force-pushed
the
model_handler_validator_tdfb
branch
from
62ae3c2 to
a224f33
Compare
Hook a tdfb blob validator into the model handler so a corrupted or mismatching run-time configuration update is rejected before it can replace the working blob. Capture then continues with the previously set filters instead of being interrupted by bad re-configuration.
The TDFB blob is variable size and the per-filter walk in tdfb_init_coef() was not bounded against the IPC payload, so a bad length field could push tdfb_filter_seek() past the buffer. The validator now walks every FIR section and the trailing arrays with byte-bounded steps, requires the total layout to exactly match config->size, and rejects blobs whose num_output_channels or input_channel_select[] entries do not fit the channel counts of the running stream. The same walk is reused at prepare time on the initial topology blob. With ingress fully validated, the redundant sanity checks inside tdfb_init_coef() are dropped.