trace: enforce mailbox size by cujomalainey · Pull Request #3558 · thesofproject/sof

cujomalainey · 2020-10-27T23:50:05Z

If we are fuzzing we can get the trace system into a state where it
tries flush dma and overshoot the mailbox boundary.

Signed-off-by: Curtis Malainey cujomalainey@chromium.org

If we are fuzzing we can get the trace system into a state where it tries flush dma and overshoot the mailbox boundary. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

cujomalainey · 2020-10-27T23:51:19Z

For record here is log from the fuzzer

...
unknown ipc: unknown pm cmd 0xd450000                                                          
unknown ipc: dai 0,0 not found                                                                                                                                                                 
unknown ipc: debug cmd 0x10000                                                                                                                                                                 
unknown FW ABI 0x3012001 DBG ABI 0x5002001 tag v1.5-rc1-833-gaa628bdcf966 src hash 0x2ca82749 (ldc hash 0x2ca82749)
=================================================================
==11==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000114b180 at pc 0x0000005203ca bp 0x7ffc048f7d00 sp 0x7ffc048f74c8
WRITE of size 4096 at 0x00000114b180 thread T0                                                 
SCARINESS: 45 (multi-byte-write-global-buffer-overflow)          
    #0 0x5203c9 in __asan_memcpy /src/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
    #1 0x5763c4 in arch_memcpy_s /src/sof/src/arch/host/include/arch/string.h:44:2
    #2 0x5763c4 in memcpy_s /src/sof/src/lib/lib.c:38:9          
    #3 0x57bb0c in dma_trace_flush /src/sof/src/trace/dma-trace.c:394:9
    #4 0x57f872 in trace_flush /src/sof/src/trace/trace.c:320:2  
    #5 0x57b575 in dma_trace_enable /src/sof/src/trace/dma-trace.c:339:2
    #6 0x55c4b7 in ipc_dma_trace_config /src/sof/src/ipc/handler.c:740:8
    #7 0x55c4b7 in ipc_glb_trace_message /src/sof/src/ipc/handler.c:810:10
    #8 0x55c4b7 in ipc_cmd /src/sof/src/ipc/handler.c:1410:9
    #9 0x5535a9 in LLVMFuzzerTestOneInput /src/sof/tools/oss-fuzz/fuzz_ipc.c:28:2
    #10 0x4581d1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:595:15
    #11 0x45771a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:503:3
    #12 0x4593c7 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:741:19
    #13 0x459e55 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:879:5
    #14 0x4491d4 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:902:6
    #15 0x471342 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #16 0x7fdf96ba883f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
    #17 0x41e578 in _start (/out/fuzz_ipc+0x41e578)

DEDUP_TOKEN: __asan_memcpy--arch_memcpy_s--memcpy_s
0x00000114b180 is located 0 bytes to the right of global variable 'mailbox' defined in '/src/sof/src/platform/library/platform.c:17:16' (0x114a200) of size 3968
SUMMARY: AddressSanitizer: global-buffer-overflow /src/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3 in __asan_memcpy
Shadow bytes around the buggy address:
  0x0000802215e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0000802215f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x000080221600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x000080221610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x000080221620: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x000080221630:[f9]f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x000080221640: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x000080221650: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x000080221660: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x000080221670: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x000080221680: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==11==ABORTING

xiulipan

@cujomalainey I think this PR as workaround is OK, but it seems not a correct fix.
The dma_trace_flush is used to dump trace message not sent by DMA trace buffer yet to mailbox. This PR just drop the overflow part.

@ranj063 Please help to check this issue, I think we should dump the latest logs to help our debug. I think we may have some mismatch with DMA_FLUSH_TRACE_SIZE and MAILBOX_TRACE_SIZE

cujomalainey · 2020-10-28T02:34:35Z

@xiulipan I agree this probably isn't correct but it works for now.

All definitions of DMA_FLUSH_TRACE_SIZE are #define DMA_FLUSH_TRACE_SIZE (MAILBOX_TRACE_SIZE >> 2)

xiulipan · 2020-10-28T02:48:32Z

@cujomalainey Interesting, if #define DMA_FLUSH_TRACE_SIZE (MAILBOX_TRACE_SIZE >> 2) I think you change won't have any effect here, size should always be DMA_FLUSH_TRACE_SIZE.

We may need to carefully check what the size means here, if it is byte size or word size.
And we may also have issue with the buffer available size.

Could you send an issue first with steps to reproduce the issue. I think we need root cause and have a correct fix for it.

cujomalainey · 2020-10-28T02:57:26Z

@cujomalainey Interesting, if #define DMA_FLUSH_TRACE_SIZE (MAILBOX_TRACE_SIZE >> 2) I think you change won't have any effect here, size should always be DMA_FLUSH_TRACE_SIZE.

We may need to carefully check what the size means here, if it is byte size or word size.
And we may also have issue with the buffer available size.

Could you send an issue first with steps to reproduce the issue. I think we need root cause and have a correct fix for it.

It has some effect as it fixes the fuzzer. The system is trying to load 0x1000 bytes into a space of 0x380. The min causes the system to not trigger the sanitizer.

lgirdwood

@ktrzcinx what's your take. IIRC trace should only be writing to the error mailbox section (for trace errors) ?

ktrzcinx · 2020-10-30T09:31:49Z

IIRC trace should only be writing to the error mailbox section (for trace errors) ?

Errors are always written to mailbox and after setting CONFIG_TRACEM then each message is written there. It happens in trace_log function.

@ktrzcinx what's your take.

as @xiulipan said, when DMA_FLUSH_TRACE_SIZE (MAILBOX_TRACE_SIZE >> 2) then this min shouldn't change anything (btw. I feel there should be rather MIN(size, DMA_FLUSH_TRACE_SIZE);). This part of code is used mainly after panic, during normal operations it's not used, so it's possible that here is some bug.

lgirdwood · 2020-10-30T14:28:08Z

I clicked the wrong button, I was going to comment that the trace and mailbox sizes probably need validated since the macros were added prior to the trace architecture we have today.

incorrectly clicked

cujomalainey · 2020-11-05T18:05:03Z

@ktrzcinx the min does have enough of an effect to satisfy the fuzzer from going out of bounds. Would you accept this fix if I moved it to the dma flush size constant?

ktrzcinx · 2020-11-06T08:50:08Z

@cujomalainey I see that it works, but it conceptually doesn't fit - the same thing is done twice or rather three times (size calculation based on avail, on r/w ptrs and min). I don't fully understand why we have some advance size calculation before, introduced here cb1fc87, simple min looks more convenience for me, so I would like to removed old size calculation and keep only yours version, but @ranj063 is the code author so may take voice here.

lgirdwood · 2020-11-11T15:58:11Z

@ranj063 ping any comments - I'm minded to merge this and wait for any reports of impact (if any).

lgirdwood · 2020-11-16T11:12:01Z

CI looks good so lets see what happens with different kernels and userspace.

trace: enforce mailbox size

cd99285

If we are fuzzing we can get the trace system into a state where it tries flush dma and overshoot the mailbox boundary. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

cujomalainey requested review from akloniex and xiulipan as code owners October 27, 2020 23:50

cujomalainey requested review from lgirdwood and ranj063 October 27, 2020 23:50

cujomalainey mentioned this pull request Oct 28, 2020

sound-open-firmware: add build files google/oss-fuzz#4527

Merged

2 tasks done

xiulipan suggested changes Oct 28, 2020

View reviewed changes

cujomalainey mentioned this pull request Oct 28, 2020

[FEATURE] Add sof to oss-fuzz #3388

Closed

lgirdwood reviewed Oct 29, 2020

View reviewed changes

lgirdwood previously approved these changes Oct 30, 2020

View reviewed changes

lgirdwood approved these changes Nov 16, 2020

View reviewed changes

lgirdwood merged commit 221189d into thesofproject:master Nov 16, 2020

cujomalainey deleted the dma-fiz branch December 3, 2020 23:21

Navigation Menu

Repository navigation

trace: enforce mailbox size - #3558

trace: enforce mailbox size#3558
lgirdwood merged 1 commit into
thesofproject:masterfrom
cujomalainey:dma-fiz

cujomalainey commented Oct 27, 2020

Uh oh!

cujomalainey commented Oct 27, 2020

Uh oh!

xiulipan left a comment

Uh oh!

cujomalainey commented Oct 28, 2020

Uh oh!

xiulipan commented Oct 28, 2020

Uh oh!

cujomalainey commented Oct 28, 2020

Uh oh!

lgirdwood left a comment

Uh oh!

ktrzcinx commented Oct 30, 2020

Uh oh!

lgirdwood commented Oct 30, 2020

Uh oh!

cujomalainey commented Nov 5, 2020

Uh oh!

ktrzcinx commented Nov 6, 2020

Uh oh!

lgirdwood commented Nov 11, 2020

Uh oh!

lgirdwood commented Nov 16, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cujomalainey commented Oct 27, 2020

Uh oh!

cujomalainey commented Oct 27, 2020

Uh oh!

xiulipan left a comment

Choose a reason for hiding this comment

Uh oh!

cujomalainey commented Oct 28, 2020

Uh oh!

xiulipan commented Oct 28, 2020

Uh oh!

cujomalainey commented Oct 28, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

ktrzcinx commented Oct 30, 2020

Uh oh!

lgirdwood commented Oct 30, 2020

Uh oh!

cujomalainey commented Nov 5, 2020

Uh oh!

ktrzcinx commented Nov 6, 2020

Uh oh!

lgirdwood commented Nov 11, 2020

Uh oh!

lgirdwood commented Nov 16, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants