| Message ID | 20221207103540.396496-1-bjorn@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d35af0a7feb077c43ff0233bba5a8c6e75b73e35 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,v2] bpf: Do not zero-extend kfunc return values | expand |
On 12/7/22 2:35 AM, Björn Töpel wrote: > From: Björn Töpel <bjorn@rivosinc.com> > > In BPF all global functions, and BPF helpers return a 64-bit > value. For kfunc calls, this is not the case, and they can return > e.g. 32-bit values. > > The return register R0 for kfuncs calls can therefore be marked as > subreg_def != DEF_NOT_SUBREG. In general, if a register is marked with > subreg_def != DEF_NOT_SUBREG, some archs (where bpf_jit_needs_zext() > returns true) require the verifier to insert explicit zero-extension > instructions. > > For kfuncs calls, however, the caller should do sign/zero extension > for return values. In other words, the compiler is responsible to > insert proper instructions, not the verifier. > > An example, provided by Yonghong Song: > > $ cat t.c > extern unsigned foo(void); > unsigned bar1(void) { > return foo(); > } > unsigned bar2(void) { > if (foo()) return 10; else return 20; > } > > $ clang -target bpf -mcpu=v3 -O2 -c t.c && llvm-objdump -d t.o > t.o: file format elf64-bpf > > Disassembly of section .text: > > 0000000000000000 <bar1>: > 0: 85 10 00 00 ff ff ff ff call -0x1 > 1: 95 00 00 00 00 00 00 00 exit > > 0000000000000010 <bar2>: > 2: 85 10 00 00 ff ff ff ff call -0x1 > 3: bc 01 00 00 00 00 00 00 w1 = w0 > 4: b4 00 00 00 14 00 00 00 w0 = 0x14 > 5: 16 01 01 00 00 00 00 00 if w1 == 0x0 goto +0x1 <LBB1_2> > 6: b4 00 00 00 0a 00 00 00 w0 = 0xa > > 0000000000000038 <LBB1_2>: > 7: 95 00 00 00 00 00 00 00 exit > > If the return value of 'foo()' is used in the BPF program, the proper > zero-extension will be done. > > Currently, the verifier correctly marks, say, a 32-bit return value as > subreg_def != DEF_NOT_SUBREG, but will fail performing the actual > zero-extension, due to a verifier bug in > opt_subreg_zext_lo32_rnd_hi32(). load_reg is not properly set to R0, > and the following path will be taken: > > if (WARN_ON(load_reg == -1)) { > verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n"); > return -EFAULT; > } > > A longer discussion from v1 can be found in the link below. > > Correct the verifier by avoiding doing explicit zero-extension of R0 > for kfunc calls. Note that R0 will still be marked as a sub-register > for return values smaller than 64-bit. > > Fixes: 83a2881903f3 ("bpf: Account for BPF_FETCH in insn_has_def32()") > Link: https://lore.kernel.org/bpf/20221202103620.1915679-1-bjorn@kernel.org/ > Suggested-by: Yonghong Song <yhs@meta.com> > Signed-off-by: Björn Töpel <bjorn@rivosinc.com> Acked-by: Yonghong Song <yhs@fb.com>
Hello: This patch was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <ast@kernel.org>: On Wed, 7 Dec 2022 11:35:40 +0100 you wrote: > From: Björn Töpel <bjorn@rivosinc.com> > > In BPF all global functions, and BPF helpers return a 64-bit > value. For kfunc calls, this is not the case, and they can return > e.g. 32-bit values. > > The return register R0 for kfuncs calls can therefore be marked as > subreg_def != DEF_NOT_SUBREG. In general, if a register is marked with > subreg_def != DEF_NOT_SUBREG, some archs (where bpf_jit_needs_zext() > returns true) require the verifier to insert explicit zero-extension > instructions. > > [...] Here is the summary with links: - [bpf,v2] bpf: Do not zero-extend kfunc return values https://git.kernel.org/bpf/bpf-next/c/d35af0a7feb0 You are awesome, thank you!
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 264b3dc714cc..bdfa6619e28f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -13386,6 +13386,10 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, if (!bpf_jit_needs_zext() && !is_cmpxchg_insn(&insn)) continue; + /* Zero-extension is done by the caller. */ + if (bpf_pseudo_kfunc_call(&insn)) + continue; + if (WARN_ON(load_reg == -1)) { verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n"); return -EFAULT;