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@xxxxxxxxxxxxxx>
> ---
Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
[...]
> +#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";
