Re: [PATCH v5] bpf: core: fix shift-out-of-bounds in ___bpf_prog_run

From: Eric Biggers <ebiggers@kernel.org>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Edward Cree <ecree.xilinx@gmail.com>,
	Kurt Manucredo <fuzzybritches0@gmail.com>,
	syzbot+bed360704c521841c85d@syzkaller.appspotmail.com,
	keescook@chromium.org, yhs@fb.com, dvyukov@google.com,
	andrii@kernel.org, ast@kernel.org, bpf@vger.kernel.org,
	davem@davemloft.net, hawk@kernel.org, john.fastabend@gmail.com,
	kafai@fb.com, kpsingh@kernel.org, kuba@kernel.org,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	songliubraving@fb.com, syzkaller-bugs@googlegroups.com,
	nathan@kernel.org, ndesaulniers@google.com,
	clang-built-linux@googlegroups.com,
	kernel-hardening@lists.openwall.com, kasan-dev@googlegroups.com
Subject: Re: [PATCH v5] bpf: core: fix shift-out-of-bounds in ___bpf_prog_run
Date: Tue, 15 Jun 2021 14:38:15 -0700	[thread overview]
Message-ID: <YMkdx1VB0i+fhjAY@gmail.com> (raw)
In-Reply-To: <YMkcYn4dyZBY/ze+@gmail.com>

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.
> +	 */
> +#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;
> +
>  	ALU(ADD,  +)
>  	ALU(SUB,  -)
>  	ALU(AND,  &)
>  	ALU(OR,   |)
> -	ALU(LSH, <<)
> -	ALU(RSH, >>)
> +	ALU_SHIFT(LSH, <<)
> +	ALU_SHIFT(RSH, >>)
>  	ALU(XOR,  ^)
>  	ALU(MUL,  *)
>  #undef ALU
> 

Note, I missed the arithmetic right shifts later on in the function.  Same
result there, though.

- Eric