| Message ID | 20210224141837.104654-1-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: Account for BPF_FETCH in insn_has_def32() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: netdev@vger.kernel.org kpsingh@kernel.org songliubraving@fb.com yhs@fb.com kafai@fb.com john.fastabend@gmail.com andrii@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 17 this patch: 17 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: Block comments use a trailing */ on a separate line |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 17 this patch: 17 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, Feb 24, 2021 at 03:18:37PM +0100, Ilya Leoshkevich wrote: > insn_has_def32() returns false for 32-bit BPF_FETCH insns. This makes > adjust_insn_aux_data() incorrectly set zext_dst, as can be seen in [1]. > This happens because insn_no_def() does not know about BPF_FETCH > variants of BPF_STX. > > Fix in two steps. > > First, replace insn_no_def() with insn_def_regno(), which returns which > the register an insn defines. Normally insn_no_def() calls are followed > by insn->dst_reg uses; replace those with insn_def_regno() return > value. > > Second, adjust the BPF_STX special case in is_reg64() to deal with > queries made from opt_subreg_zext_lo32_rnd_hi32(), where the state > information is no longer available. Add a comment, since the purpose > of this special case is not clear at the first glance. Thanks for the fix. A few nits. > > [1] https://lore.kernel.org/bpf/20210223150845.1857620-1-jackmanb@google.com/ > > Fixes: 37086bfdc737 ("bpf: Propagate stack bounds to registers in atomics w/ BPF_FETCH") Is it fixing this instead? 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg") and bpf instead of bpf-next? > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > kernel/bpf/verifier.c | 65 ++++++++++++++++++++++--------------------- > 1 file changed, 33 insertions(+), 32 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 1dda9d81f12c..f4df805d6bfd 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -1703,7 +1703,10 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn, > } > > if (class == BPF_STX) { > - if (reg->type != SCALAR_VALUE) > + /* BPF_STX (including atomic variants) has multiple source > + * operands, one of which is a ptr. Check whether the caller is > + * asking about it. */ nit. A newline for "*/". > + if (t == SRC_OP && reg->type != SCALAR_VALUE) > return true; > return BPF_SIZE(code) == BPF_DW; > } > @@ -1735,22 +1738,33 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn, > return true; > } > > -/* Return TRUE if INSN doesn't have explicit value define. */ > -static bool insn_no_def(struct bpf_insn *insn) > +/* Return the regno defined by the insn, or -1. */ > +static int insn_def_regno(const struct bpf_insn *insn) > { > - u8 class = BPF_CLASS(insn->code); > - > - return (class == BPF_JMP || class == BPF_JMP32 || > - class == BPF_STX || class == BPF_ST); > + switch (BPF_CLASS(insn->code)) { > + case BPF_JMP: > + case BPF_JMP32: > + case BPF_ST: > + return -1; > + case BPF_STX: > + return (BPF_MODE(insn->code) == BPF_ATOMIC && > + (insn->imm & BPF_FETCH)) ? > + (insn->imm == BPF_CMPXCHG ? BPF_REG_0 : insn->src_reg) : > + -1; A bit hard to read with multiple "?" stacking on each other. How about using a more verbose "if else" here? > + default: > + return insn->dst_reg; > + } > } > > /* Return TRUE if INSN has defined any 32-bit value explicitly. */ > static bool insn_has_def32(struct bpf_verifier_env *env, struct bpf_insn *insn) > { > - if (insn_no_def(insn)) > + int dst_reg = insn_def_regno(insn); > + > + if (dst_reg == -1) > return false; > > - return !is_reg64(env, insn, insn->dst_reg, NULL, DST_OP); > + return !is_reg64(env, insn, dst_reg, NULL, DST_OP); > } > > static void mark_insn_zext(struct bpf_verifier_env *env, > @@ -11006,9 +11020,10 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > for (i = 0; i < len; i++) { > int adj_idx = i + delta; > struct bpf_insn insn; > - u8 load_reg; > + int load_reg; > > insn = insns[adj_idx]; > + load_reg = insn_def_regno(&insn); > if (!aux[adj_idx].zext_dst) { > u8 code, class; > u32 imm_rnd; > @@ -11018,14 +11033,14 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > > code = insn.code; > class = BPF_CLASS(code); > - if (insn_no_def(&insn)) > + if (load_reg == -1) > continue; > > /* NOTE: arg "reg" (the fourth one) is only used for > - * BPF_STX which has been ruled out in above > - * check, it is safe to pass NULL here. > + * BPF_STX + SRC_OP, so it is safe to pass NULL > + * here. > */ > - if (is_reg64(env, &insn, insn.dst_reg, NULL, DST_OP)) { > + if (is_reg64(env, &insn, load_reg, NULL, DST_OP)) { > if (class == BPF_LD && > BPF_MODE(code) == BPF_IMM) > i++; > @@ -11040,7 +11055,7 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > imm_rnd = get_random_int(); > rnd_hi32_patch[0] = insn; > rnd_hi32_patch[1].imm = imm_rnd; > - rnd_hi32_patch[3].dst_reg = insn.dst_reg; > + rnd_hi32_patch[3].dst_reg = load_reg; > patch = rnd_hi32_patch; > patch_len = 4; > goto apply_patch_buffer; > @@ -11049,23 +11064,9 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > if (!bpf_jit_needs_zext()) > continue; > > - /* zext_dst means that we want to zero-extend whatever register > - * the insn defines, which is dst_reg most of the time, with > - * the notable exception of BPF_STX + BPF_ATOMIC + BPF_FETCH. > - */ > - if (BPF_CLASS(insn.code) == BPF_STX && > - BPF_MODE(insn.code) == BPF_ATOMIC) { > - /* BPF_STX + BPF_ATOMIC insns without BPF_FETCH do not > - * define any registers, therefore zext_dst cannot be > - * set. > - */ > - if (WARN_ON(!(insn.imm & BPF_FETCH))) > - return -EINVAL; > - load_reg = insn.imm == BPF_CMPXCHG ? BPF_REG_0 > - : insn.src_reg; > - } else { > - load_reg = insn.dst_reg; > - } > + /* If no register is defined, zext_dst cannot be set. */ > + if (WARN_ON(load_reg == -1)) May be a WARN_ON_ONCE and then followed by verbose(env, ...). > + return -EINVAL; -EFAULT instead. It is what most other places return also during verifier internal error. There are a few places return -EINVAL and probably we should change them in the future. > > zext_patch[0] = insn; > zext_patch[1].dst_reg = load_reg; > -- > 2.29.2 >
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 1dda9d81f12c..f4df805d6bfd 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1703,7 +1703,10 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn, } if (class == BPF_STX) { - if (reg->type != SCALAR_VALUE) + /* BPF_STX (including atomic variants) has multiple source + * operands, one of which is a ptr. Check whether the caller is + * asking about it. */ + if (t == SRC_OP && reg->type != SCALAR_VALUE) return true; return BPF_SIZE(code) == BPF_DW; } @@ -1735,22 +1738,33 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn, return true; } -/* Return TRUE if INSN doesn't have explicit value define. */ -static bool insn_no_def(struct bpf_insn *insn) +/* Return the regno defined by the insn, or -1. */ +static int insn_def_regno(const struct bpf_insn *insn) { - u8 class = BPF_CLASS(insn->code); - - return (class == BPF_JMP || class == BPF_JMP32 || - class == BPF_STX || class == BPF_ST); + switch (BPF_CLASS(insn->code)) { + case BPF_JMP: + case BPF_JMP32: + case BPF_ST: + return -1; + case BPF_STX: + return (BPF_MODE(insn->code) == BPF_ATOMIC && + (insn->imm & BPF_FETCH)) ? + (insn->imm == BPF_CMPXCHG ? BPF_REG_0 : insn->src_reg) : + -1; + default: + return insn->dst_reg; + } } /* Return TRUE if INSN has defined any 32-bit value explicitly. */ static bool insn_has_def32(struct bpf_verifier_env *env, struct bpf_insn *insn) { - if (insn_no_def(insn)) + int dst_reg = insn_def_regno(insn); + + if (dst_reg == -1) return false; - return !is_reg64(env, insn, insn->dst_reg, NULL, DST_OP); + return !is_reg64(env, insn, dst_reg, NULL, DST_OP); } static void mark_insn_zext(struct bpf_verifier_env *env, @@ -11006,9 +11020,10 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, for (i = 0; i < len; i++) { int adj_idx = i + delta; struct bpf_insn insn; - u8 load_reg; + int load_reg; insn = insns[adj_idx]; + load_reg = insn_def_regno(&insn); if (!aux[adj_idx].zext_dst) { u8 code, class; u32 imm_rnd; @@ -11018,14 +11033,14 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, code = insn.code; class = BPF_CLASS(code); - if (insn_no_def(&insn)) + if (load_reg == -1) continue; /* NOTE: arg "reg" (the fourth one) is only used for - * BPF_STX which has been ruled out in above - * check, it is safe to pass NULL here. + * BPF_STX + SRC_OP, so it is safe to pass NULL + * here. */ - if (is_reg64(env, &insn, insn.dst_reg, NULL, DST_OP)) { + if (is_reg64(env, &insn, load_reg, NULL, DST_OP)) { if (class == BPF_LD && BPF_MODE(code) == BPF_IMM) i++; @@ -11040,7 +11055,7 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, imm_rnd = get_random_int(); rnd_hi32_patch[0] = insn; rnd_hi32_patch[1].imm = imm_rnd; - rnd_hi32_patch[3].dst_reg = insn.dst_reg; + rnd_hi32_patch[3].dst_reg = load_reg; patch = rnd_hi32_patch; patch_len = 4; goto apply_patch_buffer; @@ -11049,23 +11064,9 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, if (!bpf_jit_needs_zext()) continue; - /* zext_dst means that we want to zero-extend whatever register - * the insn defines, which is dst_reg most of the time, with - * the notable exception of BPF_STX + BPF_ATOMIC + BPF_FETCH. - */ - if (BPF_CLASS(insn.code) == BPF_STX && - BPF_MODE(insn.code) == BPF_ATOMIC) { - /* BPF_STX + BPF_ATOMIC insns without BPF_FETCH do not - * define any registers, therefore zext_dst cannot be - * set. - */ - if (WARN_ON(!(insn.imm & BPF_FETCH))) - return -EINVAL; - load_reg = insn.imm == BPF_CMPXCHG ? BPF_REG_0 - : insn.src_reg; - } else { - load_reg = insn.dst_reg; - } + /* If no register is defined, zext_dst cannot be set. */ + if (WARN_ON(load_reg == -1)) + return -EINVAL; zext_patch[0] = insn; zext_patch[1].dst_reg = load_reg;
insn_has_def32() returns false for 32-bit BPF_FETCH insns. This makes adjust_insn_aux_data() incorrectly set zext_dst, as can be seen in [1]. This happens because insn_no_def() does not know about BPF_FETCH variants of BPF_STX. Fix in two steps. First, replace insn_no_def() with insn_def_regno(), which returns which the register an insn defines. Normally insn_no_def() calls are followed by insn->dst_reg uses; replace those with insn_def_regno() return value. Second, adjust the BPF_STX special case in is_reg64() to deal with queries made from opt_subreg_zext_lo32_rnd_hi32(), where the state information is no longer available. Add a comment, since the purpose of this special case is not clear at the first glance. [1] https://lore.kernel.org/bpf/20210223150845.1857620-1-jackmanb@google.com/ Fixes: 37086bfdc737 ("bpf: Propagate stack bounds to registers in atomics w/ BPF_FETCH") Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- kernel/bpf/verifier.c | 65 ++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 32 deletions(-)