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.

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

ipc3: check type field if using UUID match #9535

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cujomalainey
Copy link
Member

A bad IPC can mismatch UUID and type, causing downstream processes to operate differently than the matched component target. This is because get_drv will not reject the mismatch. This enforces the topology matches the UUID and type.

@cujomalainey
Copy link
Member Author

Pushed this again for exploring solutions.

@lyakh I don't think we will be able to do type checks. There are so many types that are hardcoded in ipc3 for legacy reasons that now no longer match because of the module adapter that I don't see how it is feasible to force the check.

And in all honesty, I am now sure this is the correct fix looking closer.

E.g. ASRC which is currently a MODULE_ADAPTER enum in its comp_driver but requires the SOF_COMP_ASRC enum to properly copy its specification through the IPC system.

So my current theories on how to fix this are pretty aggressive as they either require exposing a new callback / driver info to properly parse the IPC info or just uprev the major version and drop the enum field and comp_specific enum data entirely or pushing the init data parsing down to the component where it belongs.

Regardless of the solution this is unfortunately a security bug that does need to be resolved sooner rather than later.

@cujomalainey cujomalainey added bug Something isn't working as expected ABI ABI change involved P1 Blocker bugs or important features labels Oct 2, 2024
@cujomalainey
Copy link
Member Author

I'm exploring the possibilities of just packaging the data into a struct with just a size header and then forcing all downstream components to do the type checking. This would remove the dependency on the enum while also forcing the component specific code back down to the components.

@cujomalainey
Copy link
Member Author

I'm exploring the possibilities of just packaging the data into a struct with just a size header and then forcing all downstream components to do the type checking. This would remove the dependency on the enum while also forcing the component specific code back down to the components.

So we have really cornered ourselves here with IPC3 adapting to IPC4 types in a generic format. Couple of issues I see here.

  • We don't pass down spec size on IPC3 right now (really concerning) but i think we can just enable ipc_config_size for IPC3 as well.
  • I don't think we will ever win with doing the conversion in the IPC layer, we got away with it until the module adapter was introduced, but now that it is obscuring the ability to validate the comp type enum we can't truly know what we are working with. Therefore I think we need to stop using the enum field entirely except in the case of comp identification without a uuid.
  • Given the above point this means we need to force down the parsing of the data. I think this actually will be fairly simple for non module adapter processes as we just compile time apply a shim to the init function to convert the spec data in the component.
  • The module adapter is similar but instead of the shim converting on the stack it will just convert and replace the heap allocated memory.

With these changes we should be protected against enum UUID mismatch as we only check 1 of those 2 items once to get the comp_driver then let the driver handle everything else from there. This also requires no ABI or kernel changes.

The one downside i see is that modules will have IPC specific shims but i think that is a small price to pay to thin this hole out of our communication layer.

Add helper to switch out init calls to shim in conversion functions if
needed.

Signed-off-by: Curtis Malainey <[email protected]>
@lgirdwood
Copy link
Member

@cujomalainey ack - lets get rid of the enum, we should be able to do basic checking in the IPC layer and then force down module/driver specif data to the module/driver for checking. We can extend the APIs as needed, even if we have some __unused params on some IPC flavours

@cujomalainey
Copy link
Member Author

@cujomalainey ack - lets get rid of the enum, we should be able to do basic checking in the IPC layer and then force down module/driver specif data to the module/driver for checking. We can extend the APIs as needed, even if we have some __unused params on some IPC flavours

I think the enum is fine as long as we use the identified drv only for all conversions and never use the IPC after that.

I think with my fix I don't think we need to modify the APIs at all, just a compile time shim. I hope to push an example today for comments.

Right now we are doing a lot of "guess work" based on type enums and
context for what size `spec` is. Move towards checking the actual size
and trusting that.

Signed-off-by: Curtis Malainey <[email protected]>
NOT COMPILE TESTED

Given we cannot trust our enum field in the IPC as communicator may lie
about the component on the other side and the module adapter is
obscuring the nature of many of the components we need to move the type
casting into the components directly. Here is a breakdown of the general
steps to achieve this.

1. report ipc config size for all versions
2. remove type casting in the IPC3 layer
3. on non module adapter components cast the config in a compile time
   selected shim
4. in the module adapter ipc3 layer, remove type casting again
5. in module adapter components realloc init data and cast over in a
   compile time enabled shim

Signed-off-by: Curtis Malainey <[email protected]>
@cujomalainey
Copy link
Member Author

cujomalainey commented Oct 3, 2024

@lgirdwood this is a non functional example of what I think needs to happen. PTAL.

Tone is the non module adapter example, ASRC is the module adapter example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI ABI change involved bug Something isn't working as expected P1 Blocker bugs or important features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants