On Mon, Dec 4, 2023 at 4:54 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2023-12-04 at 16:23 -0800, Andrii Nakryiko wrote:
> [...]
> > > > @@ -4431,7 +4431,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> > > > * so it's aligned access and [off, off + size) are within stack limits
> > > > */
> > > > if (!env->allow_ptr_leaks &&
> > > > - state->stack[spi].slot_type[0] == STACK_SPILL &&
> > > > + is_spilled_reg(&state->stack[spi]) &&
> > > > size != BPF_REG_SIZE) {
> > > > verbose(env, "attempt to corrupt spilled pointer on stack\n");
> > > > return -EACCES;
> > >
> > > I think there is a small detail here.
> > > slot_type[0] == STACK_SPILL actually checks if a spill is 64-bit.
> >
> > Hm... I wonder if the check was written like this deliberately to
> > prevent turning any spilled register into STACK_MISC?
>
> idk, the error is about pointers and forbidding turning pointers to
> STACK_MISC makes sense. Don't see why it would be useful to forbid
> this for scalars.
you are correct that this check doesn't make sense for SCALAR_VALUE
register spill, I think the intent was to prevent pointer spills. But
that's an orthogonal issue, this could be improved separately.
>
> > > Thus, with this patch applied the test below does not pass.
> > > Log fragment:
> > >
> > > 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff))
> > > 2: (63) *(u32 *)(r10 -8) = r0
> > > 3: R0_w=scalar(...,var_off=(0x0; 0xffff)) R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff))
> > > 3: (b7) r0 = 42 ; R0_w=42
> > > 4: (63) *(u32 *)(r10 -4) = r0
> > > attempt to corrupt spilled pointer on stack
> >
> > What would happen if we have
> >
> > 4: *(u16 *)(r10 - 8) = 123; ?
>
> w/o this patch:
>
> 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar()
> 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff))
> 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(...,var_off=(0x0; 0xffff))
> R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff))
> 3: (b7) r0 = 123 ; R0_w=123
> 4: (6b) *(u16 *)(r10 -8) = r0 ; R0_w=123 R10=fp0 fp-8=mmmmmm123
> 5: (95) exit
>
> with this patch:
>
> 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar()
> 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff))
> 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(...,var_off=(0x0; 0xffff))
> R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff))
> 3: (b7) r0 = 123 ; R0_w=123
> 4: (6b) *(u16 *)(r10 -8) = r0
> attempt to corrupt spilled pointer on stack
ok, so SCALAR_VALUE aside, if it was some pointer, we should be
rejecting these writes
>
> > and similarly
> >
> > 4: *(u16 *)(r10 - 6) = 123; ?
>
> w/o this patch:
>
> 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar()
> 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff))
> 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(....,var_off=(0x0; 0xffff))
> R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff))
> 3: (b7) r0 = 123 ; R0_w=123
> 4: (6b) *(u16 *)(r10 -6) = r0 ; R0_w=123 R10=fp0 fp-8=mmmmmmmm
> 5: (95) exit
>
> with this patch:
>
> 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar()
> 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff))
> 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(...,var_off=(0x0; 0xffff))
> R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff))
> 3: (b7) r0 = 123 ; R0_w=123
> 4: (6b) *(u16 *)(r10 -6) = r0
> attempt to corrupt spilled pointer on stack
>
> > So it makes me feel like the intent was to reject any partial writes
> > with spilled reg slots. We could probably improve that to just make
> > sure that we don't turn spilled pointers into STACK_MISC in unpriv,
> > but I'm not sure if it's worth doing that instead of keeping things
> > simple?
>
> You mean like below?
>
> if (!env->allow_ptr_leaks &&
> is_spilled_reg(&state->stack[spi]) &&
> is_spillable_regtype(state->stack[spi].spilled_ptr.type) &&
Honestly, I wouldn't trust is_spillable_regtype() the way it's
written, it's too easy to forget to add a new register type to the
list. I think the only "safe to spill" register is probably
SCALAR_VALUE, so I'd just do `type != SCALAR_VALUE`.
But yes, I think that's the right approach.
If we were being pedantic, though, we'd need to take into account
offset and see if [offset, offset + size) overlaps with any
STACK_SPILL/STACK_DYNPTR/STACK_ITER slots.
But tbh, given it's unpriv programs we are talking about, I probably
wouldn't bother extending this logic too much.
> size != BPF_REG_SIZE) {
> verbose(env, "attempt to corrupt spilled pointer on stack\n");
> return -EACCES;
> }
