Skip to content
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

Jump to bottom

Selector get_data fixes #9388

Merged
merged 2 commits into from
Aug 26, 2024
Merged

Selector get_data fixes #9388

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f6f2dd
selector: add a minimum size check for get_data
cujomalainey Aug 22, 2024
4313e8a
selector: fix memcpy parameters
cujomalainey Aug 22, 2024
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
6 changes: 4 additions & 2 deletions src/audio/selector/selector.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,12 @@ static int selector_ctrl_get_data(struct comp_dev *dev,
switch (cdata->cmd) {
case SOF_CTRL_CMD_BINARY:
comp_dbg(dev, "selector_ctrl_get_data(), SOF_CTRL_CMD_BINARY");
if (size < sizeof(cd->config))
return -EINVAL;

/* Copy back to user space */
ret = memcpy_s(cdata->data->data, ((struct sof_abi_hdr *)
(cdata->data))->size, &cd->config,
ret = memcpy_s(cdata->data->data,
size, &cd->config,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should ((struct sof_abi_hdr *)(cdata->data))->size be checked for validity?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so given we overwrite it below as the reply.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but it comes from the user, right? So I thought that maybe we shouldn't be talking to misbehaving users, or at least should warn the system?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In theory yes, but I wonder if it's worth the code on DSP side given there are checks here for "size" and aother check in "ipc_comp_value()". This would be really only useful for a legitimate host that is somehow misconfigured, but that will show up already in logs as the header size will be changed by DSP in the response.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also if we were wanting to change that, it should be a layer above, we should not be doing that check at every module

sizeof(cd->config));
assert(!ret);

Expand Down
Loading