selector: re-validate configuration on control set#10912
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Re-validates selector configuration received via the control-set (binary) path to prevent accepting invalid channel counts/selection at runtime (which could later overflow internal tables).
Changes:
- Add minimum blob-size validation before parsing
struct sof_sel_config. - Re-validate
in_channels_count,out_channels_count, andsel_channelinselector_ctrl_set_data()before applying the config.
| if (cfg->sel_channel >= SEL_SOURCE_CHANNELS_MAX) { | ||
| comp_err(dev, "invalid sel_channel %u", cfg->sel_channel); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
Good point — added. sel_channel is now rejected when it is >= the configured in_channels_count (when that is non-zero/fixed), in addition to the absolute SEL_SOURCE_CHANNELS_MAX bound. When in_channels_count is 0 (derive from the stream at runtime) the absolute bound applies and .params() still checks against the actual stream channel count.
| switch (cfg->in_channels_count) { | ||
| case 0: | ||
| case SEL_SOURCE_2CH: | ||
| case SEL_SOURCE_4CH: | ||
| break; | ||
| default: | ||
| comp_err(dev, "invalid in_channels_count %u", | ||
| cfg->in_channels_count); | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| switch (cfg->out_channels_count) { | ||
| case 0: | ||
| case SEL_SINK_1CH: | ||
| case SEL_SINK_2CH: | ||
| case SEL_SINK_4CH: | ||
| break; | ||
| default: | ||
| comp_err(dev, "invalid out_channels_count %u", | ||
| cfg->out_channels_count); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
0 isn't a "no change" sentinel here — it's a valid configuration meaning "derive the channel count from the stream at runtime". See selector_verify_params(): out_channels = cd->config.out_channels_count ? cd->config.out_channels_count : audio_stream_get_channels(...) (same for in). So accepting and storing 0 is correct; the effective count is resolved (and re-validated) at .params() time.
| /* Just set the configuration */ | ||
| cd->config.in_channels_count = cfg->in_channels_count; | ||
| cd->config.out_channels_count = cfg->out_channels_count; |
There was a problem hiding this comment.
0 isn't a "no change" sentinel here — it's a valid configuration meaning "derive the channel count from the stream at runtime". See selector_verify_params(): out_channels = cd->config.out_channels_count ? cd->config.out_channels_count : audio_stream_get_channels(...) (same for in). So accepting and storing 0 is correct; the effective count is resolved (and re-validated) at .params() time.
| switch (cfg->in_channels_count) { | ||
| case 0: | ||
| case SEL_SOURCE_2CH: | ||
| case SEL_SOURCE_4CH: | ||
| break; | ||
| default: | ||
| comp_err(dev, "invalid in_channels_count %u", | ||
| cfg->in_channels_count); | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| switch (cfg->out_channels_count) { | ||
| case 0: | ||
| case SEL_SINK_1CH: | ||
| case SEL_SINK_2CH: | ||
| case SEL_SINK_4CH: | ||
| break; | ||
| default: | ||
| comp_err(dev, "invalid out_channels_count %u", | ||
| cfg->out_channels_count); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
The two paths validate different things, which is why I didn't share code: selector_verify_params() validates the effective in/out channel counts at .params() time — deriving them from the stream when the config value is 0 and checking sel_channel against the actual runtime channel count. This handler validates the raw config fields before they're stored (accepted values, and sel_channel against the fixed input count). A shared helper would only cover the accepted-value switches and would mean restructuring verify_params to validate raw values before deriving. Happy to extract one if you'd prefer that, but kept them separate to avoid changing the prepare-time logic.
A new configuration accepted at runtime through the control set path was copied in without the validation done when parameters are applied, so a later out-of-range channel count could overflow the channel table. Validate the blob size and channel fields before accepting it. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
| */ | ||
| switch (cfg->in_channels_count) { | ||
| case 0: | ||
| case SEL_SOURCE_2CH: |
There was a problem hiding this comment.
We have also 5.1 and 7.1 down-mix supported. See https://github.com/thesofproject/sof/tree/main/tools/topology/topology2/include/components/micsel
| case SEL_SINK_2CH: | ||
| case SEL_SINK_4CH: | ||
| break; | ||
| default: |
The selector validates in/out channel counts at .params() time but accepts a
new configuration via the control set path afterwards without re-validating,
so a later out-of-range channel count could overflow the channel table.
Re-validate the blob size and channel fields in the control set handler
before accepting the new config.
No functional change for valid configurations.