ipc3: get_drv: Check comp_type also when driver is matched with UUID #7891

jsarha · 2023-07-04T11:26:03Z

Add a safe guard against malformed SOF_IPC_TPLG_COMP_NEW messages where the UUID matched driver type does not match the component type in the message.

Reported-by: Curtis Malainey curtis@malainey.com

related discussions can be found here: #7830

marc-hb · 2023-07-05T01:01:40Z

The fuzzing failure seems relevant.

cc: @andyross your work in action!

This is funny: I just clicked on the most recent PR looking only for fuzzer startup times in order to close #7629 and I find what looks like an actual catch.

AddressSanitizer:DEADLYSIGNAL
=================================================================
==5247==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x081e7924 bp 0xf20fed98 sp 0xf20fed50 T7)
==5247==The signal is caused by a READ memory access.
==5247==Hint: address points to the zero page.
    #0 0x81e7924 in get_drv /home/runner/work/sof/sof/workspace/sof/src/ipc/ipc3/helper.c:147:11
    #1 0x81e6ea7 in comp_new /home/runner/work/sof/sof/workspace/sof/src/ipc/ipc3/helper.c:322:8
    #2 0x81eb55a in ipc_comp_new /home/runner/work/sof/sof/workspace/sof/src/ipc/ipc3/helper.c:659:7
    #3 0x81df7d6 in ipc_glb_tplg_comp_new /home/runner/work/sof/sof/workspace/sof/src/ipc/ipc3/handler.c:1298:8
    #4 0x81dede6 in ipc_cmd /home/runner/work/sof/sof/workspace/sof/src/ipc/ipc3/handler.c:1680:9
    #5 0x81a0319 in ipc_platform_do_cmd /home/runner/work/sof/sof/workspace/sof/src/platform/posix/ipc.c:162:2
    #6 0x81a433c in ipc_do_cmd /home/runner/work/sof/sof/workspace/sof/src/ipc/ipc-common.c:330:9
    #7 0x81ce96b in task_run /home/runner/work/sof/sof/workspace/sof/zephyr/include/rtos/task.h:94:9
    #8 0x81ce469 in edf_work_handler /home/runner/work/sof/sof/workspace/sof/zephyr/edf_schedule.c:31:16
    #9 0x828b533 in work_queue_main /home/runner/work/sof/sof/workspace/zephyr/kernel/work.c:668:3
    #10 0x8168102 in z_thread_entry /home/runner/work/sof/sof/workspace/zephyr/lib/os/thread_entry.c:36:2
    #11 0x8134e49 in __asan::AsanThread::ThreadStart(unsigned long long) (/home/runner/work/sof/sof/workspace/build-fuzz/zephyr/zephyr.exe+0x8134e49)

jsarha · 2023-07-05T07:46:17Z

The fuzzing failure seems relevant.

cc: @andyross your work in action!

This was excellent. My quick test if the this kind of fix would cause other problems (if the comp type check would be too strict in some cases) had a weakness that would not show in the normal tests, but the fuzzer found it immediately.

jsarha · 2023-07-05T12:10:20Z

I think this is now ready for review even if CI tests are still running.

lyakh · 2023-07-05T15:40:31Z

src/ipc/ipc3/helper.c

what exactly is this condition: drv->type == SOF_COMP_MODULE_ADAPTER? So it's possible to have a component in topology with UUID, matching SOF_COMP_MODULE_ADAPTER but with a different type? Or how is this possible?

I see what you are doing here, but it doesn't validate that the comp type is the right type. E.g. this still leaves the possibility for a SRC UUID with a VOL ENUM.

@lyakh how terrible would it be if we just moved that busted switch case out to each component and just container_of our way back to the correct pointer for vol and src?

For instance MUX components use module adapter. I could not figure out how dig out what is hidden behind the module adapter framework. The nature of IPC3 is still quite strange to me.

bleh, my container_of approach won't work since the data is parsed and copied, not aliased.

Honestly I am more and more inclined to just suggest we go back to #7830 (with an additional guard for vol) and move that check into a IPC3 only block. There is no easy way fix the protocol without making a major revision (deprecating the enum field) and fixing legacy component init payloads. Also if we ifdef it around IPC3 it will be very clear it can be deleted when IPC3 is removed from the repo.

we can try to modify module_adapter.c to resolve this issue.

@cujomalainey @jsarha it seems to me that we just have to go with the "first principles" here: if we don't trust what we get over IPC, then we need to check everything. E.g. .type is checked in comp_specific_builder() but unrecognised type is just silently ignored and that doesn't seem right to me. Because if we do happen to have comp->ext_data_length != 0 then get_drv() might use UUID to get a matching driver and we'll continue with an invalid type.
Then we also get size from those topology IPCs. And in general it's valid for module-adapter components to have a size of 0. In that case no initialisation data is allocated. So individual drivers must check for that, yes.

lyakh · 2023-07-05T15:41:36Z

src/ipc/ipc3/helper.c

goto out with drv == NULL is equivalent to return NULL really, but they do the goto everywhere in this function...

Yes... maybe in the history lock have been kept trough out the function. No point for the label really now. I can drop it, unless we change the approach again...

cujomalainey

patch doesn't provide a way to make sure enum is valid.

Also please use my work email in the tag, thanks

cujomalainey@chromium.org

jsarha · 2023-07-06T20:01:01Z

Now that I look give this another though, this fix is not really good for anything. Now that I am trying to follow the original root cause[1] I am little by little leaning back to my original fix [2] or a variation of it. The actual root cause and the fundamental problem with the init message interpretation is the fact that the sender can send what ever payload to what ever module. Fixing the switch-case in module_adapter_new() is not enough. The comp_common_builder() and comp_specific_builder() will anyway create the 'union ipc_config_specific spec' according to the found ipc type and copy the contents from the actual payload according to it.

So about generic fix, I do not think there is any low hanging fruit. One way could be adding the accepted message types to struct comp_driver and give it as an extra parameter to DECLARE_MODULE_ADAPTER(), and checking the type in get_drv().

The above would fix the issue we are working on, but if that really a generic fix? The fuzzer does not obey any message structures and we really cannot make any assumptions of the passed configuration data in the module. I do not see there is really any way around that the modules just have to be made robust enough to not to crash on bad or missing configuration data.

[1] #7830 (comment)
[2] #7830

What do you say @lyakh & @cujomalainey ?

cujomalainey · 2023-07-06T20:10:36Z

What do you say @lyakh & @cujomalainey ?

Looks like we were typing at the same time :) see #7891 (comment)

gkbldcig · 2023-07-09T14:18:41Z

Can one of the admins verify this patch?

jsarha · 2023-07-10T09:15:22Z

This fix is not very useful since it cannot check message types through module_adapter.

jsarha · 2023-07-13T12:52:37Z

... let's test the concept.

lgirdwood · 2023-07-13T16:02:56Z

@jsarha some build issues ?

lyakh · 2023-07-14T07:14:14Z

src/include/sof/audio/module_adapter/module/generic.h

this is a rather significant change, and I think it will break at least this check:

sof/src/audio/module_adapter/module_adapter.c

Line 135 in 7027228

if (drv->type == SOF_COMP_MODULE_ADAPTER) {

and possibly more. Whereas my proposal to change comp_specific_builder() would only affect IPC3. @ranj063 do you think this is a good idea?

@lyakh , but how would I know in comp_specific_builder() what kind of messages the module - found with uuid - actually accepts, unless its somehow built in to the module? I can of course add an extra member to struct comp_driver, but I do not think this issue can not be solved without adding the accepted component message type to each module.

No, don't think it's possible to check the type generically in common code for all components with the current module-adapter API. That's why I'm not proposing that. Instead I'm just proposing to (1) check that the type value in IPC is meaningful - one of the values, that the firmware recognises, and error out otherwise, instead of silently ignoring it, and (2) in each individual component driver check that the type from the IPC is correct

Ok, simple enough.

Add a safe guard against malformed SOF_IPC_TPLG_COMP_NEW messages where the UUID matched driver type does not match the component type in the message. Also moves the !drv check outside of &drivers->lock block. Reported-by: Curtis Malainey <cujomalainey@chromium.org> Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>

jsarha · 2023-07-14T09:47:41Z

This approach would be doable, but it would touch every module. Maybe not something worth doing for phasing out IPC3. Closing this PR. There is much simpler PR for not accepting unknown component types here: #7948

jsarha force-pushed the ipc3_get_drv_type_check branch from 87da089 to 863eae7 Compare July 5, 2023 07:39

jsarha force-pushed the ipc3_get_drv_type_check branch 2 times, most recently from bab7a52 to b4563a5 Compare July 5, 2023 10:32

jsarha marked this pull request as ready for review July 5, 2023 12:09

jsarha requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners July 5, 2023 12:09

jsarha requested a review from cujomalainey July 5, 2023 12:09

jsarha requested a review from lyakh July 5, 2023 12:14

jsarha mentioned this pull request Jul 5, 2023

Audio: SRC: Add safeguard against completely missing init_data #7830

Merged

lgirdwood approved these changes Jul 5, 2023

View reviewed changes

lyakh reviewed Jul 5, 2023

View reviewed changes

cujomalainey suggested changes Jul 5, 2023

View reviewed changes

jsarha closed this Jul 10, 2023

jsarha reopened this Jul 13, 2023

jsarha marked this pull request as draft July 13, 2023 12:52

jsarha force-pushed the ipc3_get_drv_type_check branch from b4563a5 to a83bd1b Compare July 13, 2023 12:53

jsarha force-pushed the ipc3_get_drv_type_check branch from a83bd1b to 2cc2302 Compare July 14, 2023 07:04

lyakh reviewed Jul 14, 2023

View reviewed changes

jsarha force-pushed the ipc3_get_drv_type_check branch from 2cc2302 to 7146f83 Compare July 14, 2023 08:51

jsarha closed this Jul 14, 2023

ipc3: get_drv: Check comp_type also when driver is matched with UUID #7891

ipc3: get_drv: Check comp_type also when driver is matched with UUID #7891

Uh oh!

Conversation

jsarha commented Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Jul 5, 2023

Uh oh!

jsarha commented Jul 5, 2023

Uh oh!

jsarha commented Jul 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cujomalainey Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

jsarha commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cujomalainey commented Jul 6, 2023

Uh oh!

gkbldcig commented Jul 9, 2023

Uh oh!

jsarha commented Jul 10, 2023

Uh oh!

jsarha commented Jul 13, 2023

Uh oh!

lgirdwood commented Jul 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsarha Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsarha commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jsarha commented Jul 4, 2023 •

edited

Loading

cujomalainey Jul 6, 2023 •

edited

Loading

jsarha commented Jul 6, 2023 •

edited

Loading

jsarha Jul 14, 2023 •

edited

Loading

jsarha commented Jul 14, 2023 •

edited

Loading