[fuzzilli] Redesign handling of coverage feedback and pkey sandboxing [v8/v8 : main]
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
PTAL! This is in preparation for upstreaming my pkey-based sandbox fuzzer.
Re. the drive-by change: I noticed that certain build configurations seem to immediately segfault because the instrumentation runs but shmem is still nullptr. This check fixes that and is also what's recommended by the official documentation (https://clang.llvm.org/docs/SanitizerCoverage.html#tracing-pcs-with-guards)
Related details
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
PTAL! This is in preparation for upstreaming my pkey-based sandbox fuzzer.
Re. the drive-by change: I noticed that certain build configurations seem to immediately segfault because the instrumentation runs but shmem is still nullptr. This check fixes that and is also what's recommended by the official documentation (https://clang.llvm.org/docs/SanitizerCoverage.html#tracing-pcs-with-guards)
I also did some quick benchmarking locally and the additional rdpkru doesn't seem to matter (it's a pretty fast instruction as it just reads a cpu register).
Related details
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
- Matthias Liedtke
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 1 comment![]( "Open in Gerrit")
__sanitizer_cov_trace_pc_guard(uint32_t* guard) {AFAIK, due to the fact that this is executed on each control-flow edge in the C++ binary, this is highly critical for performance.
Wasn't there some concerns on PKEY-modifying operations for V8 production? If yes, then this should probably have huge slowdowns when running Fuzzilli on PKEY-enabled hardware?
Also CC'ing @clem...@chromium.org who just investigated an incredibly slow test case when running it with Fuzzilli's coverage instrumentation a few days ago.
Related details
- Clemens Backes
- Michael Lippautz
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
__sanitizer_cov_trace_pc_guard(uint32_t* guard) {AFAIK, due to the fact that this is executed on each control-flow edge in the C++ binary, this is highly critical for performance.
Wasn't there some concerns on PKEY-modifying operations for V8 production? If yes, then this should probably have huge slowdowns when running Fuzzilli on PKEY-enabled hardware?Also CC'ing @clem...@chromium.org who just investigated an incredibly slow test case when running it with Fuzzilli's coverage instrumentation a few days ago.
Well so
(1) this only runs on builds that have V8_ENABLE_SANDBOX_HARDWARE_SUPPORT (and V8_FUZZILLI) enabled, which none of our fuzzer builds have
(2) this is only a RDPKRU on the common path, which is the fast instruction (4-6 CPU cycles or so, faster than a non-L1-cached read/write). I did some simple local benchmarking and couldn't spot a perf. difference
Related details
- Clemens Backes
- Matthias Liedtke
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
__sanitizer_cov_trace_pc_guard(uint32_t* guard) {Samuel GroßAFAIK, due to the fact that this is executed on each control-flow edge in the C++ binary, this is highly critical for performance.
Wasn't there some concerns on PKEY-modifying operations for V8 production? If yes, then this should probably have huge slowdowns when running Fuzzilli on PKEY-enabled hardware?Also CC'ing @clem...@chromium.org who just investigated an incredibly slow test case when running it with Fuzzilli's coverage instrumentation a few days ago.
Well so
(1) this only runs on builds that have V8_ENABLE_SANDBOX_HARDWARE_SUPPORT (and V8_FUZZILLI) enabled, which none of our fuzzer builds have
(2) this is only a RDPKRU on the common path, which is the fast instruction (4-6 CPU cycles or so, faster than a non-L1-cached read/write). I did some simple local benchmarking and couldn't spot a perf. difference
I ran some proper benchmarks now (Jetstream3):
========== Before this CL ==========
--- Run 1 ---
Stdlib: 0.083
MainRun: 0.007
First: 3.374
Worst: 8.238
Average: 12.563
Startup: 4.100
Runtime: 0.760
Total Score: 5.535
--- Run 2 ---
Stdlib: 0.082
MainRun: 0.007
First: 3.302
Worst: 7.953
Average: 12.255
Startup: 3.709
Runtime: 0.749
Total Score: 5.367
========== After this CL ==========
--- Run 1 ---
Stdlib: 0.084
MainRun: 0.007
First: 3.393
Worst: 8.134
Average: 12.446
Startup: 4.252
Runtime: 0.759
Total Score: 5.521
--- Run 2 ---
Stdlib: 0.084
MainRun: 0.007
First: 3.348
Worst: 8.167
Average: 12.623
Startup: 4.398
Runtime: 0.749
Total Score: 5.535
So I don't think it matters for performance.
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
Ack, it doesn't look very expensive to me.
What was slow was the combination of no-inline and fuzzilli (in particular this tracing callback). With inlining we already see a great reduction in calls to this callback, so that problem is solved. Making it slightly slower shouldn't be a problem.
Related details
- Matthias Liedtke
- Michael Lippautz
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Code-Review | +1 |
1 comment
__sanitizer_cov_trace_pc_guard(uint32_t* guard) {- Clemens Backes
- Michael Lippautz
- Samuel Groß
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted Code-Review+1![]( "Open in Gerrit")
- Clemens Backes
- Samuel Groß
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Clemens Backes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[fuzzilli] Redesign handling of coverage feedback and pkey sandboxing
When hardware sandboxing is enabled, sandboxed code can only write to
in-sandbox data. In combination with coverage feedback (for fuzzer
builds), this becomes a problem as the coverage bitmap needs to be
written. Previously, we tagged the coverage bitmap with a custom pkey
that is writable for both sandboxed and privileged code. However, this
does not work for signal handlers, as they run with access to only the
default pkey. As such, with this CL we now take a different approach: we
leave the coverage bitmap tagged with the default pkey but check if we
have access to that pkey in __sanitizer_cov_trace_pc_guard, and if not,
temporarily grant us access. This adds some overhead when running in
sandboxed execution mode, but currently we don't have any C++ code (that
would be instrumented) running in that mode (we only have a few builtins
that are invoked in sandboxed mode but then immediately ExitSandbox()).
Drive-By: bail out from __sanitizer_cov_trace_pc_guard if the index is
zero. See the comment for more details.
- M src/d8/d8.cc
- M src/fuzzilli/cov.cc
- M src/fuzzilli/cov.h
Code-Review: +1 by Matthias Liedtke, +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |