lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 6 Dec 2022 10:09:20 -0800 From: Yonghong Song <yhs@...a.com> To: Ilya Leoshkevich <iii@...ux.ibm.com>, Björn Töpel <bjorn@...nel.org>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, John Fastabend <john.fastabend@...il.com>, bpf@...r.kernel.org, netdev@...r.kernel.org Cc: Björn Töpel <bjorn@...osinc.com>, Brendan Jackman <jackmanb@...gle.com> Subject: Re: [PATCH bpf] bpf: Proper R0 zero-extension for BPF_CALL instructions On 12/6/22 9:47 AM, Yonghong Song wrote: > > > On 12/6/22 5:21 AM, Ilya Leoshkevich wrote: >> On Fri, 2022-12-02 at 11:36 +0100, Björn Töpel wrote: >>> From: Björn Töpel <bjorn@...osinc.com> >>> >>> A BPF call instruction can be, correctly, marked with zext_dst set to >>> true. An example of this can be found in the BPF selftests >>> progs/bpf_cubic.c: >>> >>> ... >>> extern __u32 tcp_reno_undo_cwnd(struct sock *sk) __ksym; >>> >>> __u32 BPF_STRUCT_OPS(bpf_cubic_undo_cwnd, struct sock *sk) >>> { >>> return tcp_reno_undo_cwnd(sk); >>> } >>> ... >>> >>> which compiles to: >>> 0: r1 = *(u64 *)(r1 + 0x0) >>> 1: call -0x1 >>> 2: exit >>> >>> The call will be marked as zext_dst set to true, and for some >>> backends >>> (bpf_jit_needs_zext() returns true) expanded to: >>> 0: r1 = *(u64 *)(r1 + 0x0) >>> 1: call -0x1 >>> 2: w0 = w0 >>> 3: exit >> >> In the verifier, the marking is done by check_kfunc_call() (added in >> e6ac2450d6de), right? So the problem occurs only for kfuncs? >> >> /* Check return type */ >> t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL); >> >> ... >> >> if (btf_type_is_scalar(t)) { >> mark_reg_unknown(env, regs, BPF_REG_0); >> mark_btf_func_reg_size(env, BPF_REG_0, t->size); >> >> I tried to find some official information whether the eBPF calling >> convention requires sign- or zero- extending return values and >> arguments, but unfortunately [1] doesn't mention this. >> >> LLVM's lib/Target/BPF/BPFCallingConv.td mentions both R* and W* >> registers, but since assigning to W* leads to zero-extension, it seems >> to me that this is the case. > > We actually follow the clang convention, the zero-extension is either > done in caller or callee, but not both. See > https://reviews.llvm.org/D131598 how the convention could be changed. > > The following is an example. > > $ 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 actually used in the bpf program, the > proper zero extension will be done. Otherwise, it is not done. > > This is with latest llvm16. I guess we need to check llvm whether > we could enforce to add a w0 = w0 in bar1(). > > Otherwise, with this patch, it will add w0 = w0 in all cases which > is not necessary in most of practical cases. > >> >> If the above is correct, then shouldn't we rather use sizeof(void *) in >> the mark_btf_func_reg_size() call above? >> >>> The opt_subreg_zext_lo32_rnd_hi32() function which is responsible for >>> the zext patching, relies on insn_def_regno() to fetch the register >>> to >>> zero-extend. However, this function does not handle call instructions >>> correctly, and opt_subreg_zext_lo32_rnd_hi32() fails the >>> verification. >>> >>> Make sure that R0 is correctly resolved for (BPF_JMP | BPF_CALL) >>> instructions. >>> >>> Fixes: 83a2881903f3 ("bpf: Account for BPF_FETCH in >>> insn_has_def32()") >>> Signed-off-by: Björn Töpel <bjorn@...osinc.com> >>> --- >>> I'm not super happy about the additional special case -- first >>> cmpxchg, and now call. :-( A more elegant/generic solution is >>> welcome! >>> --- >>> kernel/bpf/verifier.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>> index 264b3dc714cc..4f9660eafc72 100644 >>> --- a/kernel/bpf/verifier.c >>> +++ b/kernel/bpf/verifier.c >>> @@ -13386,6 +13386,9 @@ static int >>> opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, >>> if (!bpf_jit_needs_zext() && !is_cmpxchg_insn(&insn)) >>> continue; >>> + if (insn.code == (BPF_JMP | BPF_CALL)) >>> + load_reg = BPF_REG_0; Want to double check. Do we actually have a problem here? For example, on x64, we probably won't have this issue. >>> ... >>> extern __u32 tcp_reno_undo_cwnd(struct sock *sk) __ksym; >>> >>> __u32 BPF_STRUCT_OPS(bpf_cubic_undo_cwnd, struct sock *sk) >>> { >>> return tcp_reno_undo_cwnd(sk); >>> } The native code will return a 32-bit subreg to bpf program, and bpf didn't do anything and return r0 to the kernel func. In the kernel func, the kernel will take 32-bit subreg by x86_64 convention. This applies to some other return types like u8/s8/u16/s16/u32/s32. Which architecture you actually see the issue? >>> + >>> if (WARN_ON(load_reg == -1)) { >>> verbose(env, "verifier bug. zext_dst is set, >>> but no reg is defined\n"); >>> return -EFAULT; >>> >>> base-commit: 01f856ae6d0ca5ad0505b79bf2d22d7ca439b2a1 >> >> [1] >> https://docs.kernel.org/bpf/instruction-set.html#registers-and-calling-convention
Powered by blists - more mailing lists