On Wed, Nov 29, 2023 at 6:55 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Wed, Nov 29, 2023 at 8:48 AM Andrei Matei <andreimatei1@xxxxxxxxx> wrote:
> >
> > [...]
> >
> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > index af2819d5c8ee..f9546dd73f3c 100644
> > > > --- a/kernel/bpf/verifier.c
> > > > +++ b/kernel/bpf/verifier.c
> > > > @@ -1685,10 +1685,12 @@ static int resize_reference_state(struct bpf_func_state *state, size_t n)
> > > > return 0;
> > > > }
> > > >
> > > > -static int grow_stack_state(struct bpf_func_state *state, int size)
> > > > +/* Possibly update state->allocated_stack to be at least size bytes. Also
> > > > + * possibly update the function's high-water mark in its bpf_subprog_info.
> > > > + */
> > > > +static int grow_stack_state(struct bpf_verifier_env *env, struct bpf_func_state *state, int size)
> > > > {
> > > > size_t old_n = state->allocated_stack / BPF_REG_SIZE, n = size / BPF_REG_SIZE;
> > >
> > > shouldn't this be rounding up? (size + BPF_REG_SIZE - 1) / BPF_REG_SIZE?
> >
> > You're saying this was always broken, regardless of the current patch, right? I
>
> I think so, yes...
I believe that this code was OK after all because all the callers to
grow_stack_state() do the rounding. This seems fragile though; I'll include a
patch to push the rounding inside grow_stack_state() if it's OK to you.
While reading the code around how the stack is accessed, something else caught
my eye. I'm not sure if there's a problem or not; perhaps you could take a
look. Around here in check_stack_write_fixed_off() [1], the code that deals
with register spills has a couple of cases, including:
[1] https://github.com/torvalds/linux/blob/815fb87b753055df2d9e50f6cd80eb10235fe3e9/kernel/bpf/verifier.c#L4733-L4744
} else if (reg && is_spillable_regtype(reg->type)) {
/* register containing pointer is being spilled into stack */
if (size != BPF_REG_SIZE) {
verbose_linfo(env, insn_idx, "; ");
verbose(env, "invalid size of register spill\n");
return -EACCES;
}
if (state != cur && reg->type == PTR_TO_STACK) {
verbose(env, "cannot spill pointers to stack into stack frame of
the caller\n");
return -EINVAL;
}
save_register_state(state, spi, reg, size);
}
This branch, as opposed to all the others, calls save_register_state() without
checking that `!(off % BPF_REG_SIZE)`. I believe save_register_spill()
implicitly assumes that the access is aligned, so it'd be bad if `off` was not
aligned. Is it possible for the offset to not be aligned? I'm not sure... The
higher-level check_mem_access() starts with a check_ptr_alignment(), which I
think does the required check at least on the code paths that I've tried. But
then why do all the other branches in check_stack_write_fixed_off() check for
the alignment explicitly? FWIW, the branch in question also has a
is_spillable_regtype() check which is perhaps relevant.
Would an assertion that off is indeed aligned in the branch I'm talking about
(or elsewhere around) be welcomed?
Apologies if I'm paranoid for no reason.
>
> > think you're right, but that seems like a bug that should have been
> > caught somehow; I'm surprised no programs crashed the verifier. Perhaps in
> > practice all stack accesses are 8-byte aligned, so the rounding doesn't matter?
> >
> > I'll spend a bit of time reading code and come back.
>
> Thanks!
>
> >
> > [...]
