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

[Eduard Zingerman <eddyz87@gmail.com>]

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.

> +			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);