| Message ID | 20230818083920.3771-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Fix an issue in verifing allow_ptr_leaks | expand |
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@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> > Cc: stable@vger.kernel.org > --- > 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 = ®s[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 = ®s[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 = ®s[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@gmail.com> > } 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 = ®s[insn->dst_reg]; > is_jmp32 = BPF_CLASS(insn->code) == BPF_JMP32; > > if (BPF_SRC(insn->code) == BPF_K) {
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 = ®s[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 = ®s[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 = ®s[insn->src_reg]; } 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 = ®s[insn->dst_reg]; is_jmp32 = BPF_CLASS(insn->code) == BPF_JMP32; if (BPF_SRC(insn->code) == BPF_K) {
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> Cc: stable@vger.kernel.org --- kernel/bpf/verifier.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)