syzbot reported out-of-bounds read in check_atomic_load/store() when
the register number is MAX_BPF_REG or greater in this context:
    https://syzkaller.appspot.com/bug?extid=a5964227adc0f904549c

To avoid the issue from now on, let's add tests where the register
number is invalid for load-acquire/store-release.

By the way, I chose R11 as an invalid register but there's no particular
insistence on this choice as long as the register number is invalid.

Signed-off-by: Kohei Enju <enjuk@amazon.com>
---
 .../selftests/bpf/progs/verifier_load_acquire.c    | 14 ++++++++++++++
 .../selftests/bpf/progs/verifier_store_release.c   | 14 ++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_load_acquire.c b/tools/testing/selftests/bpf/progs/verifier_load_acquire.c
index 1babe9ad9b43..e3912d2c6f95 100644
--- a/tools/testing/selftests/bpf/progs/verifier_load_acquire.c
+++ b/tools/testing/selftests/bpf/progs/verifier_load_acquire.c
@@ -189,6 +189,20 @@ __naked void load_acquire_from_sock_pointer(void)
 	: __clobber_all);
 }
 
+SEC("socket")
+__description("load-acquire with invalid register R11")
+__failure __failure_unpriv __msg("R11 is invalid")
+__naked void load_acquire_with_invalid_reg(void)
+{
+	asm volatile (
+	".8byte %[load_acquire_insn];" // r0 = load_acquire((u64 *)(r11 + 0));
+	"exit;"
+	:
+	: __imm_insn(load_acquire_insn,
+		     BPF_ATOMIC_OP(BPF_DW, BPF_LOAD_ACQ, BPF_REG_0, 11 /* invalid reg */, 0))
+	: __clobber_all);
+}
+
 #else /* CAN_USE_LOAD_ACQ_STORE_REL */
 
 SEC("socket")
diff --git a/tools/testing/selftests/bpf/progs/verifier_store_release.c b/tools/testing/selftests/bpf/progs/verifier_store_release.c
index cd6f1e5f378b..2dc1d713b4a6 100644
--- a/tools/testing/selftests/bpf/progs/verifier_store_release.c
+++ b/tools/testing/selftests/bpf/progs/verifier_store_release.c
@@ -257,6 +257,20 @@ __naked void store_release_leak_pointer_to_map(void)
 	: __clobber_all);
 }
 
+SEC("socket")
+__description("store-release with invalid register R11")
+__failure __failure_unpriv __msg("R11 is invalid")
+__naked void store_release_with_invalid_reg(void)
+{
+	asm volatile (
+	".8byte %[store_release_insn];" // store_release((u64 *)(r11 + 0), r1);
+	"exit;"
+	:
+	: __imm_insn(store_release_insn,
+		     BPF_ATOMIC_OP(BPF_DW, BPF_STORE_REL, 11 /* invalid reg */, BPF_REG_1, 0))
+	: __clobber_all);
+}
+
 #else
 
 SEC("socket")
-- 
2.48.1

On Fri, 2025-03-21 at 19:59 +0900, Kohei Enju wrote:

Hi Kohei,

Thank you for adding these tests.

[...]

> +SEC("socket")
> +__description("load-acquire with invalid register R11")
> +__failure __failure_unpriv __msg("R11 is invalid")
> +__naked void load_acquire_with_invalid_reg(void)
> +{
> +	asm volatile (
> +	".8byte %[load_acquire_insn];" // r0 = load_acquire((u64 *)(r11 + 0));
> +	"exit;"
> +	:
> +	: __imm_insn(load_acquire_insn,
> +		     BPF_ATOMIC_OP(BPF_DW, BPF_LOAD_ACQ, BPF_REG_0, 11 /* invalid reg */, 0))
> +	: __clobber_all);
> +}
> +
>  #else /* CAN_USE_LOAD_ACQ_STORE_REL */
>  
>  SEC("socket")
> diff --git a/tools/testing/selftests/bpf/progs/verifier_store_release.c b/tools/testing/selftests/bpf/progs/verifier_store_release.c
> index cd6f1e5f378b..2dc1d713b4a6 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_store_release.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_store_release.c
> @@ -257,6 +257,20 @@ __naked void store_release_leak_pointer_to_map(void)
>  	: __clobber_all);
>  }
>  
> +SEC("socket")
> +__description("store-release with invalid register R11")
> +__failure __failure_unpriv __msg("R11 is invalid")
> +__naked void store_release_with_invalid_reg(void)
> +{
> +	asm volatile (
> +	".8byte %[store_release_insn];" // store_release((u64 *)(r11 + 0), r1);
> +	"exit;"
> +	:
> +	: __imm_insn(store_release_insn,
> +		     BPF_ATOMIC_OP(BPF_DW, BPF_STORE_REL, 11 /* invalid reg */, BPF_REG_1, 0))

On my machine / config, the value of 11 was too small to trigger the
KASAN warning. Value of 12 was sufficient.
Curious if it is my config, did you see KASAN warning locally when running this test
before applying the fix?
Maybe set the value to 15 here and above to maximize probability of KASAN warning?

> +	: __clobber_all);
> +}
> +
>  #else
>  
>  SEC("socket")

> [...]
> 
> > +SEC("socket")
> > +__description("load-acquire with invalid register R11")
> > +__failure __failure_unpriv __msg("R11 is invalid")
> > +__naked void load_acquire_with_invalid_reg(void)
> > +{
> > +	asm volatile (
> > +	".8byte %[load_acquire_insn];" // r0 = load_acquire((u64 *)(r11 + 0));
> > +	"exit;"
> > +	:
> > +	: __imm_insn(load_acquire_insn,
> > +		     BPF_ATOMIC_OP(BPF_DW, BPF_LOAD_ACQ, BPF_REG_0, 11 /* invalid reg */, 0))
> > +	: __clobber_all);
> > +}
> > +
> >  #else /* CAN_USE_LOAD_ACQ_STORE_REL */
> >  
> >  SEC("socket")
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_store_release.c b/tools/testing/selftests/bpf/progs/verifier_store_release.c
> > index cd6f1e5f378b..2dc1d713b4a6 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_store_release.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_store_release.c
> > @@ -257,6 +257,20 @@ __naked void store_release_leak_pointer_to_map(void)
> >  	: __clobber_all);
> >  }
> >  
> > +SEC("socket")
> > +__description("store-release with invalid register R11")
> > +__failure __failure_unpriv __msg("R11 is invalid")
> > +__naked void store_release_with_invalid_reg(void)
> > +{
> > +	asm volatile (
> > +	".8byte %[store_release_insn];" // store_release((u64 *)(r11 + 0), r1);
> > +	"exit;"
> > +	:
> > +	: __imm_insn(store_release_insn,
> > +		     BPF_ATOMIC_OP(BPF_DW, BPF_STORE_REL, 11 /* invalid reg */, BPF_REG_1, 0))
> 
> On my machine / config, the value of 11 was too small to trigger the
> KASAN warning. Value of 12 was sufficient.
> Curious if it is my config, did you see KASAN warning locally when running this test
> before applying the fix?

Yes, as you pointed out, R11 doesn't trigger the KASAN splat in practice. 
For the splat, we need a value of 12 or larger.

The sizes of struct bpf_reg_state and bpf_func_state are 120 and 1368 
respectively.[1]
In the bpf_func_state, the member `regs` ranges from 0 to 1320 bytes (each 
120 bytes for each R0 to R10).
Also, the member `type`, which is accessed in is_ctx_reg(), is the first 
member of struct bpf_reg_state.

Therefore, when the register is R11, `regs->type` reads 4 bytes from 1320.
Since the size of bpf_func_state is 1368 and it doesn't exceed the end of 
the allocated memory, it doesn't trigger the KASAN splat.

OTOH, when the register is R12, `regs->type` reads 4 bytes from 1440 (120 
* 12 + 0).
This triggers the KASAN splat since it's larger than bpf_func_state's size.

Here is a part of the splat I saw in my environment when specifying R12. 
This says that the buggy address is 1440 (1368 + 72) and also matches 
previous analysis.

    The buggy address belongs to the object at ffff888112603800
     which belongs to the cache kmalloc-2k of size 2048
    The buggy address is located 72 bytes to the right of
     allocated 1368-byte region [ffff888112603800, ffff888112603d58)
    ...
    Memory state around the buggy address:
     ffff888112603c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
     ffff888112603d00: 00 00 00 00 00 00 00 00 00 00 00 fc fc fc fc fc
    >ffff888112603d80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
                                   ^
     ffff888112603e00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
     ffff888112603e80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc

> Maybe set the value to 15 here and above to maximize probability of KASAN warning?

Understood. Thank you for the feedback.

I chose the minimum invalid register regardless of the actual occurrence 
of the splat, since the validity check of this type might be `regno >= 
MAX_BPF_REG` or not.
Sorry for my confusing choice.

Since I'm not attached to that particular choice, I'll change it to R15.
Thank you for reviewing and providing feedback!

> 
> > +	: __clobber_all);
> > +}
> > +
> >  #else
> >  
> >  SEC("socket")

Regards,
Kohei

---
[1]
struct bpf_reg_state {
        enum bpf_reg_type          type;                 /*     0     4 */
...

        /* size: 120, cachelines: 2, members: 19 */
        /* padding: 3 */
        /* last cacheline: 56 bytes */
};

struct bpf_func_state {
        struct bpf_reg_state       regs[11];             /*     0  1320 */
...
        int                        allocated_stack;      /*  1360     4 */

        /* size: 1368, cachelines: 22, members: 12 */
        /* sum members: 1363, holes: 1, sum holes: 1 */
        /* padding: 4 */
        /* last cacheline: 24 bytes */
};

On Sat, 2025-03-22 at 11:48 +0900, Kohei Enju wrote:

[...]

> I chose the minimum invalid register regardless of the actual occurrence 
> of the splat, since the validity check of this type might be `regno >= 
> MAX_BPF_REG` or not.
> Sorry for my confusing choice.
> 
> Since I'm not attached to that particular choice, I'll change it to R15.
> Thank you for reviewing and providing feedback!

Hi Kohei,

Thank you for detailed explanation.
Please add 'Acked-by: Eduard Zingerman <eddyz87@gmail.com>'
for the next revision.

Thanks,
Eduard

[...]