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

pipeline: graph: Check if source_comp is NULL in pipeline_comp_reset() #9586

Merged

Conversation

jsarha
Copy link
Contributor

@jsarha jsarha commented Oct 16, 2024

pipeline: graph: Check if source_comp is NULL in pipeline_comp_reset()

The fuzzer engine has produced crash caused by NULL pointer read
that originated from ipc3 ipc_stream_pcm_free(). The crash happens
when the pipeline of the found comp_dev does not have a
source_comp and pipeline_reset() is called for it. This commit
adds check to pipeline_comp_test() for this situation and bails
out if it is found.

Here is the call stack from the situation:

#0 0x81e9317 in dev_comp_pipe_id sof/sof/src/include/sof/audio/component.h:646:25
#1 0x81e8015 in pipeline_comp_reset sof/sof/src/audio/pipeline/pipeline-graph.c:326:22
#2 0x81e7d1d in pipeline_reset sof/sof/src/audio/pipeline/pipeline-graph.c:393:8
#3 0x820d7ea in ipc_stream_pcm_free sof/sof/src/ipc/ipc3/handler.c:398:8
#4 0x8208969 in ipc_cmd sof/sof/src/ipc/ipc3/handler.c:1689:9
#5 0x81cbed8 in ipc_platform_do_cmd sof/sof/src/platform/posix/ipc.c:162:2
#6 0x81d10db in ipc_do_cmd sof/sof/src/ipc/ipc-common.c:330:9
#7 0x81f87e9 in task_run sof/sof/zephyr/include/rtos/task.h:94:9
#8 0x81f8308 in edf_work_handler sof/sof/zephyr/edf_schedule.c:31:16
#9 0x82b4b32 in work_queue_main sof/zephyr/kernel/work.c:668:3
#10 0x8193ec2 in z_thread_entry sof/zephyr/lib/os/thread_entry.c:36:2
#11 0x815f639 in __asan::AsanThread::ThreadStart(unsigned long long) /src/llvm-project/compiler-rt/lib/asan/asan_thread.cpp:277:25

The original oss-fuzz report, for those who have access, can be found here: https://issues.oss-fuzz.com/u/1/issues/42537298

@jsarha jsarha marked this pull request as ready for review October 17, 2024 06:42
src/ipc/ipc3/handler.c Outdated Show resolved Hide resolved
@jsarha jsarha force-pushed the fuzzer_fix_add_source_comp_test branch from de50527 to 71afb2f Compare October 17, 2024 09:30
lgirdwood
lgirdwood previously approved these changes Oct 17, 2024
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsarha do we need same for IPC4 ?

dbaluta
dbaluta previously approved these changes Oct 17, 2024
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal in line to manage in comp_reset...

src/ipc/ipc3/handler.c Outdated Show resolved Hide resolved
@jsarha
Copy link
Contributor Author

jsarha commented Oct 17, 2024

@jsarha do we need same for IPC4 ?

@lgirdwood , the crash report is from IPC3. I have no idea if the same issue would be possible on IPC4, but if we want to cover IPC4 just to be safe, then that is easy. I'll make the change.

The fuzzer engine has produced crash caused by NULL pointer read
that originated from ipc3 ipc_stream_pcm_free(). The crash happens
when the pipeline of the found comp_dev does not have a
source_comp and pipeline_reset() is called for it. This commit
adds check to pipeline_comp_test() for this situation and bails
out if it is found.

Here is the call stack from the situation:

    #0 0x81e9317 in dev_comp_pipe_id sof/sof/src/include/sof/audio/component.h:646:25
    thesofproject#1 0x81e8015 in pipeline_comp_reset sof/sof/src/audio/pipeline/pipeline-graph.c:326:22
    thesofproject#2 0x81e7d1d in pipeline_reset sof/sof/src/audio/pipeline/pipeline-graph.c:393:8
    thesofproject#3 0x820d7ea in ipc_stream_pcm_free sof/sof/src/ipc/ipc3/handler.c:398:8
    thesofproject#4 0x8208969 in ipc_cmd sof/sof/src/ipc/ipc3/handler.c:1689:9
    thesofproject#5 0x81cbed8 in ipc_platform_do_cmd sof/sof/src/platform/posix/ipc.c:162:2
    thesofproject#6 0x81d10db in ipc_do_cmd sof/sof/src/ipc/ipc-common.c:330:9
    thesofproject#7 0x81f87e9 in task_run sof/sof/zephyr/include/rtos/task.h:94:9
    thesofproject#8 0x81f8308 in edf_work_handler sof/sof/zephyr/edf_schedule.c:31:16
    thesofproject#9 0x82b4b32 in work_queue_main sof/zephyr/kernel/work.c:668:3
    thesofproject#10 0x8193ec2 in z_thread_entry sof/zephyr/lib/os/thread_entry.c:36:2
    thesofproject#11 0x815f639 in __asan::AsanThread::ThreadStart(unsigned long long) /src/llvm-project/compiler-rt/lib/asan/asan_thread.cpp:277:25

Signed-off-by: Jyri Sarha <[email protected]>
@jsarha jsarha dismissed stale reviews from dbaluta and lgirdwood October 17, 2024 21:44

This new implementation that covers also IPC4, looks quite different.

@jsarha jsarha changed the title ipc3: handler: Add check for pipeline->source_comp being NULL pipeline: graph: Check if source_comp is NULL in pipeline_comp_reset() Oct 17, 2024
@jsarha jsarha force-pushed the fuzzer_fix_add_source_comp_test branch from 71afb2f to 5c1f737 Compare October 18, 2024 07:19
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jsarha ! I think this is better. We have many calls to pipeline_reset() and it's hard to conclude all the places are safe w.r.t. source_comp, so better to check.

@lgirdwood lgirdwood merged commit 73a17ae into thesofproject:main Oct 21, 2024
42 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants