Re: [PATCH 5.10 294/406] bpf: Fix issue in verifying allow_ptr_leaks

Eduard Zingerman
2023-09-17T20:22:21Z

On Sun, 2023-09-17 at 21:12 +0200, Greg Kroah-Hartman wrote:
> 5.10-stable review patch. If anyone has any objections, please let me know.

I believe Luis Gerhorst posted an objection to this patch for 6.1 in [1],
for reasons described in [2]. The objection is relevant for 5.10 as well
(does not depend on kernel version, actually).

[1] https://lore.kernel.org/all/2023091653-peso-sprint-889d@gregkh/
[2] https://lore.kernel.org/bpf/20230913122827.91591-1-gerhorst@amazon.de/

>
> ------------------
>
> From: Yafang Shao <laoar.shao@gmail.com>
>
> commit d75e30dddf73449bc2d10bb8e2f1a2c446bc67a2 upstream.
>
> 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@mail.gmail.com/
>
> Suggested-by: Yonghong Song <yonghong.song@linux.dev>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/r/20230823020703.3790-2-laoar.shao@gmail.com
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> kernel/bpf/verifier.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8178,6 +8178,12 @@ static int check_cond_jmp_op(struct bpf_
> 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");
> @@ -8189,12 +8195,13 @@ static int check_cond_jmp_op(struct bpf_
> 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];
> } else {
> if (insn->src_reg != BPF_REG_0) {
> verbose(env, "BPF_JMP/JMP32 uses reserved fields\n");
> @@ -8202,12 +8209,6 @@ static int check_cond_jmp_op(struct bpf_
> }
> }
>
> - /* 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) {
>
>