On Fri, Jan 5, 2024 at 3:52 PM Eduard Zingerman <eddyz87@xxxxxxxxx> 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. I agree. Thinking some more I agree with you, what I was concerned about should be handled properly at read time. So I think what we have in this patch is ok. Sorry for the noise :) > > 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.
