[PATCH bpf] bpf: verifier: support BPF_LOAD_ACQ in insn_def_regno()

[PATCH bpf] bpf: verifier: support BPF_LOAD_ACQ in insn_def_regno()

[Puranjay Mohan]

insn_def_regno() currently returns -1 for a BPF_LOAD_ACQ which is
incorrect as BPF_LOAD_ACQ loads a value from (src_reg + off) into the
dst_reg.

This was uncovered by syzkaller while fuzzing on arm32 architecture
where this function was being called by opt_subreg_zext_lo32_rnd_hi32()
and the warning inside this function was triggered because the
BPF_LOAD_ACQ instruction can read 32 bit values so it needs to be
zero-extended on some archs (eg. arm32) but the destination register (to
be zero-extended) returned by insn_def_regno() was invalid (-1).

Fixes: 880442305a39 ("bpf: Introduce load-acquire and store-release instructions")
Reported-by: syzbot+0ef84a7bdf5301d4cbec@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/bpf/682dd10b.a00a0220.29bc26.028e.GAE@google.com/T/#m1457e14da8cf6c1d9703b446c224407bca758f5c
Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
 kernel/bpf/verifier.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 54c6953a8b84..9aa67e46cb8b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3643,6 +3643,9 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
 /* Return the regno defined by the insn, or -1. */
 static int insn_def_regno(const struct bpf_insn *insn)
 {
+	if (is_atomic_load_insn(insn))
+		return insn->dst_reg;
+
 	switch (BPF_CLASS(insn->code)) {
 	case BPF_JMP:
 	case BPF_JMP32:
-- 
2.47.1

Re: [PATCH bpf] bpf: verifier: support BPF_LOAD_ACQ in insn_def_regno()

[Eduard Zingerman]

On 2025-05-21 11:39, Puranjay Mohan wrote:
[...]
> @@ -3643,6 +3643,9 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>   /* Return the regno defined by the insn, or -1. */
>   static int insn_def_regno(const struct bpf_insn *insn)
>   {
> +	if (is_atomic_load_insn(insn))
> +		return insn->dst_reg;
> +
>   	switch (BPF_CLASS(insn->code)) {
>   	case BPF_JMP:
>   	case BPF_JMP32:

I'm confused, is_atomic_load_insn() is defined as:

          return BPF_CLASS(insn->code) == BPF_STX &&
                 BPF_MODE(insn->code) == BPF_ATOMIC &&
                 insn->imm == BPF_LOAD_ACQ;

And insn_def_regno() has the following case:

          case BPF_STX:
                  if (BPF_MODE(insn->code) == BPF_ATOMIC ||
                      BPF_MODE(insn->code) == BPF_PROBE_ATOMIC) {
                          if (insn->imm == BPF_CMPXCHG)
                                  return BPF_REG_0;
                          else if (insn->imm == BPF_LOAD_ACQ)
                                  return insn->dst_reg;
                          else if (insn->imm & BPF_FETCH)
                                  return insn->src_reg;
                  }
                  return -1;

Why is it not triggering?

Also, can this be tested with a BPF_F_TEST_RND_HI32 flag?
E.g. see verifier_scalar_ids.c:linked_regs_and_subreg_def() test case.

Re: [PATCH bpf] bpf: verifier: support BPF_LOAD_ACQ in insn_def_regno()

[Alexei Starovoitov]

On Wed, May 21, 2025 at 12:13 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
>
> On 2025-05-21 11:39, Puranjay Mohan wrote:
> [...]
> > @@ -3643,6 +3643,9 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >   /* Return the regno defined by the insn, or -1. */
> >   static int insn_def_regno(const struct bpf_insn *insn)
> >   {
> > +     if (is_atomic_load_insn(insn))
> > +             return insn->dst_reg;
> > +
> >       switch (BPF_CLASS(insn->code)) {
> >       case BPF_JMP:
> >       case BPF_JMP32:
>
> I'm confused, is_atomic_load_insn() is defined as:
>
>           return BPF_CLASS(insn->code) == BPF_STX &&
>                  BPF_MODE(insn->code) == BPF_ATOMIC &&
>                  insn->imm == BPF_LOAD_ACQ;
>
> And insn_def_regno() has the following case:
>
>           case BPF_STX:
>                   if (BPF_MODE(insn->code) == BPF_ATOMIC ||
>                       BPF_MODE(insn->code) == BPF_PROBE_ATOMIC) {
>                           if (insn->imm == BPF_CMPXCHG)
>                                   return BPF_REG_0;
>                           else if (insn->imm == BPF_LOAD_ACQ)
>                                   return insn->dst_reg;
>                           else if (insn->imm & BPF_FETCH)
>                                   return insn->src_reg;
>                   }
>                   return -1;
>
> Why is it not triggering?
>
> Also, can this be tested with a BPF_F_TEST_RND_HI32 flag?
> E.g. see verifier_scalar_ids.c:linked_regs_and_subreg_def() test case.

I suspect it was already fixed by commit
fce7bd8e385a ("bpf/verifier: Handle BPF_LOAD_ACQ instructions in
insn_def_regno()")

Re: [PATCH bpf] bpf: verifier: support BPF_LOAD_ACQ in insn_def_regno()

[Eduard Zingerman]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

[...]

> I suspect it was already fixed by commit
> fce7bd8e385a ("bpf/verifier: Handle BPF_LOAD_ACQ instructions in
> insn_def_regno()")

I see, series [1] is not a part of the tag [2] tested by syzbot, thank you.

[1] "bpf, riscv64: Support load-acquire and store-release instructions"
    https://lore.kernel.org/all/cover.1746588351.git.yepeilin@google.com/
[2] 172a9d94339c ("Merge tag '6.15-rc6-smb3-client-fixes'")

Re: [PATCH bpf] bpf: verifier: support BPF_LOAD_ACQ in insn_def_regno()

[Puranjay Mohan]

Eduard Zingerman <eddyz87@gmail.com> writes:

> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>
> [...]
>
>> I suspect it was already fixed by commit
>> fce7bd8e385a ("bpf/verifier: Handle BPF_LOAD_ACQ instructions in
>> insn_def_regno()")
>
> I see, series [1] is not a part of the tag [2] tested by syzbot, thank you.
>
> [1] "bpf, riscv64: Support load-acquire and store-release instructions"
>     https://lore.kernel.org/all/cover.1746588351.git.yepeilin@google.com/
> [2] 172a9d94339c ("Merge tag '6.15-rc6-smb3-client-fixes'")


Yes, sorry for missing this. The fix is in bpf-next but syzkaller was
testing mainline.

Thanks,
Puranjay

Re: [PATCH bpf] bpf: verifier: support BPF_LOAD_ACQ in insn_def_regno()

[Peilin Ye]

On Wed, May 21, 2025 at 01:04:47PM -0700, Alexei Starovoitov wrote:
> On Wed, May 21, 2025 at 12:13 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > I'm confused, is_atomic_load_insn() is defined as:
> >
> >           return BPF_CLASS(insn->code) == BPF_STX &&
> >                  BPF_MODE(insn->code) == BPF_ATOMIC &&
> >                  insn->imm == BPF_LOAD_ACQ;
> >
> > And insn_def_regno() has the following case:
> >
> >           case BPF_STX:
> >                   if (BPF_MODE(insn->code) == BPF_ATOMIC ||
> >                       BPF_MODE(insn->code) == BPF_PROBE_ATOMIC) {
> >                           if (insn->imm == BPF_CMPXCHG)
> >                                   return BPF_REG_0;
> >                           else if (insn->imm == BPF_LOAD_ACQ)
> >                                   return insn->dst_reg;
> >                           else if (insn->imm & BPF_FETCH)
> >                                   return insn->src_reg;
> >                   }
> >                   return -1;
> >
> > Why is it not triggering?
> >
> > Also, can this be tested with a BPF_F_TEST_RND_HI32 flag?
> > E.g. see verifier_scalar_ids.c:linked_regs_and_subreg_def() test case.
> 
> I suspect it was already fixed by commit
> fce7bd8e385a ("bpf/verifier: Handle BPF_LOAD_ACQ instructions in
> insn_def_regno()")

Ah, right; I did it when adding support for riscv64 (which needs_zext).
I targeted bpf-next because at that time only x86_64 and arm64 (both
!needs_zext) supported BPF_LOAD_ACQ, and didn't realize this could
affect arm32 (needs_zext).

It should've targeted bpf with a Fixes: tag instead.  Sorry for any
confusion.

Thanks,
Peilin Ye