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.

Sign up for GitHub

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

Jump to bottom

Bochscpu: edge coverage #137

Merged
merged 12 commits into from
Dec 18, 2022
Merged

Bochscpu: edge coverage #137

merged 12 commits into from
Dec 18, 2022

Conversation

clslgrnc
Copy link
Contributor

@clslgrnc clslgrnc commented Oct 9, 2022

Enable edge coverage for the Bochscpu backend

@0vercl0k
Copy link
Owner

Amazing! Let me review this when I'm back from Hexacon 🙂

Cheers

@0vercl0k
fix a few things: edges in both run/fuzz, use same before_execution h…
1dff8a6 
…ook, nits here and there, runstats for edges
@0vercl0k
Copy link
Owner

I tried to fix up a few things, mainly removing the special path for the before_execution hook. After thinking about it, it should be fine, but let me know if I am missing anything!

Cheers

@clslgrnc
Copy link
Contributor Author

My initial intent was to replace the RIP coverage with edge coverage, I also expected the option to only be used in the fuzz mode.
BochscpuBackend_t::BeforeExecutionHookNoCover is thus a copy of BochscpuBackend_t::BeforeExecutionHook without RIP coverage and trace generation, while still processing breakpoints.

In terms of exploration, it probably does not matter. Having the RIP actually helps when combining bochscpu with other fuzzers running the KVM backend. After running the bochscpu edge fuzzers on the initial corpus, starting KVM backends would add spurious samples to the corpus if RIP was not registered as coverage.
On the other end, not registering RIP might be slightly faster.

I do not have any strong opinion on the matter

I did not thought about the edges stats, that's great.

@0vercl0k
Copy link
Owner

0vercl0k commented Nov 4, 2022

Also tagging @yrp604 to take a look at this whenever he has a minute.

Cheers

@yrp604
Copy link

yrp604 commented Nov 11, 2022

This looks good to me.

There are major perf advantages if we can avoid the overhead of per-instruction hooks which this gets us closer to switching to. With this plus something like the kvm breakpoint strategy it might be possible?

@0vercl0k 0vercl0k mentioned this pull request Nov 13, 2022
Open
@0vercl0k
Copy link
Owner

That's a good idea - I don't think it should be hard to move away from those callbacks actually. If I'm not mistaken, we're only using them for coverage, tracing and breakpoints. We'd just need to turn on edge coverage by default and implement breakpoints without the hook. I filed #141 to investigate the speed-up and implement if it looks good.

I am currently pushing a few changes to bxcpu so will wait for them to land, update wtf's version and land this 🍾😃

Cheers

@0vercl0k
Copy link
Owner

Okay all the bxcpu changes have landed, I've updated the header / the binaries. I've run it through old snapshot / traces and there seems to be an issue somewhere.. I'll have to debug what's going on tomorrow 😔.

Cheers

@0vercl0k
old bdump used to dump mxcsr_mask as 0. because of a bug in bxcpu, th…
bd24a8b 
…is used to have no effect.

This was fixed in bx (see 0vercl0k#140), so old dumps running w/ newer version of wtf will fire #GPs on `ldmscxr` instructions.
To avoid that issue, let's detect screwed up values and fix them up ourselves.
@0vercl0k
Copy link
Owner

0vercl0k commented Dec 15, 2022 •
edited
Loading

Okay I investigated what was going on. Basically, the old dump I ran my test one must have been dumped with an old version of bdump with a mxcsr_mask set to 0. Until now this didn't matter because of the bug found in #140. But now that this bug is fixed trying to execute ldmxcsr will trigger a #GP. Note that bdump has been fixed in yrp604/bdump@5a86bdc to set it to a 'sane' default value, and I've added code in SanitizeCpuState that will do the same if a dump is loaded with an mxcsr_mask set to 0 to not break users of new wtf with an old dump.

I've run a trace through an old dump and they are now the same which is great:

>fc output_old.txt output_new.txt
Comparing files output_old.txt and OUTPUT_NEW.TXT
***** output_old.txt
Setting debug register status to zero.
Running targets\ida64_dmp\ntfs-3g
***** OUTPUT_NEW.TXT
Setting debug register status to zero.
Setting mxcsr_mask to 0xffbf.
Running targets\ida64_dmp\ntfs-3g
*****

***** output_old.txt
      Memory accesses: 1933930864 bytes (1844 MB)
#1 cov: 267759 exec/s: 0.0 lastcov: 0.0s crash: 0 timeout: 0 cr3: 0 uptime: 1.3min
***** OUTPUT_NEW.TXT
      Memory accesses: 1933930864 bytes (1844 MB)
       Edges executed: 0 (0 unique)
#1 cov: 267759 exec/s: 0.0 lastcov: 0.0s crash: 0 timeout: 0 cr3: 0 uptime: 1.6min
*****

And same with --edges on:

>fc output_old.txt output_new_edges.txt
Comparing files output_old.txt and OUTPUT_NEW_EDGES.TXT
***** output_old.txt
Setting debug register status to zero.
Running targets\ida64_dmp\ntfs-3g
***** OUTPUT_NEW_EDGES.TXT
Setting debug register status to zero.
Setting mxcsr_mask to 0xffbf.
Running targets\ida64_dmp\ntfs-3g
*****

***** output_old.txt
Run stats:
Instructions executed: 709891705 (267759 unique)
          Dirty pages: 92413952 bytes (5 MB)
***** OUTPUT_NEW_EDGES.TXT
Run stats:
Instructions executed: 709891705 (309770 unique)
          Dirty pages: 92413952 bytes (5 MB)
*****

***** output_old.txt
      Memory accesses: 1933930864 bytes (1844 MB)
#1 cov: 267759 exec/s: 0.0 lastcov: 0.0s crash: 0 timeout: 0 cr3: 0 uptime: 1.3min
***** OUTPUT_NEW_EDGES.TXT
      Memory accesses: 1933930864 bytes (1844 MB)
       Edges executed: 114881293 (42011 unique)
#1 cov: 309770 exec/s: 0.0 lastcov: 0.0s crash: 0 timeout: 0 cr3: 0 uptime: 42.0s
*****

@clslgrnc I'm not sure if you still have old dump / states, but it'd be nice if you could give this a shot as well to build confidence but no worries if you have moved on / have no more time to spend on this, I understand 🙂

I'll try to bench perfs between the two versions and if I don't hear back in a week or so will merge as is!

Cheers

@clslgrnc
Copy link
Contributor Author

@clslgrnc I'm not sure if you still have old dump / states, but it'd be nice if you could give this a shot as well to build confidence but no worries if you have moved on / have no more time to spend on this, I understand slightly_smiling_face

Unfortunately my disk died recently. I did not backup my dumps and I won't have time to generate new ones before next year.

Thanks for the good work

best

@0vercl0k
Copy link
Owner

0vercl0k commented Dec 16, 2022 via email

@0vercl0k 0vercl0k merged commit af9730a into 0vercl0k:main Dec 18, 2022
@clslgrnc
Copy link
Contributor Author

No worries at all Colas, I hope you didn't lose anything important 🤞🏽sweat_smile Cheers

Fortunately no
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clslgrnc @0vercl0k @yrp604