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

[Eduard Zingerman <eddyz87@gmail.com>]

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.
But that is not related to change in check_stack_write_var_off()
introduced by this patch.