On Mon, Apr 18, 2022 at 4:57 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Tue, Apr 19, 2022 at 03:50:38AM IST, Joanne Koong wrote:
> > ()On Sat, Apr 16, 2022 at 10:42 AM Kumar Kartikeya Dwivedi
> > <memxor@xxxxxxxxx> wrote:
> > > [...]
> > > >
> > > > if (arg_type == ARG_CONST_MAP_PTR) {
> > > > @@ -5565,6 +5814,44 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > > > bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
> > > >
> > > > err = check_mem_size_reg(env, reg, regno, zero_size_allowed, meta);
> > > > + } else if (arg_type_is_dynptr(arg_type)) {
> > > > + /* Can't pass in a dynptr at a weird offset */
> > > > + if (reg->off % BPF_REG_SIZE) {
> > >
> > > In invalid_helper2 test, you are passing &dynptr + 8, which means reg will be
> > > fp-8 (assuming dynptr is at top of stack), get_spi will compute spi as 0, so
> > > spi-1 will lead to OOB access for the second dynptr stack slot. If you run the
> > > dynptr test under KASAN, you should see a warning for this.
> > >
> > > So we should ensure here that reg->off is atleast -16.
> > I think this is already checked against in is_spi_bounds(), where we
> > explicitly check that spi - 1 and spi is between [0, the allocated
> > stack). is_spi_bounds() gets called in "is_dynptr_reg_valid_init()" a
> > few lines down where we check if the initialized dynptr arg that's
> > passed in by the program is valid.
> >
> > On my local environment, I simulated this "reg->off = -8" case and
> > this fails the is_dynptr_reg_valid_init() -> is_spi_bounds() check and
> > we get back the correct verifier error "Expected an initialized dynptr
> > as arg #3" without any OOB accesses. I also tried running it with
> > CONFIG_KASAN=y as well and didn't see any warnings show up. But maybe
> > I'm missing something in this analysis - what are your thoughts?
>
> I should have been clearer, the report is for accessing state->stack[spi -
> 1].slot_type[0] in is_dynptr_reg_valid_init, when the program is being loaded.
>
> I can understand why you might not see the warning. It is accessing
> state->stack[spi - 1], and the allocation comes from kmalloc slab cache, so if
> another allocation has an object that covers the region being touched, KASAN
> probably won't complain, and you won't see the warning.
>
> Getting back the correct result for the test can also happen if you don't load
> STACK_DYNPTR at the state->stack[spi - 1].slot_type[0] byte. The test is passing
> for me too, fwiw.
>
> Anyway, digging into this reveals the real problem.
>
> >>> static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> >>> enum bpf_arg_type arg_type)
> >>> {
> >>> struct bpf_func_state *state = func(env, reg);
> >>> int spi = get_spi(reg->off);
> >>>
>
> Here, for reg->off = -8, get_spi is (-(-8) - 1)/BPF_REG_SIZE = 0.
" >
> >>> if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
>
> is_spi_bounds_valid will return true, probably because of the unintended
> conversion of the expression (spi - nr_slots + 1) to unsigned, so the test
Oh interesting. I missed that arithmetic on int and unsigned always
casts the result to unsigned if both have the same conversion rank.
Thanks for pointing this out. I'll change nr_slots to int.
> against >= 0 is always true (compiler will optimize it out), and just test
> whether spi < allocated_stacks.
>
> You should probably declare nr_slots as int, instead of u32. Just doing this
> should be enough to prevent this, without ensuring reg->off is <= -16.
>
> >>> state->stack[spi].slot_type[0] != STACK_DYNPTR ||
>
> Execution moves on to this, which is (second dynptr slot is STACK_DYNPTR).
>
> >>> state->stack[spi - 1].slot_type[0] != STACK_DYNPTR ||
>
> and it accesses state->stack[-1].slot_type[0] here, which triggers the KASAN
> warning for me.
>
> >>> !state->stack[spi].spilled_ptr.dynptr.first_slot)
> >>> return false;
> >>>
> >>> /* ARG_PTR_TO_DYNPTR takes any type of dynptr */
> >>> if (arg_type == ARG_PTR_TO_DYNPTR)
> >>> return true;
> >>>
> >>> return state->stack[spi].spilled_ptr.dynptr.type == arg_to_dynptr_type(arg_type);
> >>> }
>
> > > [...]
>
> There is another issue I noticed while basing other work on this. You have
> declared bpf_dynptr in UAPI header as:
>
> struct bpf_dynptr {
> __u64 :64;
> __u64 :64;
> } __attribute__((aligned(8)));
>
> Sadly, in C standard, the compiler is under no obligation to initialize padding
> bits when the object is zero initialized (using = {}). It is worse, when
> unrelated struct fields are assigned the padding bits are assumed to attain
> unspecified values, but compilers are usually conservative in that case (C11
> 6.2.6.1 p6).
Thanks for noting this. By "padding bits", you are referring to the
unnamed fields, correct?
