On Thu, Dec 19, 2024 at 4:43 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Thu, 2024-12-19 at 17:40 -0700, Daniel Xu wrote: > > [...] > > > > Ok, thinking a bit more, the best test I can come up with is: > > > > > > u8 vals[8]; > > > vals[0] = 0; > > > ... > > > vals[6] = 0; > > > vals[7] = 0xf; > > > p = bpf_map_lookup_elem(... vals ...); > > > *p = 42; > > > > > > For LE vals as u32 should be 0x0f; > > > For BE vals as u32 should be 0xf000_0000. > > > Hence, it is not safe to remove null check for this program. > > > What would verifier think about the value of such key? > > > As far as I understand, there would be stack zero for for vals[0-6] > > > and u8 stack spill for vals[7]. > > > > Right. By checking that spill size is same as key size, we stay endian > > neutral, as constant values are tracked in native endianness. > > > > However, if we were to start interpreting combinations of STACK_ZERO, > > STACK_MISC, and STACK_SPILL, the verifier would have to be endian aware > > (IIUC). Which makes it a somewhat interesting problem but also requires > > some thought to correctly handle the state space. > > Right. > > > > You were going to add a check for the spill size, which should help here. > > > So, a negative test like above that checks that verifier complains > > > that 'p' should be checked for nullness first? > > > > > > If anyone has better test in mind, please speak-up. > > > > I think this case reduces down to a spill_size != key_size test. As long > > as the sizes match, we don't have to worry about endianness. > > Agree. Earlier I suggested to generalize this zero/misc/spill counting into a helper and reuse here and in check_stack_read_fixed_off(). We do very similar checks there with a similar purpose. It sounds there are ideas to make this particular feature smarter than what we have in check_stack_read_fixed_off(). Let's not overdo it. Even if a common helper is not possible, keep things consistent. The simpler the better.
