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
+71 −0
Open

selector: re-validate configuration on control set #10912

Show file tree
Hide file tree
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
70 changes: 70 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,86 @@ 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_1CH:
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

@lgirdwood lgirdwood Jun 24, 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.

lets add to follow up PR.

@singalsu singalsu Jul 1, 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.

Please add the SEL_SOURCE_1CH case so we don't break the existing supported mono playback case. The 5.1 and 7.1 can be added later. We don't have those in normal topologies yet.

@lgirdwood lgirdwood Jul 1, 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.

Done. Added #define SEL_SOURCE_1CH 1 in selector.h and a case SEL_SOURCE_1CH: in the in_channels_count switch, so mono playback keeps working. 5.1/7.1 left for a later PR as discussed. The sel_channel bound still holds for mono: with a 1-channel source sel_channel is required to be 0.

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?

@lgirdwood lgirdwood Jun 24, 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.

Lets add this as a followup PR.

@singalsu singalsu Jul 1, 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.

The module already supports 5.1 and 7.1. I've tested it several times with test topologies. The higher channels counts are not yet in production topologies.

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

@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 +279

@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 314 to 316

@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
1 change: 1 addition & 0 deletions src/include/sof/audio/selector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ struct comp_dev;

#if CONFIG_IPC_MAJOR_3
/** \brief Supported channel count on input. */
#define SEL_SOURCE_1CH 1
#define SEL_SOURCE_2CH 2
#define SEL_SOURCE_4CH 4

Expand Down
Loading