On Thu, 2024-12-12 at 16:22 -0700, Daniel Xu wrote:
> Previously, the verifier was treating all PTR_TO_STACK registers passed
> to a helper call as potentially written to by the helper. However, all
> calls to check_stack_range_initialized() already have precise access type
> information available.
>
> Rather than treat ACCESS_HELPER as a proxy for BPF_WRITE, pass
> enum bpf_access_type to check_stack_range_initialized() to more
> precisely track helper arguments.
>
> One benefit from this precision is that registers tracked as valid
> spills and passed as a read-only helper argument remain tracked after
> the call. Rather than being marked STACK_MISC afterwards.
>
> An additional benefit is the verifier logs are also more precise. For
> this particular error, users will enjoy a slightly clearer message. See
> included selftest updates for examples.
>
> Signed-off-by: Daniel Xu <dxu@xxxxxxxxx>
> ---
I think this change is ok.
With it there is only one use of 'enum bpf_access_src' remains,
but it doesn't look like it could be removed.
Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
[...]
> --- a/tools/testing/selftests/bpf/progs/uninit_stack.c
> +++ b/tools/testing/selftests/bpf/progs/uninit_stack.c
> @@ -55,33 +55,4 @@ exit_%=: r0 = 0; \
> : __clobber_all);
> }
>
> -static __noinline void dummy(void) {}
> -
> -/* Pass a pointer to uninitialized stack memory to a helper.
> - * Passed memory block should be marked as STACK_MISC after helper call.
> - */
> -SEC("socket")
> -__log_level(7) __msg("fp-104=mmmmmmmm")
> -__naked int helper_uninit_to_misc(void *ctx)
Is it possible to peek a helper that writes into memory and not delete
this test?
> -{
> - asm volatile (" \
> - /* force stack depth to be 128 */ \
> - *(u64*)(r10 - 128) = r1; \
> - r1 = r10; \
> - r1 += -128; \
> - r2 = 32; \
> - call %[bpf_trace_printk]; \
> - /* Call to dummy() forces print_verifier_state(..., true), \
> - * thus showing the stack state, matched by __msg(). \
> - */ \
> - call %[dummy]; \
> - r0 = 0; \
> - exit; \
> -"
> - :
> - : __imm(bpf_trace_printk),
> - __imm(dummy)
> - : __clobber_all);
> -}
> -
[...]
