Re: [PATCH bpf v2 1/2] bpf: Don't trust r0 bounds after BPF to BPF calls with abnormal returns

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2024-12-13 at 22:27 +0100, Arthur Fabre wrote:
> When making BPF to BPF calls, the verifier propagates register bounds
> info for r0 from the callee to the caller.
> 
> For example loading:
> 
>     #include <linux/bpf.h>
>     #include <bpf/bpf_helpers.h>
> 
>     static __attribute__((noinline)) int callee(struct xdp_md *ctx)
>     {
>             int ret;
>             asm volatile("%0 = 23" : "=r"(ret));
>             return ret;
>     }
> 
>     static SEC("xdp") int caller(struct xdp_md *ctx)
>     {
>             int res = callee(ctx);
>             if (res == 23) {
>                     return XDP_PASS;
>             }
>             return XDP_DROP;
>     }
> 
> The verifier logs:
> 
>     func#0 @0
>     func#1 @6
>     0: R1=ctx() R10=fp0
>     ; int res = callee(ctx); @ test.c:15
>     0: (85) call pc+5
>     caller:
>      R10=fp0
>     callee:
>      frame1: R1=ctx() R10=fp0
>     6: frame1: R1=ctx() R10=fp0
>     ; asm volatile("%0 = 23" : "=r"(ret)); @ test.c:9
>     6: (b7) r0 = 23                       ; frame1: R0_w=23
>     ; return ret; @ test.c:10
>     7: (95) exit
>     returning from callee:
>      frame1: R0_w=23 R1=ctx() R10=fp0
>     to caller at 1:
>      R0_w=23 R10=fp0
> 
>     from 7 to 1: R0_w=23 R10=fp0
>     ; int res = callee(ctx); @ test.c:15
>     1: (bc) w1 = w0                       ; R0_w=23 R1_w=23
>     2: (b4) w0 = 2                        ; R0_w=2
>     ;  @ test.c:0
>     3: (16) if w1 == 0x17 goto pc+1
>     3: R1_w=23
>     ; } @ test.c:20
>     5: (95) exit
>     processed 7 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> 
> And correctly tracks R0_w=23 from the callee through to the caller.
> This lets it completely prune the res != 23 branch, skipping over
> instruction 4.
> 
> But this isn't sound if the callee can return "abnormally" before an
> exit instruction:
> - If LD_ABS or LD_IND try to access data beyond the end of the packet,
>   the callee returns 0 directly.
> - If a tail_call succeeds, the return value of the tail called program
>   will be returned directly.
> We can't know what the bounds of r0 will be.
> 
> The verifier still incorrectly tracks the bounds of r0 in these cases. Loading:
> 
>     #include <linux/bpf.h>
>     #include <bpf/bpf_helpers.h>
> 
>     struct {
>             __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
>             __uint(max_entries, 1);
>             __uint(key_size, sizeof(__u32));
>             __uint(value_size, sizeof(__u32));
>     } tail_call_map SEC(".maps");
> 
>     static __attribute__((noinline)) int callee(struct xdp_md *ctx)
>     {
>             bpf_tail_call(ctx, &tail_call_map, 0);
> 
>             int ret;
>             asm volatile("%0 = 23" : "=r"(ret));
>             return ret;
>     }
> 
>     static SEC("xdp") int caller(struct xdp_md *ctx)
>     {
>             int res = callee(ctx);
>             if (res == 23) {
>                     return XDP_PASS;
>             }
>             return XDP_DROP;
>     }
> 
> The verifier logs:
> 
>     func#0 @0
>     func#1 @6
>     0: R1=ctx() R10=fp0
>     ; int res = callee(ctx); @ test.c:24
>     0: (85) call pc+5
>     caller:
>      R10=fp0
>     callee:
>      frame1: R1=ctx() R10=fp0
>     6: frame1: R1=ctx() R10=fp0
>     ; bpf_tail_call(ctx, &tail_call_map, 0); @ test.c:15
>     6: (18) r2 = 0xffff8a9c82a75800       ; frame1: R2_w=map_ptr(map=tail_call_map,ks=4,vs=4)
>     8: (b4) w3 = 0                        ; frame1: R3_w=0
>     9: (85) call bpf_tail_call#12
>     10: frame1:
>     ; asm volatile("%0 = 23" : "=r"(ret)); @ test.c:18
>     10: (b7) r0 = 23                      ; frame1: R0_w=23
>     ; return ret; @ test.c:19
>     11: (95) exit
>     returning from callee:
>      frame1: R0_w=23 R10=fp0
>     to caller at 1:
>      R0_w=23 R10=fp0
> 
>     from 11 to 1: R0_w=23 R10=fp0
>     ; int res = callee(ctx); @ test.c:24
>     1: (bc) w1 = w0                       ; R0_w=23 R1_w=23
>     2: (b4) w0 = 2                        ; R0=2
>     ;  @ test.c:0
>     3: (16) if w1 == 0x17 goto pc+1
>     3: R1=23
>     ; } @ test.c:29
>     5: (95) exit
>     processed 10 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
> 
> It still prunes the res != 23 branch, skipping over instruction 4.
> But the tail called program can return any value.
> 
> Aside from pruning incorrect branches, this can also be used to read and
> write arbitrary memory by using r0 as a index.
> 
> Fixes: e411901c0b77 ("bpf: allow for tailcalls in BPF subprograms for x64 JIT")
> Signed-off-by: Arthur Fabre <afabre@cloudflare.com>
> Cc: stable@vger.kernel.org
> ---

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

[...]

> @@ -10359,6 +10364,10 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
>  				*insn_idx, callee->callsite);
>  			return -EFAULT;
>  		}
> +	} else if (has_abnormal_return(
> +		    &env->subprog_info[state->frame[state->curframe]->subprogno])) {
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                       Nit: this is 'callee'

> +		/* callee can return before exit instruction, r0 could hold anything */
> +		__mark_reg_unknown(env, &caller->regs[BPF_REG_0]);
>  	} else {
>  		/* return to the caller whatever r0 had in the callee */
>  		caller->regs[BPF_REG_0] = *r0;
> @@ -16881,17 +16890,14 @@ static int check_cfg(struct bpf_verifier_env *env)
>  	return ret;
>  }
>  
> +

Nit: this empty line is not needed.

[...]