-
Notifications
You must be signed in to change notification settings - Fork 1.3k
KFuzzTest: proof of concept #6280
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
base: master
Are you sure you want to change the base?
KFuzzTest: proof of concept #6280
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do more batches later.
arg := queue.pop() | ||
padWithAlignment(&encoded, arg.Type().Alignment(), 0) | ||
|
||
switch a := arg.(type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's concerning that we're getting one more place in the code where we manually traverse the argument tree and switch over the argument types.
Is there some way to restructure this code using the Foreach*
functions we have in https://github.com/google/syzkaller/blob/master/prog/analysis.go?
I also wonder if we can rely more on the functionality we already have in https://github.com/google/syzkaller/blob/master/prog/alloc.go and in execContext.writeArg
. In particular, memAlloc
could be taught to insert kFuzzTestPoisonSize
between the allocated data and writeArg
could optionally keep track of the pointers/where they point.
1e61119
to
d15ea9d
Compare
@gemini /review |
@gemini-cli /review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This pull request is a solid proof-of-concept for integrating KFuzzTest into syzkaller. The overall structure is well-thought-out, with clear separation of concerns between extracting KFuzzTest definitions, building syzlang descriptions, and executing the tests.
🔍 General Feedback
- The main point of feedback is the extensive use of
panic
throughout the new code. While this might be acceptable for a proof-of-concept, it should be replaced with proper error handling to make the code more robust and suitable for production use, especially in library code. - There are a few places where constants are hardcoded (e.g., file paths, syscall IDs). It would be better to define these in a central place to improve maintainability.
- The code is generally well-written and easy to follow. The addition of documentation and a standalone tool for generating descriptions is a great touch.
Overall, this is a great start, and with some improvements to error handling and configuration, it will be a valuable addition to syzkaller.
pkg/kfuzztest/builder.go
Outdated
} | ||
|
||
// just appends the type as it appear in the | ||
append_type: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
low: The use of goto
here makes the code harder to follow. It can be refactored to use a boolean flag or by restructuring the if/else
blocks to avoid it.
|
||
// marshallKFuzzTestArg serializes a syzkaller Arg into a flat binary format | ||
// understood by the KFuzzTest kernel interface (see `include/linux/kfuzztest.h`). | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium: The function panics on unsupported types. It would be better to return an error to allow the caller to handle this case, for example by skipping the unsupported program.
89b70a6
to
444f025
Compare
ff828c0
to
f533625
Compare
Adds a Go-native KCOV package, which helper functions for tracing a call. This is in preparation for a standalone KFuzzTest tool, which should be written in Go in order to take advantage of existing fuzzing infrastructure. Signed-off-by: Ethan Graham <[email protected]>
This commit introduces logic and types needed for KFuzzTest. In particular, we introduce a new pseudo-syscall syz_kfuzztest_run with specialized encoding for the KFuzzTest input format, as well as a new KFuzzTest attribute used to distinguish normal syscalls from KFuzzTest targets. Signed-off-by: Ethan Graham <[email protected]>
This commit introduces an extend method to target, which is used to dynamically add new system calls to a target at runtime. This is in preparation for KFuzzTest dynamic discovery which uses dwarf parsing to identify KFuzzTest targets at runtime. Signed-off-by: Ethan Graham <[email protected]>
Add pkg/kfuzztest, which contains the core logic for parsing targets fuzz targets (including domain constraints and annotations) from a vmlinux binary. This is namely used for dynamically discovering KFuzzTest targets, generating and generating syzkaller descriptions for them. Signed-off-by: Ethan Graham <[email protected]>
Introduces the syz-kfuzztest tool, as well as a new executor file in pkg/kfuzztest. This tool, alongside existing fuzzing infrastructure, performs coverage guided fuzzing of KFuzzTest targets, collecting coverage using pkg/kcov and outputting a list of collects program counters which can be fed into tools/syz-cover. Signed-off-by: Ethan Graham <[email protected]>
497948e
to
aed938f
Compare
We favour only logging something on failure.
Remove flag package, as we only parse a single argument every time. Instead, we can just look at os.Args[1] to get the path to VMLinux.
4993bed
to
e82e345
Compare
e82e345
to
2ef1912
Compare
It was proving difficult and unreliable to modify the string generation logic to ensure null-termination of KFuzzTest strings. Doing so directly within the encoding is more reliable, and seems to result in no false positive string overflows.
Prior to this, we were doing pattern matching on the dwarf type names. This is brittle, and means that support for new types (e.g., u64) would have to be added manually. Switch to an approach that recurses over the dwarf types, resolving typedefs into primitive types that map very easily onto syzkaller types. Also refactor some of the code related to annotation processing, which was become unwieldly.
5642304
to
f963d06
Compare
0a29c84
to
221f297
Compare
221f297
to
91dc22a
Compare
Important
These commits are not cleaned at all - they are taken directly from my personal development branch, and the intention is to cleanly chunk it up at a later point when the code has stabilized, and a kernel RFC is opened.
The source of truth for this PR is the global diff, rather than the commits themselves.
Description
Proof of concept for KFuzzTest integration in syzkaller.
Readme in
docs/kfuzztest.md
.Additions
syz-kfuzztest
.syz-manager
for dynamic discovery and continuous fuzzing ofof KFuzzTest targets.
for KFuzzTest targets discovered from a
vmlinux
binary.