netdev - Re: [PATCH bpf-next] bpf: fix issue that packet only contains l2 is dropped

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87f67a8c-2fb2-9478-adbb-f55c7a7c94f9@huawei.com>
Date:   Fri, 21 Oct 2022 15:25:04 +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/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.

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?
>>>
>>>
>>>
>>>>           ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff));
>>>>           if (IS_ERR(ctx)) {
>>>>                   kfree(data);
>>>> --
>>>> 2.17.1
>>>>