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

Unicorn CompareCoverage + Neverzero counters #50

Merged
merged 8 commits into from Aug 29, 2019
Merged

Conversation

@andreafioraldi
Copy link
Collaborator

andreafioraldi commented Aug 28, 2019

Now CompareCoverage is enabled for unicorn_mode (only x86 atm).
I changed a lot the structure of the patch for unicorn and so performance tests are needed.
My preliminary tests shows that the speed is almost the same, please double check (maybe @domenukk directly if you want so that you can also check the compatibility with your unicorefuzz).
I added a test for compcov, unicorn_mode/samples/compcov_x64.
Like QEMU, AFL_COMPCOV_LEVEL=1 enable compcov only for comparisons with immediates and AFL_COMPCOV_LEVEL=2 logs all comparisons.

Regards other architectures I'm an instrinsic x86 guy and I'm not confident in implementing this for the other architectures. If you want to help me look at the qemu/target-i386/trannslate.c patch and insert proper calls to afl_gen_compcov after the comparisons in the other files (qemu/target-ARCHNAME/translate.c).

@andreafioraldi

This comment has been minimized.

Copy link
Collaborator Author

andreafioraldi commented Aug 28, 2019

ATTENTION
I accidentally created this branch not from master but from qemu_neverzero, make sure to merge before such branch and than merge this (or don't merge qemu_neverzero at all),

@andreafioraldi andreafioraldi changed the title Unicorn CompareCoverage Unicorn CompareCoverage + Neverzero counters Aug 28, 2019
Copy link
Collaborator

domenukk left a comment

We might simply want to do something like

  cur_loc &= MAP_SIZE - 7;

in

So that we never write out of bounds, what do you think @andreafioraldi

unicorn_mode/patches/afl-unicorn-tcg-runtime-inl.h Outdated Show resolved Hide resolved
@andreafioraldi

This comment has been minimized.

Copy link
Collaborator Author

andreafioraldi commented Aug 28, 2019

Fixed, Ty for the tip

@domenukk

This comment has been minimized.

Copy link
Collaborator

domenukk commented Aug 29, 2019

Segfaults for me (SIGSEV) :(

   0x7ffff5986a36 <cpu_x86_exec+902> xor    rsi, r14
   0x7ffff5986a39 <cpu_x86_exec+905> inc    BYTE PTR [rcx+rsi*1]
0x7ffff5986a3c <cpu_x86_exec+908> adc    DWORD PTR [rcx+rsi*1], 0x0

I'll try to see what and why...

@domenukk

This comment has been minimized.

Copy link
Collaborator

domenukk commented Aug 29, 2019

Oke got this covered.
It was reading out of bounds as it would access dword instead of byte ptrs... Fixes in #51.
This leads me to the question, why do we have the assembly code twice now per project (4 times)?
If it's a define already, wouldn't it make sense to use it form one shared header file?
At most two - QEMU and UC?

@domenukk

This comment has been minimized.

Copy link
Collaborator

domenukk commented Aug 29, 2019

Oke so non-scientific results:
Your new instrumentation was almost 15% faster(!) over a run of 12 minutes.
However, even though I was using AFL_COMPCOV_LEVEL=2 in these 12 minutes, it only found 106 vs 130 prior to the patch.
Probably it just got lucky the first time around or there are just very few longer compares in my test code (ASN 1 parser, probably mostly byte based...)
Will play around some more.
All in all my verdict is: Great work, ship it! ;)

Fixed SIGSEV due to wrong pointer size
@andreafioraldi

This comment has been minimized.

Copy link
Collaborator Author

andreafioraldi commented Aug 29, 2019

Ok ty for the adc fix, it was a typo.
Regards AFL_COMPCOV_LEVEL=2 it should increase the number of discovered BBs after some time. In the first minutes the normal instrumentation should perform better in terms of BBs discovered.

@andreafioraldi

This comment has been minimized.

Copy link
Collaborator Author

andreafioraldi commented Aug 29, 2019

Regards the two INC_AFL_AREA macros in QEMU and Uc, they are similar but not equal.
In Unicorn there is uc->afl_area_ptr. I moved these variables in the uc_struct cause Unicorn duplicates the tcg.o and cpu-exec.o for each architecture and if the variables are not static (they can't be static if I must use it for compcov) in the end when linking libunicorn.a results in a mutiple symbols definition.

@andreafioraldi

This comment has been minimized.

Copy link
Collaborator Author

andreafioraldi commented Aug 29, 2019

Last thing before merge: @domenukk try to repeat your experiment using an uninformed input (eg. "\x00"*32) and compcov level 2.
In the example that I inserted here https://github.com/vanhauser-thc/AFLplusplus/tree/uc_compcov/unicorn_mode/samples/compcov_x64 for me works.

@domenukk

This comment has been minimized.

Copy link
Collaborator

domenukk commented Aug 29, 2019

Regards the two INC_AFL_AREA macros in QEMU and Uc, they are similar but not equal.
In Unicorn there is uc->afl_area_ptr. I moved these variables in the uc_struct cause Unicorn duplicates the tcg.o and cpu-exec.o for each architecture and if the variables are not static (they can't be static if I must use it for compcov) in the end when linking libunicorn.a results in a mutiple symbols definition.

I agree they are not equal in QEMU and UC (could still be combined somhow, probably), however even in one mode, they are used twice.
For example in unicorn at

#if (defined(__x86_64__) || defined(__i386__)) && defined(AFL_QEMU_NOT_ZERO)

and at

I would suggest using INC_AFL_AREA(loc) in afl-unicorn-cpu-inl.h - maybe by adding a new header file for it - also?
That way future patches (Neverzero for different architectures, etc.) don't need to touch multiple files.
Your call.

I'll do some additional "proper" testing of compcov later, I think I have some test cases where it will shine, thanks! :)

@andreafioraldi

This comment has been minimized.

Copy link
Collaborator Author

andreafioraldi commented Aug 29, 2019

See now.
Joining common stuffs between uc and qemu will be done in the code-cleanup branch later.

@domenukk

This comment has been minimized.

Copy link
Collaborator

domenukk commented Aug 29, 2019

For lack of time ended up doing the same (quick) test of the ASN1 parser again, but with an quasi-empty seed set.
Running the test (one time each, neverzero on), for 5 Minutes:
no AFL_COMPCOV_LEVEL: total paths: 39, total execs: 60,5k
AFL_COMPCOV_LEVEL=1: total paths: 46, total execs: 59,2k
AFL_COMPCOV_LEVEL=2: total paths: 49, total execs: 57,9k

Again, this test is very unscientific.
However I'm happy with the results. Setting compcov level 2 for unicorefuzz from now on. 🎉

@andreafioraldi

This comment has been minimized.

Copy link
Collaborator Author

andreafioraldi commented Aug 29, 2019

It's time to merge!

@andreafioraldi andreafioraldi merged commit f677427 into master Aug 29, 2019
@vanhauser-thc

This comment has been minimized.

Copy link
Owner

vanhauser-thc commented Aug 30, 2019

@domenukk what path value are you comparing? the afl path value? that is not a good one :)
If so: It is better to compile the target with gcov/lcov and then run the afl queue entries against that and then compare the line coverage. e.g. afl-cov is a great script doing that and removing a lot of the work.

@andreafioraldi andreafioraldi deleted the uc_compcov branch Aug 30, 2019
@andreafioraldi

This comment has been minimized.

Copy link
Collaborator Author

andreafioraldi commented Aug 30, 2019

Yeah @vanhauser-thc you're 100% right but running afl-cov on pieces of binary data emulated with Unicorn is not so simple. He should adapt this instrumentation of QEMU (https://github.com/andreafioraldi/afl-queue-bb-coverage) to Unicorn to measure BBs coverage of afl-unicorn.

@domenukk

This comment has been minimized.

Copy link
Collaborator

domenukk commented Aug 30, 2019

Yeah this occurred to me also yesterday, since compcov might add new artificial edges, I guess(?)
I do have a module ported to userspace, I could try on - or try out bb coverage 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.