Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add selftests for load-acquire/store-release when register number is invalid

[Kohei Enju <enjuk@amazon.com>]

> [...]
> 
> > +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 */
};