-
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
module_adapter: fix switch case for spec parsing #9384
Conversation
Sparse errors should be fixed by #9385 |
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.
Comments inline
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
f98b1fb
to
8214c19
Compare
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.
Given the list is enumerated in comp_specific_builder(), this seems the best approach in the end.
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.