Re: [PATCH bpf-next 1/2] bpf: Fix issue in verifying allow_ptr_leaks

[Eduard Zingerman <eddyz87@xxxxxxxxx>]

On Fri, 2023-08-18 at 08:39 +0000, Yafang Shao wrote:
> After we converted the capabilities of our networking-bpf program from
> cap_sys_admin to cap_net_admin+cap_bpf, our networking-bpf program
> failed to start. Because it failed the bpf verifier, and the error log
> is "R3 pointer comparison prohibited".
> 
> A simple reproducer as follows,
> 
> SEC("cls-ingress")
> int ingress(struct __sk_buff *skb)
> {
> 	struct iphdr *iph = (void *)(long)skb->data + sizeof(struct ethhdr);
> 
> 	if ((long)(iph + 1) > (long)skb->data_end)
> 		return TC_ACT_STOLEN;
> 	return TC_ACT_OK;
> }
> 
> Per discussion with Yonghong and Alexei [1], comparison of two packet
> pointers is not a pointer leak. This patch fixes it.
> 
> Our local kernel is 6.1.y and we expect this fix to be backported to
> 6.1.y, so stable is CCed.
> 
> [1]. https://lore.kernel.org/bpf/CAADnVQ+Nmspr7Si+pxWn8zkE7hX-7s93ugwC+94aXSy4uQ9vBg@xxxxxxxxxxxxxx/
> 
> Suggested-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  kernel/bpf/verifier.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4ccca1f..b6b60cd 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -14047,6 +14047,12 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		return -EINVAL;
>  	}
>  
> +	/* check src2 operand */
> +	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
> +	if (err)
> +		return err;
> +
> +	dst_reg = &regs[insn->dst_reg];
>  	if (BPF_SRC(insn->code) == BPF_X) {
>  		if (insn->imm != 0) {
>  			verbose(env, "BPF_JMP/JMP32 uses reserved fields\n");
> @@ -14058,12 +14064,13 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		if (err)
>  			return err;
>  
> -		if (is_pointer_value(env, insn->src_reg)) {
> +		src_reg = &regs[insn->src_reg];
> +		if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) &&
> +		    is_pointer_value(env, insn->src_reg)) {
>  			verbose(env, "R%d pointer comparison prohibited\n",
>  				insn->src_reg);
>  			return -EACCES;
>  		}
> -		src_reg = &regs[insn->src_reg];

I tested this change and it seem to work as intended. Was worried a
bit that there are three places in this function where such checks are
applied:
1. upon entry for BPF_X case (this one): checks if dst_reg/src_reg are
   pointers to packet or packet end or packet meta;
2. when attempting to predict branch: prediction would be triggered
   only when dst/src is packet/packet_end (or vice-versa);
3. when prediction failed and both branches have to be visited
   (`try_match_pkt_pointers`): dst/src have to be packet/packet_end or
   meta/packet-start (or vice versa).
   
Check (1) is more permissive than (2) or (3) but either (2) or (3)
would be applied before exit, so there is no contradiction.

Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>

>  	} else {
>  		if (insn->src_reg != BPF_REG_0) {
>  			verbose(env, "BPF_JMP/JMP32 uses reserved fields\n");
> @@ -14071,12 +14078,6 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		}
>  	}
>  
> -	/* check src2 operand */
> -	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
> -	if (err)
> -		return err;
> -
> -	dst_reg = &regs[insn->dst_reg];
>  	is_jmp32 = BPF_CLASS(insn->code) == BPF_JMP32;
>  
>  	if (BPF_SRC(insn->code) == BPF_K) {