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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAKH8qBtyfS0Otpugn7_ZiG5APA_WTKOVAe1wsFfyaxF-03X=5w@mail.gmail.com>
Date:   Thu, 20 Oct 2022 10:45:00 -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 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?

> >
> >
> >
> >>          ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff));
> >>          if (IS_ERR(ctx)) {
> >>                  kfree(data);
> >> --
> >> 2.17.1
> >>