Skip to content

selector: re-validate configuration on control set #10912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lgirdwood wants to merge 1 commit into
base: main
Choose a base branch
from
+69 −0
Open

selector: re-validate configuration on control set #10912

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions src/audio/selector/selector.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,16 +231,85 @@ static int selector_ctrl_set_data(struct comp_dev *dev,
struct sof_ipc_ctrl_data *cdata)
{
struct comp_data *cd = comp_get_drvdata(dev);
struct comp_buffer *src;
struct sof_sel_config *cfg;
uint32_t src_channels;
int ret = 0;

switch (cdata->cmd) {
case SOF_CTRL_CMD_BINARY:
comp_dbg(dev, "SOF_CTRL_CMD_BINARY");

if (cdata->data->size < sizeof(struct sof_sel_config)) {
comp_err(dev, "invalid config blob size %u", cdata->data->size);
return -EINVAL;
}

cfg = (struct sof_sel_config *)
ASSUME_ALIGNED(&cdata->data->data, 4);

/*
* The config validated at .params() time can be replaced here at
* runtime, so re-validate the new channel counts and selected
* channel before accepting them; otherwise an out-of-range value
* later indexes past the source channels in the copy routine.
*/
switch (cfg->in_channels_count) {
case 0:
case SEL_SOURCE_2CH:

@singalsu singalsu Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@singalsu singalsu Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and also 1CH input is supported

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:

@singalsu singalsu Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also 5.1 and 7.1 sinks

@lgirdwood lgirdwood Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The accepted set here intentionally mirrors the existing selector_verify_params() (.params() path), which only allows SEL_SINK_1CH/2CH/4CH today — so this re-validation doesn't change what the component accepts. Adding 5.1 (6) and 7.1 (8) would be a feature change: it needs the same cases added to selector_verify_params() and the passthrough check, plus confirming the selector processing handles 6/8 ch. Happy to do it, but it feels like a separate enhancement rather than part of this security re-validation fix — would you prefer it here or as a follow-up?

comp_err(dev, "invalid out_channels_count %u",
cfg->out_channels_count);
return -EINVAL;
}
Comment on lines +257 to +278

@lgirdwood lgirdwood Jun 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +257 to +278

@lgirdwood lgirdwood Jun 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/* sel_channel indexes the source channels, so it must be below
* the source channel count, otherwise the copy routine reads
* past the end of each source frame. The copy routine strides by
* the live producer stream channel count, so when the source is
* already connected that live count is the authority for both the
* selected channel and any fixed input count; before connection
* fall back to the configured input count. Always cap by the
* maximum supported source width.
*/
src = comp_dev_get_first_data_producer(dev);
if (src)
src_channels = audio_stream_get_channels(&src->stream);
else
src_channels = cfg->in_channels_count;

/* A fixed (non-zero) input count must not exceed the live source
* width, otherwise sel_channel could be accepted below the
* requested count yet still index past the actual stream.
*/
if (src && cfg->in_channels_count &&
cfg->in_channels_count > src_channels) {
comp_err(dev, "in_channels_count %u exceeds source channels %u",
cfg->in_channels_count, src_channels);
return -EINVAL;
}

if (cfg->sel_channel >= SEL_SOURCE_4CH ||
(src_channels && cfg->sel_channel >= src_channels)) {
comp_err(dev, "invalid sel_channel %u (source channels %u)",
cfg->sel_channel, src_channels);
return -EINVAL;
}

/* Just set the configuration */
cd->config.in_channels_count = cfg->in_channels_count;
cd->config.out_channels_count = cfg->out_channels_count;
Comment on lines 313 to 315

@lgirdwood lgirdwood Jun 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
Loading