netdev - Re: [PATCH] net: fix wrong network header length

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: <CANP3RGc42sobVoq8LCs=9dAgRZmerV7ev_EMi8Qjb+1ZKeO4jQ@mail.gmail.com>
Date:   Thu, 10 Feb 2022 21:54:23 -0800
From:   Maciej Żenczykowski <maze@...gle.com>
To:     Lina Wang <lina.wang@...iatek.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Linux NetDev <netdev@...r.kernel.org>,
        Kernel hackers <linux-kernel@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Willem Bruijn <willemb@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        zhuoliang.zhang@...iatek.com, chao.song@...iatek.com
Subject: Re: [PATCH] net: fix wrong network header length

On Thu, Feb 10, 2022 at 8:12 PM Lina Wang <lina.wang@...iatek.com> wrote:
>
> On Thu, 2022-02-10 at 17:02 +0100, Paolo Abeni wrote:
>
> > > @@ -3682,6 +3682,7 @@ struct sk_buff *skb_segment_list(struct
> > > sk_buff *skb,
> > >     struct sk_buff *tail = NULL;
> > >     struct sk_buff *nskb, *tmp;
> > >     int err;
> > > +   unsigned int len_diff = 0;
> >
> > Mintor nit: please respect the reverse x-mas tree order.
> >
>
> Yes,v2 has change unsigned int to int

Reverse christmas tree, means from longest to shortest, like so:

     struct sk_buff *tail = NULL;
     struct sk_buff *nskb, *tmp;
+   int len_diff = 0;
     int err;

That said, I think the = 0 is not needed, so this can be just

+ int len_diff, err;

>
> > >
> > >     skb_push(skb, -skb_network_offset(skb) + offset);
> > > @@ -3721,9 +3722,11 @@ struct sk_buff *skb_segment_list(struct
> > > sk_buff *skb,
> > >             skb_push(nskb, -skb_network_offset(nskb) + offset);
> > >
> > >             skb_release_head_state(nskb);
> > > +           len_diff = skb_network_header_len(nskb) -
> > > skb_network_header_len(skb);
> > >              __copy_skb_header(nskb, skb);
> > >
> > >             skb_headers_offset_update(nskb, skb_headroom(nskb) -
> > > skb_headroom(skb));
> > > +           nskb->transport_header += len_diff;
> >
> > This does not look correct ?!? the network hdr position for nskb will
> > still be uncorrect?!? and even the mac hdr likely?!? possibly you
> > need
> > to change the offset in skb_headers_offset_update().
> >
>
> Network hdr position and mac hdr are both right, because bpf processing &
> skb_headers_offset_update have updated them to right position. After bpf
> loading, the first skb's network header&mac_header became 44, transport
> header still is 64. After skb_headers_offset_update, fraglist skb's mac
> header and network header are still 24, the same with original packet.
> Just fraglist skb's transport header became 44, as original is 64.
> Only transport header cannot be easily updated the same offset, because
> 6to4 has different network header.
>
> Actually,at the beginning, I want to change skb_headers_offset_update, but
> it has been called also in other place, maybe a new function should be
> needed here.
>
> Skb_headers_offset_update has other wrong part in my scenary,
> inner_transport_header\inner_network_header\inner_mac_header shouldnot be
> changed, but they are been updated because of different headroom. They are
> not used later, so wrong value didnot affect anything.
>
> > Paolo
> >
>
> Thanks!Maciej Żenczykowski, Kernel Networking Developer @ Google