Date:   Mon, 24 Oct 2022 10:13:06 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     shaozhengchao <shaozhengchao@...wei.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 Sat, Oct 22, 2022 at 4:36 AM shaozhengchao <shaozhengchao@...wei.com> wrote:
>
>
>
> 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?

Nope, this will eat a byte out of the l2 header. We need to skb_put
and make sure we allocate enough to make that skb_put succeed.

But stepping back a bit: it feels like it's all unnecessary? The only
valid use-case of this is probing for the BPF_PROG_TEST_RUN as cilium
does. This is mostly about testing, so fixing it in the users seems
fair? No real production code is expected to generate these zero-len
packets. Or are we concerned that this will leak into stable kernels?

I feel like we are trying to add more complexity here for no apparent reason.

Powered by blists - more mailing lists