* [PATCH bpf-next] bpf: Account for BPF_FETCH in insn_has_def32()
@ 2021-02-24 14:18 Ilya Leoshkevich
2021-02-26 19:25 ` Martin KaFai Lau
0 siblings, 1 reply; 2+ messages in thread
From: Ilya Leoshkevich @ 2021-02-24 14:18 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: bpf, Heiko Carstens, Vasily Gorbik, Brendan Jackman, Ilya Leoshkevich
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(-)
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;
--
2.29.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH bpf-next] bpf: Account for BPF_FETCH in insn_has_def32()
2021-02-24 14:18 [PATCH bpf-next] bpf: Account for BPF_FETCH in insn_has_def32() Ilya Leoshkevich
@ 2021-02-26 19:25 ` Martin KaFai Lau
0 siblings, 0 replies; 2+ messages in thread
From: Martin KaFai Lau @ 2021-02-26 19:25 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Heiko Carstens,
Vasily Gorbik, Brendan Jackman
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-02-26 19:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-24 14:18 [PATCH bpf-next] bpf: Account for BPF_FETCH in insn_has_def32() Ilya Leoshkevich
2021-02-26 19:25 ` Martin KaFai Lau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).