-
Notifications
You must be signed in to change notification settings - Fork 314
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.
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
Selector get_data fixes #9388
Selector get_data fixes #9388
Conversation
Check to make sure we actually have enough space to copy the config, if we don't we will assert and crash when we could have handled this gracefully. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
We should be using the trusted max size passed in via the framework rather than the scratch memory we are passing into the host. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
ret = memcpy_s(cdata->data->data, ((struct sof_abi_hdr *) | ||
(cdata->data))->size, &cd->config, | ||
ret = memcpy_s(cdata->data->data, | ||
size, &cd->config, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
ret = memcpy_s(cdata->data->data, ((struct sof_abi_hdr *) | ||
(cdata->data))->size, &cd->config, | ||
ret = memcpy_s(cdata->data->data, | ||
size, &cd->config, |
There was a problem hiding this comment.
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.
merging as this is a outstanding fuzzer failure |
No description provided.