New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 793425 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Add fuzzer for platform2/midis (seq_handler)

Project Member Reported by pmalani@chromium.org, Dec 8 2017

Issue description

platform2/midis has a SeqHandler class that handles interactions with ALSA lib. The routines in this class should be fuzzed.

Use either libfuzzer (with clusterfuzz), or, if that takes too long to land in platform2, use ossfuzz to implement this fuzzing.


 
Cc: -mnilsson@google.com mnissler@chromium.org
Components: OS>Kernel>Audio
I was looking into https://build.chromium.org/p/chromiumos/builders/amd64-generic-fuzzer/builds/394 and the build packages stage does not build midis.

So some extra deps are needed to make the amd64-generic-fuzzer build midis.
Hmm. What are these dependencies ? Is there a way to add them to the platform eclass? I bet other daemons would like to benefit from this too.
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/843b662e6ffb9ef04d55796c707beb03044e29da

commit 843b662e6ffb9ef04d55796c707beb03044e29da
Author: Prashant Malani <pmalani@google.com>
Date: Sat Mar 17 03:55:25 2018

midis: Enable seq_handler_fuzzer

BUG= chromium:793425 
TEST=Ran fuzzer locally, also ran unit tests
CQ-DEPEND=CL:965407

Change-Id: I0219f3c66b01c0d66e51a6227abf3b54352c3054
Reviewed-on: https://chromium-review.googlesource.com/966594
Commit-Ready: Manoj Gupta <manojgupta@chromium.org>
Tested-by: Prashant Malani <pmalani@google.com>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>

[modify] https://crrev.com/843b662e6ffb9ef04d55796c707beb03044e29da/chromeos-base/midis/midis-9999.ebuild

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/519dd74b32a3fb138dca3a1f01926de2fbf457f2

commit 519dd74b32a3fb138dca3a1f01926de2fbf457f2
Author: Prashant Malani <pmalani@google.com>
Date: Sat Mar 17 03:55:24 2018

midis: Add a fuzzer for SeqHandler

Currently, only ProcessMidiEvent is fuzzed.

BUG= chromium:793425 
TEST=Ran fuzzer locally, also ran unit tests.
CQ-DEPEND=CL:966594

Change-Id: Ia5e6b38534dbf166db640cdb77f9043e96a404df
Reviewed-on: https://chromium-review.googlesource.com/965407
Commit-Ready: Manoj Gupta <manojgupta@chromium.org>
Tested-by: Prashant Malani <pmalani@google.com>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>

[add] https://crrev.com/519dd74b32a3fb138dca3a1f01926de2fbf457f2/midis/seq_handler_fuzzer.cc
[modify] https://crrev.com/519dd74b32a3fb138dca3a1f01926de2fbf457f2/midis/midis.gyp
[modify] https://crrev.com/519dd74b32a3fb138dca3a1f01926de2fbf457f2/midis/seq_handler.h

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/989e17743a6f5b810fc333f8d2856b932ed05d7e

commit 989e17743a6f5b810fc333f8d2856b932ed05d7e
Author: Manoj Gupta <manojgupta@google.com>
Date: Tue Mar 20 09:22:01 2018

Fuzzing: add virtual/chromium-os-fuzzers package.

amd64-generic build may not build all packages with fuzzers.
Therefore, create new virtual packages virtual/target-fuzzers
and virtual/chromium-os-fuzzers that will pull in all packages
with fuzzers.

BUG= chromium:793425 
BUG= chromium:822828 

TEST=USE="fuzzer" ./build_packages builds chromeos-base/midis.

Change-Id: Iac338f858b6c226795ea4ee072138a2be23f0b14
Reviewed-on: https://chromium-review.googlesource.com/967003
Commit-Ready: Manoj Gupta <manojgupta@chromium.org>
Tested-by: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[add] https://crrev.com/989e17743a6f5b810fc333f8d2856b932ed05d7e/virtual/target-fuzzers/target-fuzzers-1-r1.ebuild
[rename] https://crrev.com/989e17743a6f5b810fc333f8d2856b932ed05d7e/virtual/target-chromium-os/target-chromium-os-1-r91.ebuild
[add] https://crrev.com/989e17743a6f5b810fc333f8d2856b932ed05d7e/virtual/target-fuzzers/target-fuzzers-1.ebuild
[add] https://crrev.com/989e17743a6f5b810fc333f8d2856b932ed05d7e/virtual/chromium-os-fuzzers/chromium-os-fuzzers-1.ebuild
[modify] https://crrev.com/989e17743a6f5b810fc333f8d2856b932ed05d7e/virtual/target-chromium-os/target-chromium-os-1.ebuild
[add] https://crrev.com/989e17743a6f5b810fc333f8d2856b932ed05d7e/virtual/chromium-os-fuzzers/chromium-os-fuzzers-1-r1.ebuild

Hey Manoj,

I now see the fuzzer on the dashboard! :)

However, it seems to be crashing on start up and I can't access the logs:
https://clusterfuzz.com/v2/fuzzer-stats/by-day/2018-02-24/2018-03-30/fuzzer/libFuzzer_midis_seq_handler_fuzzer/job/libfuzzer_asan_chromeos

Could you kindly attach the logs if you can access them?
Cc: manojgupta@chromium.org
Cc: infe...@chromium.org metzman@chromium.org
Abhishek/Joanathan,
Can you give access to Prashant?
Also, I would like to get access to logs as well if that is possible.
^I realize those are logs for puffin, but the permissions are job-wide.
Thanks, I can access the the logs in #12.
The fuzzer's output when run is suspect. Here's an example:

#67108864       pulse  cov: 136 ft: 138 corp: 2/2b lim: 4096 exec/s: 139810 rss: 759Mb

A couple things seem off first "cov" is 136, which is very low. Second the corpus is only size 2 and is 2 bytes (meaning that the fuzzer can reach all edges it is able to find with just 2 testcases of 1 byte each) third "exec/s" is 139,810 which is probably 10X faster than the simplest targets in chromium (eg: base64 decoder, md5).

How simple is this target? 
Is it comparable to a fuzzer that decodes or encodes base64 or one that calculates the md5 of it's input?

If not, I would guess that something is going wrong, probably in instrumenting. 

Manoj mentioned that some of the libraries it uses may not be instrumented with ASAN, I think they should be, and I would guess that they aren't instrumented using coverage, which they need to be.


The function being fuzzed is here:
https://cs.corp.google.com/chromeos_public/src/platform2/midis/seq_handler.cc?rcl=e05946b652b0c85fa7e0e9e384b07d580ebb4971&l=390

It calls into an ALSA library routine (snd_midi_event_decode())

I size of the snd_seq_event_t struct ptr passed to the SendMidiData function is probably smaller than the size of the data being passed to the fuzzer entry point, so I instantiate a struct and basically memcpy the necessary data into the struct.

How can I instrument the ALSA lib with ASAN and coverage? These are upstream libraries FWIU.
Can you build alsa-lib with asan? basically inherit toolchain-funcs and call asan-setup-env in src_prepare.

Note: You may have to move the build to chromiumos-overlay from portage-stable (verify that -fsanitize=address flags are being passed to alsa). 
The asan-setup-env command is not found when I add it to the portage-stable version.

Can you kindly elaborate on "move the build...."?

Is there a procedure to follow in moving an upstream library like this into chromiumos-overlay?
Just do a cp -r in chromiumos-overlay followed by rm -r in portage-stable.

Normally, for upstream libraries we want to keep them in portage-stable if we are not making any significant changes to the ebuild when compared to upstream.

But if we are, chromiumos-overlay is a better place to keep the ebuild.

There is some documentation here: https://www.chromium.org/chromium-os/gentoo-package-upgrade-process
Project Member

Comment 20 by bugdroid1@chromium.org, May 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/e6e6b27687233080d3ed60ac95b49feb5ca2f3b6

commit e6e6b27687233080d3ed60ac95b49feb5ca2f3b6
Author: Prashant Malani <pmalani@google.com>
Date: Sat May 05 03:37:30 2018

midis: Add fuzzer for SeqHandler::SendMidiData()

Also move the IsValidWebMIDIData() check to inside SendMidiData().
This is OK, because all data going to SendMidiData() *has* to go through
that check anyway, so we may as well put it in the same function.

BUG= chromium:793425 
TEST=emerge, run fuzzer locally.

Change-Id: Icfeb4ba117433e61369dc21054b28ea3161d9e4c
Reviewed-on: https://chromium-review.googlesource.com/1040308
Commit-Ready: Prashant Malani <pmalani@google.com>
Tested-by: Prashant Malani <pmalani@google.com>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>

[modify] https://crrev.com/e6e6b27687233080d3ed60ac95b49feb5ca2f3b6/midis/seq_handler_fuzzer.cc
[modify] https://crrev.com/e6e6b27687233080d3ed60ac95b49feb5ca2f3b6/midis/subdevice_client_fd_holder.cc
[modify] https://crrev.com/e6e6b27687233080d3ed60ac95b49feb5ca2f3b6/midis/seq_handler.cc

I was able to confirm that the third party libraries this uses are not properly instrumented. If you compare the first disassembly, which is taken from the target itself, to the second which is from the library it uses, you can see that the library is lacking compiler-inserted functions such as __asan_option_detect_stack_use_after_return and __sanitizer_cov_trace_const_cmp8

Theres still the issue with crashes that I'm looking into. And manoj is working on a global fix that may make this issue go away, so fixing this should be a low priority.
Selection_007.png
251 KB View Download
Selection_008.png
301 KB View Download
This should be fixed, right?
The issue I reported in #21 is fixed. 

But the fuzzer still has a problem that I pointed out here: https://bugs.chromium.org/p/chromium/issues/detail?id=840111#c5

It may make sense to close this bug and use the other one since the fuzzer was already added.
Status: Fixed (was: Assigned)
If there's another bug tracking problems then we can close this.

Sign in to add a comment