ipc: move all init parsing to components/modules

[cujomalainey]

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. This change moves all type casting down to the final module so it cannot confuse intermediate processes.

Exempted modules/components from shims

• Probe
  • instantiation only supported in IPC4
• ALSA
  • Does not parse spec in IPC3
• Google Hotword Detect
  • Does not parse spec
• Chain DMA
  • IPC4 specific

[cujomalainey]

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]

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]

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.

[lgirdwood]

@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]

@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.

[cujomalainey]

@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

[cujomalainey]

this will need to be refined to check if there was data appended to the IPC, not if the IPC had size total

[singalsu]

@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

Tone generator is an early SOF component that has not been maintained to keep it usable. Should I just make a PR to remove it?

[lyakh]

is the configuration so large or why rballoc()?

[cujomalainey]

Rballoc is what is used module_load_config to alloc the same region. Used it to reduce the number of changes.

[lyakh]

IIUC you use cfg.data for freeing, isn't rfree() missing here - similar to asrc? Please also check others

[cujomalainey]

No this is correct. Data is set to the raw IPC so it should not be freed.