lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 22 Oct 2022 19:36:27 +0800 From: shaozhengchao <shaozhengchao@...wei.com> To: Stanislav Fomichev <sdf@...gle.com> CC: <bpf@...r.kernel.org>, <netdev@...r.kernel.org>, <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>, <ast@...nel.org>, <daniel@...earbox.net>, <andrii@...nel.org>, <martin.lau@...ux.dev>, <song@...nel.org>, <yhs@...com>, <john.fastabend@...il.com>, <kpsingh@...nel.org>, <haoluo@...gle.com>, <jolsa@...nel.org>, <oss@....io>, <weiyongjun1@...wei.com>, <yuehaibing@...wei.com> Subject: Re: [PATCH bpf-next] bpf: fix issue that packet only contains l2 is dropped On 2022/10/22 2:16, Stanislav Fomichev wrote: > On Fri, Oct 21, 2022 at 12:25 AM shaozhengchao <shaozhengchao@...wei.com> wrote: >> >> >> >> On 2022/10/21 1:45, Stanislav Fomichev wrote: >>> On Wed, Oct 19, 2022 at 6:47 PM shaozhengchao <shaozhengchao@...wei.com> wrote: >>>> >>>> >>>> >>>> On 2022/10/18 0:36, Stanislav Fomichev wrote: >>>>> On Sat, Oct 15, 2022 at 2:16 AM Zhengchao Shao <shaozhengchao@...wei.com> wrote: >>>>>> >>>>>> As [0] see, bpf_prog_test_run_skb() should allow user space to forward >>>>>> 14-bytes packet via BPF_PROG_RUN instead of dropping packet directly. >>>>>> So fix it. >>>>>> >>>>>> 0: https://github.com/cilium/ebpf/commit/a38fb6b5a46ab3b5639ea4d421232a10013596c0 >>>>>> >>>>>> Fixes: fd1894224407 ("bpf: Don't redirect packets with invalid pkt_len") >>>>>> Signed-off-by: Zhengchao Shao <shaozhengchao@...wei.com> >>>>>> --- >>>>>> net/bpf/test_run.c | 6 +++--- >>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c >>>>>> index 13d578ce2a09..aa1b49f19ca3 100644 >>>>>> --- a/net/bpf/test_run.c >>>>>> +++ b/net/bpf/test_run.c >>>>>> @@ -979,9 +979,6 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb) >>>>>> { >>>>>> struct qdisc_skb_cb *cb = (struct qdisc_skb_cb *)skb->cb; >>>>>> >>>>>> - if (!skb->len) >>>>>> - return -EINVAL; >>>>>> - >>>>>> if (!__skb) >>>>>> return 0; >>>>>> >>>>>> @@ -1102,6 +1099,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, >>>>>> if (IS_ERR(data)) >>>>>> return PTR_ERR(data); >>>>>> >>>>>> + if (size == ETH_HLEN) >>>>>> + is_l2 = true; >>>>>> + >>>>> >>>>> Don't think this will work? That is_l2 is there to expose proper l2/l3 >>>>> skb for specific hooks; we can't suddenly start exposing l2 headers to >>>>> the hooks that don't expect it. >>>>> Does it make sense to start with a small reproducer that triggers the >>>>> issue first? We can have a couple of cases for >>>>> len=0/ETH_HLEN-1/ETH_HLEN+1 and trigger them from the bpf program that >>>>> redirects to different devices (to trigger dev_is_mac_header_xmit). >>>>> >>>>> >>>> Hi Stanislav: >>>> Thank you for your review. Is_l2 is the flag of a specific >>>> hook. Therefore, do you mean that if skb->len is equal to 0, just >>>> add the length back? >>> >>> Not sure I understand your question. All I'm saying is - you can't >>> flip that flag arbitrarily. This flag depends on the attach point that >>> you're running the prog against. Some attach points expect packets >>> with l2, some expect packets without l2. >>> >>> What about starting with a small reproducer? Does it make sense to >>> create a small selftest that adds net namespace + fq_codel + >>> bpf_prog_test run and do redirect ingress/egress with len >>> 0/1...tcphdr? Because I'm not sure I 100% understand whether it's only >>> len=0 that's problematic or some other combination as well? >>> >> yes, only skb->len = 0 will cause null-ptr-deref issue. >> The following is the process of triggering the problem: >> enqueue a skb: >> fq_codel_enqueue() >> ... >> idx = fq_codel_classify() --->if idx != 0 >> flow = &q->flows[idx]; >> flow_queue_add(flow, skb); --->add skb to flow[idex] >> q->backlogs[idx] += qdisc_pkt_len(skb); --->backlogs = 0 >> ... >> fq_codel_drop() --->set sch->limit = 0, always >> drop packets >> ... >> idx = i --->becuase backlogs in every >> flows is 0, so idx = 0 >> ... >> flow = &q->flows[idx]; --->get idx=0 flow >> ... >> dequeue_head() >> skb = flow->head; --->flow->head = NULL >> flow->head = skb->next; --->cause null-ptr-deref >> So, if skb->len !=0,fq_codel_drop() could get the correct idx, and >> then skb!=NULL, it will be OK. >> Maybe, I will fix it in fq_codel. > > I think the consensus here is that the stack, in general, doesn't > expect the packets like this. So there are probably more broken things > besides fq_codel. Thus, it's better if we remove the ability to > generate them from the bpf side instead of fixing the individual users > like fq_codel. > >> But, as I know, skb->len = 0 is just invalid packet. I prefer to add the >> length back, like bellow: >> if (is_l2 || !skb->len) >> __skb_push(skb, hh_len); >> is it OK? > > Probably not? > > Looking at the original syzkaller report, prog_type is > BPF_PROG_TYPE_LWT_XMIT which does expect a packet without l2 header. > Can we do something like: > > if (!is_l2 && !skb->len) { > // append some dummy byte to the skb ? > } > > I pad one byte, and test OK. if (!is_l2 && !skb->len) __skb_push(skb, 1); Does it look OK to you? > } > >>>>> >>>>> >>>>> >>>>>> ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff)); >>>>>> if (IS_ERR(ctx)) { >>>>>> kfree(data); >>>>>> -- >>>>>> 2.17.1 >>>>>>
Powered by blists - more mailing lists