rtnr: clamp host-supplied control element counts#10911
Conversation
lgirdwood
requested review from
dbaluta,
kv2019i,
lbetlej,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds bounds-checking for host-supplied SWITCH control element counts in rtnr get/set handlers to prevent reading/writing past the IPC payload.
Changes:
- Validate
cdata->num_elemsagainstSOF_IPC_MAX_CHANNELSinrtnr_get_config(). - Validate
cdata->num_elemsagainstSOF_IPC_MAX_CHANNELSinrtnr_set_value().
| if (cdata->num_elems > SOF_IPC_MAX_CHANNELS) { | ||
| comp_err(dev, "rtnr_get_config() error: num_elems %u out of range", | ||
| cdata->num_elems); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
Reworded — the behaviour is intentional rejection (a control message with more elements than channels is malformed). I consolidated the get/set changes into a single commit titled "rtnr: reject out-of-range control switch element counts" and updated the PR description to match.
| if (cdata->num_elems > SOF_IPC_MAX_CHANNELS) { | ||
| comp_err(dev, "rtnr_set_value() error: num_elems %u out of range", | ||
| cdata->num_elems); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
Fixed — both messages now include the limit: num_elems %u > max %u (with SOF_IPC_MAX_CHANNELS).
abonislawski
left a comment
abonislawski
left a comment
There was a problem hiding this comment.
@lgirdwood looks like there are 2 accidentally added txt files?
|
|
||
| case SOF_CTRL_CMD_SWITCH: | ||
| if (cdata->num_elems > SOF_IPC_MAX_CHANNELS) { | ||
| comp_err(dev, "rtnr_get_config() error: num_elems %u > max %u", |
There was a problem hiding this comment.
"rtnr_get_config()" func name duplicate not needed
There was a problem hiding this comment.
Done — dropped the rtnr_get_config() prefix; the message is now just "num_elems %u > max %u" (comp_err already tags the component/function context).
| int32_t j; | ||
|
|
||
| if (cdata->num_elems > SOF_IPC_MAX_CHANNELS) { | ||
| comp_err(dev, "rtnr_set_value() error: num_elems %u > max %u", |
There was a problem hiding this comment.
"rtnr_set_value()" func name duplicate not needed
There was a problem hiding this comment.
Done — same here, dropped the rtnr_set_value() prefix.
singalsu
left a comment
singalsu
left a comment
There was a problem hiding this comment.
The alsa-info.txt and dump.txt look like accidentally included to this PR.
| @@ -0,0 +1,5744 @@ | |||
| upload=true&script=true&cardinfo= | |||
There was a problem hiding this comment.
are these two files needed here? Don't seem related to rtnr. Supposedly a bad commit.
There was a problem hiding this comment.
You're right — those two files (alsa-info.txt..., dell-arl-dump.txt) were unrelated local files accidentally swept in by a git add -A. Removed them; the branch now contains only the rtnr.c change. Sorry about that.
They do indeed, no idea why Claude would add debug to the PR. This sound like an addition to agent workflow, i.e check commit for log files and remove before commit. |
The SWITCH control get and set handlers looped over a host-supplied element count while reading/writing the control channel array, which could run past the IPC payload. Reject counts larger than the maximum channel count in both handlers before the loop. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The SWITCH control get and set handlers looped over a host-supplied element
count while reading/writing the control channel array, which could run past
the IPC payload. Reject counts larger than
SOF_IPC_MAX_CHANNELSin bothhandlers before the loop.
No functional change for valid configurations.