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

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

On 1/5/24 3:52 PM, Eduard Zingerman wrote:
> On Thu, 2024-01-04 at 17:05 -0800, Andrii Nakryiko wrote:
> [...]
>>> The test is actually quite minimal, the longest part is conjuring of
>>> varying offset pointer in r2, here it is with additional comments:
>>>
>>>      /* Write 0 or 100500 to fp-16, 0 is on the first verification pass */
>>>      "call %[bpf_get_prandom_u32];"
>>>      "r9 = 100500;"
>>>      "if r0 > 42 goto +1;"
>>>      "r9 = 0;"
>>>      "*(u64 *)(r10 - 16) = r9;"
>>>      /* prepare a variable length access */
>>>      "call %[bpf_get_prandom_u32];"
>>>      "r0 &= 0xf;" /* r0 range is [0; 15] */
>>>      "r1 = -1;"
>>>      "r1 -= r0;"  /* r1 range is [-16; -1] */
>>>      "r2 = r10;"
>>>      "r2 += r1;"  /* r2 range is [fp-16; fp-1] */
>>>      /* do a variable length write of constant 0 */
>>>      "r0 = 0;"
>>>      "*(u8 *)(r2 + 0) = r0;"
> [...]
>> Yes, and the test fails, but if you read the log, you'll see that fp-8
>> is never marked precise, but it should. So we need more elaborate test
>> that would somehow exploit fp-8 imprecision.
> Sorry, I don't understand why fp-8 should be precise for this particular test.
> Only value read from fp-16 is used in precise context.
>
> [...]
>> So keep both read and write as variable offset. And we are saved by
>> some missing logic in read_var_off that would set r2 as known zero
>> (because it should be for the branch where both fp-8 and fp-16 are
>> zero). But that fails in the branch that should succeed, and if that
>> branch actually succeeds, I suspect the branch where we initialize
>> with non-zero r9 will erroneously succeed.
>>
>> Anyways, I still claim that we are mishandling a precision of spilled
>> register when doing zero var_off writes.
> Currently check_stack_read_var_off() has two possible outcomes:
> - if all bytes at varying offset are STACK_ZERO destination register
>    is set to zero;
> - otherwise destination register is set to unbound scalar.
>
> Unless I missed something, STACK_ZERO is assigned to .slot_type only
> in check_stack_write_fixed_off(), and there the source register is
> marked as precise immediately.
>
> So, it appears to me that current state of patch #1 is ok.
>
> In case if check_stack_read_var_off() would be modified to check not
> only for STACK_ZERO, but also for zero spills, I think that all such
> spills would have to be marked precise at the time of read,
> as backtracking would not be able to find those later.

I don't understand the above. If the code pattern looks like
   r1 = ...; /* r1 range [-32, -16);
   *(u8 *)(r10 + r1) = r2;
   ...
   r3 = *(u8 *)(r10 + r1);
   r3 needs to be marked as precise.

Conservatively marking r2 in '*(u8 *)(r10 + r1) = r2' as precise
should be the correct way to do.

Or you are thinking even more complex code pattern like
   *(u64 *)(r10 - 32) = r4;
   *(u64 *)(r10 - 24) = r5;
   ...
   r1 = ...; /* r1 range [-32, -16) */
   r3 = *(u8 *)(r10 + r1);
   r3 needs to be marked as precise.

In this case, we should proactively mark r4 and r5 as precise.
But currently we did not do it, right?
I think this later case is a very unlikely case.

> But that is not related to change in check_stack_write_var_off()
> introduced by this patch.