Skip to content

bpf: Fix and test aux usage after do_check_insn() #5536

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
Closed
wants to merge 3 commits into from
Closed

bpf: Fix and test aux usage after do_check_insn() #5536

wants to merge 3 commits into from
+174 −9

Conversation

kernel-patches-daemon-bpf-rc[bot]
Copy link

Pull request for series with
subject: bpf: Fix and test aux usage after do_check_insn()
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=976916

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: c4b1be9
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976916
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: c4b1be9
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976916
version: 1

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: c4b1be9
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976926
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: c4b1be9
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976926
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: c4b1be9
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976926
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: c4b1be9
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976926
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 26d0e53
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976926
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 0df1a55
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976926
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: cce3fee
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976926
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 1230be8
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976926
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 212ec92
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976926
version: 2

@luisgerhorst
bpf: Update env->prev_insn_idx after do_check_insn()
a34e1f3 
To introduce prev_aux(env), env->prev_insn_idx must be up-to-date
directly after do_check_insn(). To achieve this, replace prev_insn_idx
with a tmp variable (to discourage use) and update env->prev_insn_idx
directly after do_check_insn().

A concern would be that some code relied on env->prev_insn_idx still
having the old value between do_check_insn() and the old
update-assignment. However, outside the do_check() function it is only
used through push_jmp_history()/do_check_insn()/is_state_visisted()
which are not called in-between the old and new assignment location.
This can also be seen from the -O0 call graph for push_jmp_history()
[1].

[1] https://sys.cs.fau.de/extern/person/gerhorst/25-06_d69baf_push_jmp_history_O0_callgraph.png

Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
@luisgerhorst
bpf: Fix aux usage after do_check_insn()
87eb680 
We must terminate the speculative analysis if the just-analyzed insn had
nospec_result set. Using cur_aux() here is wrong because insn_idx might
have been incremented by do_check_insn(). Therefore, introduce and use
prev_aux().

Also change cur_aux(env)->nospec in case do_check_insn() ever manages to
increment insn_idx but still fail.

Change the warning to check the insn class (which prevents it from
triggering for ldimm64, for which nospec_result would not be
problematic) and use verifier_bug_if().

Fixes: d6f1c85 ("bpf: Fall back to nospec for Spectre v1")
Reported-by: Paul Chaignon <paul.chaignon@gmail.com>
Reported-by: Eduard Zingerman <eddyz87@gmail.com>
Reported-by: syzbot+dc27c5fb8388e38d2d37@syzkaller.appspotmail.com
Link: https://lore.kernel.org/bpf/685b3c1b.050a0220.2303ee.0010.GAE@google.com/
Link: https://lore.kernel.org/bpf/4266fd5de04092aa4971cbef14f1b4b96961f432.camel@gmail.com/
Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
@luisgerhorst
selftests/bpf: Add Spectre v4 tests
0cd31b4 
Add the following tests:

1. A test with an (unimportant) ldimm64 (16 byte insn) and a
   Spectre-v4--induced nospec that clarifies and serves as a basic
   Spectre v4 test.

2. Make sure a Spectre v4 nospec_result does not prevent a Spectre v1
   nospec from being added before the dangerous instruction (tests that
   [1] is fixed).

3. Combine the two, which is the combination that triggers the warning
   in [2]. This is because the unanalyzed stack write has nospec_result
   set, but the ldimm64 (which was just analyzed) had incremented
   insn_idx by 2. That violates the assertion that nospec_result is only
   used after insns that increment insn_idx by 1 (i.e., stack writes).

[1] https://lore.kernel.org/bpf/4266fd5de04092aa4971cbef14f1b4b96961f432.camel@gmail.com/
[2] https://lore.kernel.org/bpf/685b3c1b.050a0220.2303ee.0010.GAE@google.com/

Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 621af19
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=976926
version: 2

@kernel-patches-daemon-bpf-rc
Copy link
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=976926 expired. Closing PR.

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
bpf-next changes-requested V2-ci-pass V2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@luisgerhorst