Unsanitized IPC input is used for memory flags

[marc-hb]

./scripts/fuzz.sh -o fuzz-stdout.txt -t 600 fails systematically after L3_HEAP commit 58a42e5

Example: https://github.com/thesofproject/sof/actions/runs/7763189541/job/21174929628

This may take a few minutes but never much longer.

P1 because this is affecting daily tests and our ability to fuzz.

Originally posted by @marc-hb in #8632 (comment)

PR #8632 is probably just the messenger but this fuzzing failure looks like a "good catch" to me.

Since #8632 was merged, one of the new k_panic() gets triggered because caps & SOF_MEM_CAPS_L3 is true even when CONFIG_L3_HEAP is false.

I think the reason caps & SOF_MEM_CAPS_L3 is true is because... caps comes directly from untrusted IPC input!? Why would IPCs be able to set caps directly?

The panic happens when ipc_glb_tplg_buffer_new() does this:

ret = ipc_buffer_new(ipc, (struct sof_ipc_buffer *)ipc->comp_data); 

At this point comp_data looks like it came straight from the fuzzer's untrusted input:

(gdb) p /x *desc

$5 = {comp = {hdr = {size = 0x66, cmd = 0x3020ffff}, id = 0xb6b6b600, type = 0xb6b6b6b6, pipeline_id = 0xb6b6b6b6, core = 0xffffb236, 
    ext_data_length = 0xffffffff}, size = 0xffffffff, caps = 0xffffffff, flags = 0xffffffff, reserved = 0xffffffff}

#0  rballoc_align (flags=0, caps=4294903040, bytes=86376703, align=64) at sof/zephyr/lib/alloc.c:398
#1  0x0831d38c in buffer_alloc (size=86376703, caps=4294903040, flags=4294967295, align=64, is_shared=false)
    at sof/src/audio/buffer.c:58
#2  0x082e4d6b in buffer_new (desc=0x8b184c0 <heapmem+1728>, is_shared=false) at sof/src/ipc/ipc-helper.c:48
#3  0x082cc78e in ipc_buffer_new (ipc=0x8b183c0 <heapmem+1472>, desc=0x8b184c0 <heapmem+1728>) at sof/src/ipc/ipc3/helper.c:459
#4  0x082b7249 in ipc_glb_tplg_buffer_new (header=807469055) at sof/src/ipc/ipc3/handler.c:1305
#5  0x082b090c in ipc_glb_tplg_message (header=807469055) at sof/src/ipc/ipc3/handler.c:1416
#6  0x082aff8a in ipc_cmd (_hdr=0x8b184c0 <heapmem+1728>) at sof/src/ipc/ipc3/handler.c:1651
#7  0x082fcb1b in ipc_platform_do_cmd (ipc=0x8b183c0 <heapmem+1472>) at sof/src/platform/posix/ipc.c:162
#8  0x082e2827 in ipc_do_cmd (data=0x8b183c0 <heapmem+1472>) at sof/src/ipc/ipc-common.c:328
#9  0x0836a27a in task_run (task=0x8b183e8 <heapmem+1512>) at sof/zephyr/include/rtos/task.h:94

[marc-hb]

Debugging this is much easier with zephyrproject-rtos/zephyr#68494 and a bit easier with #8831

[cujomalainey]

What is the reason we are just not reverting the change?

[marc-hb]

The change looks like just the messenger to me. Commit 58a42e5 adds a k_panic() which stops the fuzzer now but I believe the unsanitized IPC input has always been used for memory flags even before that k_panic().

Fuzzing is great but it's not a silver bullet. In this case, fuzzing never seemed to notice the unsanitized flags because they never caused any obvious corruption?

[cujomalainey]

Agreed, I thought the commit added the fallthrough case not just expressing already broken logic, I was under a bad assumption.

[lgirdwood]

@marc-hb can you fix and add validation checks around the memory types passed to IPC. Thanks.

[marc-hb]

I have a quick and dirty hack that is passing all the tests in #8850 but it will break again whenever we add a new MEM_CAPS bit.

What is the reason we are just not reverting the change?

I still don't think this L3 heap should be reverted but @jxstelter I think it should be reworked to better handle invalid inputs. k_panic() is simply too extreme to handle invalid inputs (and fuzzing is just the messenger).

In the meantime I submitted a major rework of fuzz.sh because it was good enough for CI but really too inconvenient and too slow for interactive use. With #8851 it's great for both, please review.

[marc-hb]

One last open PR on this topic and then we can close:

• ipc-helper.c: reject invalid SOF_MEM_CAPS_* bits #8850

[kv2019i]

Stable-v2.9 branched, this didn't make the cut, bumping to 2.10.

[cujomalainey]

@kv2019i given this is a security issue, can we not hotfix?

[kv2019i]

@cujomalainey wrote:

@kv2019i given this is a security issue, can we not hotfix?

I actually thought this was a follow-up and the primary issue was already fixed (and thus the P3 priority).

But if not, let's indeed backport. The main PR is merged yesterday, @marc-hb can you submit a backport to stable-v2.9?

I think we can then close this issue, right? Or anything else pending?

[marc-hb]

I think this was a potential security issue. But it's most likely not as long as unknown flags are ignored or rejected. #8853 is already in stable-v2.9 so I think it's enough.