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
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).
See 5eaed6eedbe9 ("bpf: Fix a bpf_timer initialization issue") on how this has
bitten us once before.
I was kinda surprised you don't hit this with your selftests, since in the BPF
assembly of dynptr_fail.o/dynptr_success.o I seldom see stack location of dynptr
being zeroed out. But after applying the fix for the above issue, I see this
error and many failing tests (only 26/36 passed).
verifier internal error: variable not initialized on stack in mark_as_dynptr_data
So I think the bug above was papering over this issue? I will look at it in more
detail later.
--
Kartikeya
