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

pkg/vminfo: move feature checking to host #4790

Merged
merged 1 commit into from May 15, 2024

Conversation

dvyukov
Copy link
Collaborator

@dvyukov dvyukov commented May 9, 2024

Feature checking procedure is split into 2 phases:

  1. syz-fuzzer invokes "syz-executor setup feature" for each feature one-by-one,
    and checks if executor does not fail.
    Executor can also return a special "this feature does not need custom setup",
    this allows to not call setup of these features in each new VM.
  2. pkg/vminfo runs a simple program with ipc.ExecOpts specific for a concrete feature,
    e.g. for wifi injection it will try to run a program with wifi feature enabled,
    if setup of the feature fails, executor should also exit with an error.
    For coverage features we also additionally check that we actually got coverage.
    Then pkg/vminfo combines results of these 2 checks into final result.

syz-execprog now also uses vminfo package and mimics the same checking procedure.

Update #1541

@dvyukov dvyukov enabled auto-merge May 9, 2024 12:23
glpesk added a commit to glpesk/syzkaller that referenced this pull request May 9, 2024
With this commit + google#4790,
syz-manager runs in host fuzzer mode (with
SYZ_STARNIX_HACK=1) without getting stuck on
feature setup.

Co-authored-by: [email protected]
executor/common_bsd.h Outdated Show resolved Hide resolved
@tarasmadan tarasmadan force-pushed the dvyukov-check-features-on-host branch from 152bd72 to 227dda6 Compare May 10, 2024 12:00
@tarasmadan
Copy link
Collaborator

Rebasing it to get the test coverage details.

Copy link

codecov bot commented May 10, 2024

Codecov Report

Attention: Patch coverage is 51.14007% with 150 lines in your changes are missing coverage. Please review.

Project coverage is 61.3%. Comparing base (94b087b) to head (1666a9b).

Additional details and impacted files
Files Coverage Δ
pkg/vminfo/linux_syscalls.go 63.4% <ø> (-2.6%) ⬇️
pkg/vminfo/vminfo.go 77.4% <100.0%> (+1.1%) ⬆️
syz-fuzzer/testing.go 0.0% <0.0%> (ø)
syz-fuzzer/proc.go 0.0% <0.0%> (ø)
pkg/vminfo/netbsd.go 62.5% <62.5%> (ø)
pkg/vminfo/openbsd.go 57.1% <57.1%> (ø)
pkg/vminfo/syscalls.go 79.8% <84.0%> (+0.9%) ⬆️
pkg/ipc/ipc.go 48.3% <0.0%> (-0.4%) ⬇️
syz-manager/rpc.go 0.0% <0.0%> (ø)
syz-fuzzer/fuzzer.go 4.0% <0.0%> (+0.2%) ⬆️
... and 1 more

... and 2 files with indirect coverage changes

Copy link
Collaborator

@a-nogikh a-nogikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good luck to us in merging the changes from this PR and from #4762 :)


Here's the summary I've tried to write after looking at the code. Does it look correct?

Some features (1) require a one-time setup, other features (2) are passed via ipc.EnvFlags and we must set them up on every syz-executor restart. In syz-fuzzer (actually, mostly in syz-executor), we now try to enable all (1) and also do tests for some of (2). Then, pkg/vminfo does more tests for (2).

There are also csource.Features, which are at this point just a collection of string flags for a subset (?) of (1) and (2). In host.SetupFeatures, we intersect it with the available features of type (1). In ipc.FeaturesToFlags, we intersect it with the available features (2) to form EnvFlags.

We don't seem to care about csource.Features in the generation of C programs at all, so do csource.Features have to stay in pkg/csource?

There's also csource.Options, which seems to both intersect with (1)/(2) and ipc.ExecOpts. csource.Options is only used for C repro generation and only determines what consts are substituted into the C code.


Interestingly, it seems that fuzzer.Request (= queue.Request from the other PR) is already moving to a direction of being a superset of all parameters we want to control when executing a program. Could it eventually cover also all of csource.Features and csource.Features and actually replace them?

I think it will be a very nice if we could both execute such a Request on a syz-executor and translate it to a C program without having to specify any more parameters.

tools/syz-execprog/execprog.go Show resolved Hide resolved
tools/syz-execprog/execprog.go Outdated Show resolved Hide resolved
tools/syz-execprog/execprog.go Show resolved Hide resolved
pkg/host/features.go Outdated Show resolved Hide resolved
pkg/vminfo/features.go Show resolved Hide resolved
pkg/vminfo/features.go Show resolved Hide resolved
pkg/runtest/run_test.go Show resolved Hide resolved
@dvyukov dvyukov force-pushed the dvyukov-check-features-on-host branch 2 times, most recently from afae8ac to af08937 Compare May 15, 2024 10:18
Feature checking procedure is split into 2 phases:
1. syz-fuzzer invokes "syz-executor setup feature" for each feature one-by-one,
and checks if executor does not fail.
Executor can also return a special "this feature does not need custom setup",
this allows to not call setup of these features in each new VM.
2. pkg/vminfo runs a simple program with ipc.ExecOpts specific for a concrete feature,
e.g. for wifi injection it will try to run a program with wifi feature enabled,
if setup of the feature fails, executor should also exit with an error.
For coverage features we also additionally check that we actually got coverage.
Then pkg/vminfo combines results of these 2 checks into final result.

syz-execprog now also uses vminfo package and mimics the same checking procedure.

Update google#1541
@dvyukov dvyukov force-pushed the dvyukov-check-features-on-host branch from af08937 to 1666a9b Compare May 15, 2024 12:24
@dvyukov dvyukov added this pull request to the merge queue May 15, 2024
@dvyukov
Copy link
Collaborator Author

dvyukov commented May 15, 2024

Some features (1) require a one-time setup, other features (2) are passed via ipc.EnvFlags and we must set them up on every syz-executor restart. In syz-fuzzer (actually, mostly in syz-executor), we now try to enable all (1) and also do tests for some of (2). Then, pkg/vminfo does more tests for (2).

Correct.

There are also csource.Features, which are at this point just a collection of string flags for a subset (?) of (1) and (2). In host.SetupFeatures, we intersect it with the available features of type (1). In ipc.FeaturesToFlags, we intersect it with the available features (2) to form EnvFlags.

We don't seem to care about csource.Features in the generation of C programs at all, so do csource.Features have to stay in pkg/csource?

There's also csource.Options, which seems to both intersect with (1)/(2) and ipc.ExecOpts. csource.Options is only used for C repro generation and only determines what consts are substituted into the C code.

csource.Features is not just subset of host/flatrpc.Features, they also contain some unique ones.
I don't know why it's in pkg/csource, I would say it should not stay anywhere at all :)

Interestingly, it seems that fuzzer.Request (= queue.Request from the other PR) is already moving to a direction of being a superset of all parameters we want to control when executing a program. Could it eventually cover also all of csource.Features and csource.Features and actually replace them?

I think it will be a very nice if we could both execute such a Request on a syz-executor and translate it to a C program without having to specify any more parameters.

Yes, we need 1 set of features that should cover all of them.
The single set of features must be in a low-level package to avoid import cycles and also C code will need access to them. So flatrpc features looks like the best candidate.
Flatbuffers generated code already allows some reflection (iterate over all flags, convert to string names). I think we should attach more meta info so that they can be used in all other places (e.g. which features are supported on which OSes, which features are only used by C source).

Merged via the queue into google:master with commit 0b3dad4 May 15, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants