| Message ID | 52c7df55-84d9-f6d2-ed84-51ac90eb6bcc@solarflare.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | Bug with BPF_ALU64 | BPF_END? | expand |
From: Edward Cree <ecree@solarflare.com> Date: Thu, 14 Sep 2017 18:53:17 +0100 > Is BPF_END supposed to only be used with BPF_ALU, never with BPF_ALU64? > In kernel/bpf/core.c:___bpf_prog_run(), there are only jump table targets > for the BPF_ALU case, not for the BPF_ALU64 case (opcodes 0xd7 and 0xdf). > But the verifier doesn't enforce this; by crafting a program that uses > these opcodes I can get a WARN when they're run (without JIT; it looks > like the x86 JIT, at least, won't like it either). > Proposed patch below the cut; build-tested only. Good catch. A really neat test would be a program that uploads random BPF programs into the kernel, in a syzkaller'ish way. It might have triggered this (eventually).
On Thu, Sep 14, 2017 at 11:14 AM, David Miller <davem@davemloft.net> wrote: > From: Edward Cree <ecree@solarflare.com> > Date: Thu, 14 Sep 2017 18:53:17 +0100 > >> Is BPF_END supposed to only be used with BPF_ALU, never with BPF_ALU64? Yes, only BPF_ALU. The below is LLVM bpf swap insn encoding: ... // bswap16, bswap32, bswap64 class BSWAP ... ... let op = 0xd; // BPF_END let BPFSrc = 1; // BPF_TO_BE (TODO: use BPF_TO_LE for big-endian target) let BPFClass = 4; // BPF_ALU ... >> In kernel/bpf/core.c:___bpf_prog_run(), there are only jump table targets >> for the BPF_ALU case, not for the BPF_ALU64 case (opcodes 0xd7 and 0xdf). >> But the verifier doesn't enforce this; by crafting a program that uses >> these opcodes I can get a WARN when they're run (without JIT; it looks >> like the x86 JIT, at least, won't like it either). >> Proposed patch below the cut; build-tested only. > > Good catch. > > A really neat test would be a program that uploads random BPF programs > into the kernel, in a syzkaller'ish way. It might have triggered this > (eventually). >
On 09/14/2017 07:53 PM, Edward Cree wrote: > Is BPF_END supposed to only be used with BPF_ALU, never with BPF_ALU64? > In kernel/bpf/core.c:___bpf_prog_run(), there are only jump table targets > for the BPF_ALU case, not for the BPF_ALU64 case (opcodes 0xd7 and 0xdf). > But the verifier doesn't enforce this; by crafting a program that uses > these opcodes I can get a WARN when they're run (without JIT; it looks > like the x86 JIT, at least, won't like it either). > Proposed patch below the cut; build-tested only. > > -Ed > --- > > [PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END > > Neither ___bpf_prog_run nor the JITs accept it. > > Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)") > Signed-off-by: Edward Cree <ecree@solarflare.com> Good catch! Can you submit this as an official patch for -net together with a test case for tools/testing/selftests/bpf/test_verifier.c? Thanks! Acked-by: Daniel Borkmann <daniel@iogearbox.net> > --- > kernel/bpf/verifier.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 477b693..799b245 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -2292,7 +2292,8 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn) > } > } else { > if (insn->src_reg != BPF_REG_0 || insn->off != 0 || > - (insn->imm != 16 && insn->imm != 32 && insn->imm != 64)) { > + (insn->imm != 16 && insn->imm != 32 && insn->imm != 64) || > + BPF_CLASS(insn->code) == BPF_ALU64) { > verbose("BPF_END uses reserved fields\n"); > return -EINVAL; > } >
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 477b693..799b245 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2292,7 +2292,8 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn) } } else { if (insn->src_reg != BPF_REG_0 || insn->off != 0 || - (insn->imm != 16 && insn->imm != 32 && insn->imm != 64)) { + (insn->imm != 16 && insn->imm != 32 && insn->imm != 64) || + BPF_CLASS(insn->code) == BPF_ALU64) { verbose("BPF_END uses reserved fields\n"); return -EINVAL; }
Is BPF_END supposed to only be used with BPF_ALU, never with BPF_ALU64? In kernel/bpf/core.c:___bpf_prog_run(), there are only jump table targets for the BPF_ALU case, not for the BPF_ALU64 case (opcodes 0xd7 and 0xdf). But the verifier doesn't enforce this; by crafting a program that uses these opcodes I can get a WARN when they're run (without JIT; it looks like the x86 JIT, at least, won't like it either). Proposed patch below the cut; build-tested only. -Ed --- [PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END Neither ___bpf_prog_run nor the JITs accept it. Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)") Signed-off-by: Edward Cree <ecree@solarflare.com> --- kernel/bpf/verifier.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)