netdev - Re: [PATCH v2] ip6_tunnel: Clear IP6CB in ip6_tnl

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:   Fri, 28 Oct 2016 13:13:45 +0800
From:   Eli Cooper <elicooper@....com>
To:     Tom Herbert <tom@...bertland.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2] ip6_tunnel: Clear IP6CB in ip6_tnl_xmit() after
 encapsulation

On 2016/10/28 10:17, Tom Herbert wrote:
> On Thu, Oct 27, 2016 at 6:52 PM, Eli Cooper <elicooper@....com> wrote:
>> > skb->cb may contain data from previous layers. In the observed scenario,
>> > the garbage data were misinterpreted as IP6CB(skb)->frag_max_size, so
>> > that small packets sent through the tunnel are mistakenly fragmented.
>> >
>> > This patch clears the control buffer for the next layer, after an IPv6
>> > header is installed.
>> >
> Nice catch, but can you rectify this with what udp_tunnel6_xmit_skb is
> doing. udp_tunnel6_xmit_skb calls ip6tunnel_xmit directly. Looks like
> we do
>
> memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
> IPCB(skb)->flags &= ~(IPSKB_XFRM_TUNNEL_SIZE | IPSKB_XFRM_TRANSFORMED
>                             | IPSKB_REROUTED);
>
> Is this what we should be doing in ip6_tnl_xmit also, or is
> udp_tunnel6_xmit_skb broken because it doesn't zero all the cb?

udp_tunnel6_xmit_skb() is also broken, I can confirm. Actually I found
it pretty easy to reproduce by having a netns or router forwarding
between two tunnels established with another two namespaces or hosts.
The router sends out packets with IPv6 Fragment headers, even when the
packet is too small to really get fragmented.

The control buffer, by definition, is private to the layer having the
skb queued at the moment. As David has pointed out in v1 of this patch,
cb is either interpreted as IPCB or as IP6CB, and in this case, it will
be IP6CB after ip6tunnel_xmit(). So I think it is best that all the
IP6CB gets cleared before it is pushed to the next layer. Maybe we
should clear IP6CB in ip6tunnel_xmit(), rather than in every tunnel's codes?

By the way, I don't see any point in setting IPCB(skb)->flags in
udp_tunnel6_xmit_skb(). It will not be interpreted as IPCB any further
past ip6tunnel_xmit(), even if it were not cleared. Plus, nothing seems
to use these flags anyway.

Thanks,
Eli