ipc3: Add size checks on ipc subtypes#9297
Conversation
|
Pushed as draft as validation is still ongoing but I wanted early input |
|
whoops, was building IPC4 when testing, not IPC3, will fix tomorrow |
src/include/sof/ipc/common.h
Outdated
| (object).hdr.size == sizeof(object) ? 0 : 1 | ||
|
|
||
| #define IPC_TAIL_IS_SIZE_INVALID(object) \ | ||
| (object).hdr.size + (object).ext_data_length == sizeof(object) ? 0 : 1 |
There was a problem hiding this comment.
at the very least maybe IPC_TAIL_SIZE_IS_INVALID() and ideally I'd make this
#define IPC_TAIL_SIZE_IS_VALID(object) \
((object).hdr.size + (object).ext_data_length == sizeof(object))
because in general affirmative names might be easier to work with, and the == comparison already returns a perfect boolean result, but the definition above already does the other way, so...
There was a problem hiding this comment.
I would rather IPC_TAIL_IS_SIZE_INVALID to keep the naming semantics the same as above or update the one above to match your suggestion. But i rather not deviate on this alone.
Seconded on the brackets, i thought that was weird, not sure why they were ommitted
I am not sure either, it is the same as using != I am really not sure what is going on with that macro, i can update both to keep them the same
Looks like @lgirdwood was the original author of the macro. Maybe he can share some of his logic
b58b4ed to
5dca67b
Compare
|
@lyakh I flipped the comparison to |
We have a few gaps in input validation from the kernel. Right now we check the IPC doesn't claim its larger the window, this patch adds the following checks: 1. That the IPC size is at least large enough for any downcast type on comp new 2. That any reported size for a process total size is less than the IPC window. Also adjust the other helper to be a bit safer and more direct Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
|
On second thought this is possibly more of a mess than i originally thought. This check here given its a So what do we want to fix? Do we want to have the firmware not able to trust the top level size? Or do we want to fix the kernel and firmware to properly emit sizes in the top level header (likely bringing in breakages between versions). |
|
@lgirdwood @plbossart thoughts on the proper fix here? I would argue the ABI spec is either wrong or violated here |
|
Moving to bug for discussion |
|
Change of plans, i don't think an abi change will help, going to continue hammering this. |
lgirdwood
left a comment
There was a problem hiding this comment.
I think this looks correct for IPC3
|
SOFCI TEST |
|
Reruns CI as been pending for a while. |
We have a few gaps in input validation from the kernel. Right now we check the IPC doesn't claim its larger the window, this patch adds the following checks: