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.
