Re: [PATCH bpf v3 2/2] selftests/bpf: Test r0 and ref lifetime after BPF-BPF call with abnormal return

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2025-01-06 at 18:15 +0100, Arthur Fabre wrote:
> In all three cases where a callee can abnormally return (tail_call(),
> LD_ABS, and LD_IND), test the verifier doesn't know the bounds of:
> 
> - r0 / what the callee returned.
> - References to the caller's stack passed to the callee.
> 
> Additionally, ensure the tail_call fallthrough case can't access r0, as
> bpf_tail_call() returns nothing on failure.
> 
> Signed-off-by: Arthur Fabre <afabre@cloudflare.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> +#define TEST(NAME, CALLEE) \
> +	SEC("socket")					\
> +	__description("r0: " #NAME)	\
> +	__failure __msg("math between ctx pointer and register with unbounded min value") \
> +	__naked int check_abnormal_ret_r0_##NAME(void)	\
> +	{						\
> +		asm volatile("				\
> +		r6 = r1;				\
> +		r2 = r10;				\
> +		r2 += -8;				\
> +		call " #CALLEE ";			\
> +		r6 += r0;				\
> +		r0 = 0;					\
> +		exit;					\
> +	"	:					\
> +		:					\
> +		: __clobber_all);			\
> +	}						\
> +							\
> +	SEC("socket")					\
> +	__description("ref: " #NAME)	\
> +	__failure __msg("math between ctx pointer and register with unbounded min value") \
> +	__naked int check_abnormal_ret_ref_##NAME(void)	\
> +	{						\
> +		asm volatile("				\
> +		r6 = r1;				\
> +		r7 = r10;				\
> +		r7 += -8;				\
> +		r2 = r7;				\
> +		call " #CALLEE ";			\
> +		r0 = *(u64*)(r7 + 0);			\
> +		r6 += r0;				\
> +		exit;					\
> +	"	:					\
> +		:					\
> +		: __clobber_all);			\
> +	}

Nit: I think having both cases is an overkill, as both effectively
     test if branching occur.

[...]

> +struct {
> +	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> +	__uint(max_entries, 1);
> +	__uint(key_size, sizeof(int));
> +	__array(values, void(void));
> +} map_prog SEC(".maps") = {
> +	.values = {
> +		[0] = (void *)&dummy_prog,
> +	},
> +};
> +
> +static __noinline __used
> +int callee_tail_call(struct __sk_buff *skb, __u64 *foo)
> +{
> +	bpf_tail_call(skb, &map_prog, 0);
> +	*foo = 1;
> +	return 0;
> +}

Nit: I'd also add a test where invalid action is taken
     after bpf_tail_call inside the callee,
     just to make sure that both branches are explored.

> +
> +SEC("socket")
> +__description("r0 not set by tail_call")
> +__failure __msg("R0 !read_ok")
> +int check_abnormal_ret_tail_call_fail(struct __sk_buff *skb)
> +{
> +	return bpf_tail_call(skb, &map_prog, 0);
> +}
> +
> +char _license[] SEC("license") = "GPL";