Skip to content

Commit 95dbe21

Browse files
author
Alexei Starovoitov
committed
Merge branch 'bpf-fix-verifier_bug_if-to-account-for-bpf_call'
Luis Gerhorst says: ==================== bpf: Fix verifier_bug_if to account for BPF_CALL This fixes the verifier_bug_if() that runs on nospec_result to not trigger for BPF_CALL (bug reported by Hu, Mei, and Mu). See patch 1 for a full description and patch 2 for a test (based on the PoC from the report). While working on this I noticed two other problems: - nospec_result is currently ignored for BPF_CALL during patching, but it may be required if we assume the CPU may speculate into/out of functions. - Both the instruction patching for nospec and nospec_result erases the instruction aux information even thought it might be better to keep that. For nospec_result it may be fine as it is only applied to store instructions currently (except for when we decide to change the thing from above), but nospec may be set for arbitrary instructions and if these require rewrites they break. I assume these issues are better fixed separately, thus I decided to exclude them from this series. ==================== Link: https://patch.msgid.link/20260127115912.3026761-1-luis.gerhorst@fau.de Signed-off-by: Alexei Starovoitov <ast@kernel.org>
2 parents 08a7491 + 60d2c43 commit 95dbe21

File tree

2 files changed

+30
-6
lines changed

2 files changed

+30
-6
lines changed

kernel/bpf/verifier.c

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21065,17 +21065,19 @@ static int do_check(struct bpf_verifier_env *env)
2106521065
* may skip a nospec patched-in after the jump. This can
2106621066
* currently never happen because nospec_result is only
2106721067
* used for the write-ops
21068-
* `*(size*)(dst_reg+off)=src_reg|imm32` which must
21069-
* never skip the following insn. Still, add a warning
21070-
* to document this in case nospec_result is used
21071-
* elsewhere in the future.
21068+
* `*(size*)(dst_reg+off)=src_reg|imm32` and helper
21069+
* calls. These must never skip the following insn
21070+
* (i.e., bpf_insn_successors()'s opcode_info.can_jump
21071+
* is false). Still, add a warning to document this in
21072+
* case nospec_result is used elsewhere in the future.
2107221073
*
2107321074
* All non-branch instructions have a single
2107421075
* fall-through edge. For these, nospec_result should
2107521076
* already work.
2107621077
*/
21077-
if (verifier_bug_if(BPF_CLASS(insn->code) == BPF_JMP ||
21078-
BPF_CLASS(insn->code) == BPF_JMP32, env,
21078+
if (verifier_bug_if((BPF_CLASS(insn->code) == BPF_JMP ||
21079+
BPF_CLASS(insn->code) == BPF_JMP32) &&
21080+
BPF_OP(insn->code) != BPF_CALL, env,
2107921081
"speculation barrier after jump instruction may not have the desired effect"))
2108021082
return -EFAULT;
2108121083
process_bpf_exit:

tools/testing/selftests/bpf/progs/verifier_unpriv.c

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -950,4 +950,26 @@ l3_%=: r0 = 0; \
950950
" ::: __clobber_all);
951951
}
952952

953+
SEC("socket")
954+
__description("unpriv: nospec after dead stack write in helper")
955+
__success __success_unpriv
956+
__retval(0)
957+
/* Dead code sanitizer rewrites the call to `goto -1`. */
958+
__naked void unpriv_dead_helper_stack_write_nospec_result(void)
959+
{
960+
asm volatile (" \
961+
r0 = 0; \
962+
if r0 != 1 goto l0_%=; \
963+
r2 = 0; \
964+
r3 = r10; \
965+
r3 += -16; \
966+
r4 = 4; \
967+
r5 = 0; \
968+
call %[bpf_skb_load_bytes_relative]; \
969+
l0_%=: exit; \
970+
" :
971+
: __imm(bpf_skb_load_bytes_relative)
972+
: __clobber_all);
973+
}
974+
953975
char _license[] SEC("license") = "GPL";

0 commit comments

Comments
 (0)