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

[Yonghong Song <yonghong.song@linux.dev>]

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