linux-kernel - 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: <20220211040629.23703-1-lina.wang@mediatek.com>
Date:   Fri, 11 Feb 2022 12:06:29 +0800
From:   Lina Wang <lina.wang@...iatek.com>
To:     "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>
CC:     <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <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>, <maze@...gle.com>,
        <willemb@...gle.com>, <edumazet@...gle.com>,
        <zhuoliang.zhang@...iatek.com>, <chao.song@...iatek.com>
Subject: Re: [PATCH] net: fix wrong network header length

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

> >  
> >  	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!