Repository navigation
[BUG] bad coding patterns leading to security bugs/crashes #9563
Description
Activity
This warrants a wider discussion. Maybe start from these concrete examples and see if there is something systematic and then file for fine-grained items to address. Do you @cujomalainey a stack trace from fuzz to some example case for e.g. comp_dev_get_first_data()? There is a clear assumption at least one buffer is always connected, but if this check can be avoided, this is indeed a problem.
As comp_dev_get_first_data_() is quite recent addition, I'll loop in @marcinszkudlinski to comment on this. Did you consider how to handle NULL returns (it does look you just kept the semantics of the old list_first_item().
I don't have any examples of
comp_get_data_blobyet but its not hard to see the relationship between the binary coming from the IPC (I.e. user space). The issue is not that there is a buffer, its that the buffer is the size we expect. If user space passed in a single byte and we were expecting 20, that is an issue if we don't check the size.Regarding the buffer checks, here is an example where the fuzzer called trigger on an disconnected
selectorAddressSanitizer:DEADLYSIGNAL
==20834==ERROR: AddressSanitizer: SEGV on unknown address 0x00000088 (pc 0x082409de bp 0xec2dfd78 sp 0xec2dfd60 T7)
==20834==The signal is caused by a READ memory access.
==20834==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
#0 0x82409de in selector_trigger sof_workspace/sof/src/audio/selector/selector.c:354:32
#1 0x81fa636 in comp_trigger_local sof_workspace/sof/src/include/sof/audio/component_ext.h:154:8
#2 0x81fa4d8 in comp_trigger sof_workspace/sof/src/include/sof/audio/component_ext.h:184:9
#3 0x81f955f in pipeline_comp_trigger sof_workspace/sof/src/audio/pipeline/pipeline-stream.c:507:8
#4 0x81f7a64 in pipeline_trigger_run sof_workspace/sof/src/audio/pipeline/pipeline-stream.c:569:8
#5 0x81c9fa2 in ipc_stream_trigger sof_workspace/sof/src/ipc/ipc3/handler.c:522:9
#6 0x81c48d6 in ipc_cmd sof_workspace/sof/src/ipc/ipc3/handler.c:1664:9
#7 0x81dd8d9 in ipc_platform_do_cmd sof_workspace/sof/src/platform/posix/ipc.c:160:2
#8 0x81d6e0c in ipc_do_cmd sof_workspace/sof/src/ipc/ipc-common.c:350:9
#9 0x81fe70b in task_run sof_workspace/sof/zephyr/include/rtos/task.h:94:9
#10 0x81fe298 in edf_work_handler sof_workspace/sof/zephyr/edf_schedule.c:32:16
#11 0x8314571 in work_queue_main sof_workspace/zephyr/kernel/work.c:688:3
#12 0x81ac4b2 in z_thread_entry sof_workspace/zephyr/lib/os/thread_entry.c:48:2rc2 tagged, so running out of time with this one. Given this is P2 and no pleas to bump this to P1, I'll push this to v2.13 as we wrap up 2.12 release.
Reacted by Liam GirdwoodFYI @abonislawski @lgirdwood , what's your take addressing this for v2.13?
Reacted by Liam Girdwoodthe fuzzer called trigger on an disconnected
selector@cujomalainey do I understand it correctly, that this bug was also a result of IPC fuzzing? I.e. just by sending corrupted IPCs it's possible to bring SOF into a state, where this will happen?
the fuzzer called trigger on an disconnected
selector@cujomalainey do I understand it correctly, that this bug was also a result of IPC fuzzing? I.e. just by sending corrupted IPCs it's possible to bring SOF into a state, where this will happen?
Correct which means there is a security hole in the implementation
comp_dbg(dev, "selector_trigger()"); sourceb = comp_dev_get_first_data_producer(dev);
sourceb not check and can be NULL. Fixing.
Describe the bug
Lack of size checks on blobs or topology state.
E.g.
comp_dev_get_first_data_*missing nullity checkscomp_get_data_blobnot checking the size paramsTo Reproduce
fuzz
Reproduction Rate
high
Expected behavior
robust code
Impact
security
Possible long term fix
Algebraic typing (RUST!)
Short term fixes
Some sort of checks on special functions we know are misused using CI tooling