Re: [PATCH bpf-next v2 1/2] bpf: Track aligned st store as imprecise spilled registers

[Yonghong Song <yonghong.song@xxxxxxxxx>]

On 1/4/24 10:30 AM, Eduard Zingerman wrote:
> On Wed, 2024-01-03 at 15:26 -0800, Yonghong Song wrote:
>
> I missed one thing while looking at this patch, please see below.
>
> [...]
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index d4e31f61de0e..cfe7a68d90a5 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4491,7 +4491,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> >   		if (fls64(reg->umax_value) > BITS_PER_BYTE * size)
> >   			state->stack[spi].spilled_ptr.id = 0;
> >   	} else if (!reg && !(off % BPF_REG_SIZE) && is_bpf_st_mem(insn) &&
> > -		   insn->imm != 0 && env->bpf_capable) {
> > +		   env->bpf_capable) {
> >   		struct bpf_reg_state fake_reg = {};
> >   
> >   		__mark_reg_known(&fake_reg, insn->imm);
> > @@ -4613,11 +4613,28 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env,
> >   
> >   	/* Variable offset writes destroy any spilled pointers in range. */
> >   	for (i = min_off; i < max_off; i++) {
> > +		struct bpf_reg_state *spill_reg;
> >   		u8 new_type, *stype;
> > -		int slot, spi;
> > +		int slot, spi, j;
> >   
> >   		slot = -i - 1;
> >   		spi = slot / BPF_REG_SIZE;
> > +
> > +		/* If writing_zero and the the spi slot contains a spill of value 0,
> > +		 * maintain the spill type.
> > +		 */
> > +		if (writing_zero && !(i % BPF_REG_SIZE) && is_spilled_scalar_reg(&state->stack[spi])) {
> Talked to Andrii today, and he noted that spilled reg should be marked
> precise at this point.

Could you help explain why?

Looks we did not mark reg as precise with fixed offset as below:

        if (reg && !(off % BPF_REG_SIZE) && register_is_bounded(reg) && env->bpf_capable) {
                save_register_state(env, state, spi, reg, size);
                /* Break the relation on a narrowing spill. */
                if (fls64(reg->umax_value) > BITS_PER_BYTE * size)
                        state->stack[spi].spilled_ptr.id = 0;
        } else if (!reg && !(off % BPF_REG_SIZE) && is_bpf_st_mem(insn) &&
                   insn->imm != 0 && env->bpf_capable) {

I probably missed something about precision tracking...

> > +			spill_reg = &state->stack[spi].spilled_ptr;
> > +			if (tnum_is_const(spill_reg->var_off) && spill_reg->var_off.value == 0) {
> > +				for (j = BPF_REG_SIZE; j > 0; j--) {
> > +					if (state->stack[spi].slot_type[j - 1] != STACK_SPILL)
> > +						break;
> > +				}
> > +				i += BPF_REG_SIZE - j - 1;
> > +				continue;
> > +			}
> > +		}
> > +
> >   		stype = &state->stack[spi].slot_type[slot % BPF_REG_SIZE];
> >   		mark_stack_slot_scratched(env, spi);