Date: Thu, 17 Jun 2021 12:09:36 +0200 From: Daniel Borkmann <daniel@...earbox.net> To: Eric Biggers <ebiggers@...nel.org> Cc: Edward Cree <ecree.xilinx@...il.com>, Kurt Manucredo <fuzzybritches0@...il.com>, syzbot+bed360704c521841c85d@...kaller.appspotmail.com, keescook@...omium.org, yhs@...com, dvyukov@...gle.com, andrii@...nel.org, ast@...nel.org, bpf@...r.kernel.org, davem@...emloft.net, hawk@...nel.org, john.fastabend@...il.com, kafai@...com, kpsingh@...nel.org, kuba@...nel.org, linux-kernel@...r.kernel.org, netdev@...r.kernel.org, songliubraving@...com, syzkaller-bugs@...glegroups.com, nathan@...nel.org, ndesaulniers@...gle.com, clang-built-linux@...glegroups.com, kernel-hardening@...ts.openwall.com, kasan-dev@...glegroups.com Subject: Re: [PATCH v5] bpf: core: fix shift-out-of-bounds in ___bpf_prog_run On 6/16/21 12:07 AM, Eric Biggers wrote: > On Tue, Jun 15, 2021 at 11:54:41PM +0200, Daniel Borkmann wrote: >> On 6/15/21 11:38 PM, Eric Biggers wrote: >>> On Tue, Jun 15, 2021 at 02:32:18PM -0700, Eric Biggers wrote: >>>> On Tue, Jun 15, 2021 at 11:08:18PM +0200, Daniel Borkmann wrote: >>>>> On 6/15/21 9:33 PM, Eric Biggers wrote: >>>>>> On Tue, Jun 15, 2021 at 07:51:07PM +0100, Edward Cree wrote: >>>>>>> >>>>>>> As I understand it, the UBSAN report is coming from the eBPF interpreter, >>>>>>> which is the *slow path* and indeed on many production systems is >>>>>>> compiled out for hardening reasons (CONFIG_BPF_JIT_ALWAYS_ON). >>>>>>> Perhaps a better approach to the fix would be to change the interpreter >>>>>>> to compute "DST = DST << (SRC & 63);" (and similar for other shifts and >>>>>>> bitnesses), thus matching the behaviour of most chips' shift opcodes. >>>>>>> This would shut up UBSAN, without affecting JIT code generation. >>>>>> >>>>>> Yes, I suggested that last week >>>>>> (https://lkml.kernel.org/netdev/YMJvbGEz0xu9JU9D@gmail.com). The AND will even >>>>>> get optimized out when compiling for most CPUs. >>>>> >>>>> Did you check if the generated interpreter code for e.g. x86 is the same >>>>> before/after with that? >>>> >>>> Yes, on x86_64 with gcc 10.2.1, the disassembly of ___bpf_prog_run() is the same >>>> both before and after (with UBSAN disabled). Here is the patch I used: >>>> >>>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c >>>> index 5e31ee9f7512..996db8a1bbfb 100644 >>>> --- a/kernel/bpf/core.c >>>> +++ b/kernel/bpf/core.c >>>> @@ -1407,12 +1407,30 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn) >>>> DST = (u32) DST OP (u32) IMM; \ >>>> CONT; >>>> + /* >>>> + * Explicitly mask the shift amounts with 63 or 31 to avoid undefined >>>> + * behavior. Normally this won't affect the generated code. >> >> The last one should probably be more specific in terms of 'normally', e.g. that >> it is expected that the compiler is optimizing this away for archs like x86. Is >> arm64 also covered by this ... do you happen to know on which archs this won't >> be the case? >> >> Additionally, I think such comment should probably be more clear in that it also >> needs to give proper guidance to JIT authors that look at the interpreter code to >> see what they need to implement, in other words, that they don't end up copying >> an explicit AND instruction emission if not needed there. > > Same result on arm64 with gcc 10.2.0. > > On arm32 it is different, probably because the 64-bit shifts aren't native in > that case. I don't know about other architectures. But there aren't many ways > to implement shifts, and using just the low bits of the shift amount is the most > logical way. > > Please feel free to send out a patch with whatever comment you want. The diff I > gave was just an example and I am not an expert in BPF. > >> >>>> + */ >>>> +#define ALU_SHIFT(OPCODE, OP) \ >>>> + ALU64_##OPCODE##_X: \ >>>> + DST = DST OP (SRC & 63);\ >>>> + CONT; \ >>>> + ALU_##OPCODE##_X: \ >>>> + DST = (u32) DST OP ((u32)SRC & 31); \ >>>> + CONT; \ >>>> + ALU64_##OPCODE##_K: \ >>>> + DST = DST OP (IMM & 63); \ >>>> + CONT; \ >>>> + ALU_##OPCODE##_K: \ >>>> + DST = (u32) DST OP ((u32)IMM & 31); \ >>>> + CONT; >> >> For the *_K cases these are explicitly rejected by the verifier already. Is this >> required here nevertheless to suppress UBSAN false positive? > > No, I just didn't know that these constants are never out of range. Please feel > free to send out a patch that does this properly. Summarized and fixed via: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=28131e9d933339a92f78e7ab6429f4aaaa07061c Thanks everyone, Daniel
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.