module_adapter: fix switch case for spec parsing #9384
Conversation
|
Sparse errors should be fixed by #9385 |
| case SOF_COMP_DCBLOCK: | ||
| case SOF_COMP_SMART_AMP: | ||
| case SOF_COMP_MODULE_ADAPTER: | ||
| case SOF_COMP_NONE: |
There was a problem hiding this comment.
@singalsu Can you check this?
The idea was really to have common handling for process types and not need to enumerate process types in so many places. OTOH, this was more of a concern to Linux kernel side (needed to change the Linux kernel for every new SOF module type would not scale). We certainly know the superset of modules that can be used, but this leaves the operational risk that a new component type needs to be added here as well.
If malicious host uses an invalid type, it can just as well use one of the valid component types, and use invalid fields for rest of values, right?
There was a problem hiding this comment.
The problem here was that components that were not expected to be in the module adapter (DAI) were being cast to process IPCs and resulting in out of bounds accesses. My opinion we have 2 problems here.
- we need to remove the legacy init types to simplify the logic.
- filter special types so we create a block list rather than an allow list.
There was a problem hiding this comment.
how about adding an enumeration value after all the valid DSP-bound (not testing ones) components and using
case SOF_COMP_NONE ... SOF_COMP_MAX:
?
There was a problem hiding this comment.
how about adding an enumeration value after all the valid DSP-bound (not testing ones) components and using
case SOF_COMP_NONE ... SOF_COMP_MAX:?
Doesn't work as *HOST and *DAI also caught in that range and that is intentionally not being caught here.
There was a problem hiding this comment.
For reference, this is a copy from here
Which raises the question, are the missing COMPs also supposed to be here? If yes this further backs my suggestions in my previous comment to dissolve these hard coded types in the component layer.
There was a problem hiding this comment.
Doesn't work as *HOST and *DAI also caught in that range and that is intentionally not being caught here.
@cujomalainey ah, so they would go to default? But they aren't really "unsupported," they're just invalid for this operation? But ok, maybe would be good then to explicitly add them to the default case
as
case SOF_COMP_HOST:
case SOF_COMP_DAI:
default:
...
or at least mention that in a comment, but that's cosmetics, not really important.
There was a problem hiding this comment.
I would recommend not adding the above to the default case. As much as it makes it exhaustive (which in something like rust would actually be important as it affects compilation) here it is cosmetic as you said so I would like the intention of added cases to be only for types that can be instantiated and not mix intents of support. Personal preference though.
This switch case is non exhaustive and has not guards on config types which means assuming all types that are default are process types is false. The fuzzer found a way to utilize a DAI to bypass IPC layer checks but break the module adapter. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
cujomalainey
force-pushed
the
mod_check
branch
from
f98b1fb
to
8214c19
Compare
|
i would like to merge this by EOW, there are a number of fuzzer bugs fixed by this patch |
This switch case is non exhaustive and has not guards on config types which means assuming all types that are default are process types is false. The fuzzer found a way to utilize a DAI to bypass IPC layer checks but break the module adapter.