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]

Date:   Thu, 20 Oct 2022 09:46:54 +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/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?
> 
> 
> 
>>          ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff));
>>          if (IS_ERR(ctx)) {
>>                  kfree(data);
>> --
>> 2.17.1
>>