kernel/bpf/verifier.c | 3 +++ 1 file changed, 3 insertions(+)
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
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.
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()")
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
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'")
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
© 2016 - 2025 Red Hat, Inc.