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]

Date:   Mon, 17 Oct 2022 09:36:10 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Zhengchao Shao <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 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).





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