netdev - Re: [PATCH v6 net-next 09/14] ip6_tun: Add infrastructure for doing encapsulation

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: <CAKgT0UcMxXwRRi8QcrOFKpQ50OuLSEu90wM4wcVq5j=_=t54Eg@mail.gmail.com>
Date:	Mon, 16 May 2016 15:47:31 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Tom Herbert <tom@...bertland.com>
Cc:	David Miller <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>, Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v6 net-next 09/14] ip6_tun: Add infrastructure for doing encapsulation

On Mon, May 16, 2016 at 3:37 PM, Tom Herbert <tom@...bertland.com> wrote:
> On Mon, May 16, 2016 at 3:25 PM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> On Mon, May 16, 2016 at 2:33 PM, Tom Herbert <tom@...bertland.com> wrote:
>>> Add encap_hlen and ip_tunnel_encap structure to ip6_tnl. Add functions
>>> for getting encap hlen, setting up encap on a tunnel, performing
>>> encapsulation operation.
>>>
>>> Signed-off-by: Tom Herbert <tom@...bertland.com>
>>> ---
>>>  include/net/ip6_tunnel.h  | 58 ++++++++++++++++++++++++++++++++
>>>  net/ipv4/ip_tunnel_core.c |  5 +++
>>>  net/ipv6/ip6_tunnel.c     | 85 ++++++++++++++++++++++++++++++++++++++++++-----
>>>  3 files changed, 139 insertions(+), 9 deletions(-)
>>>
>>
>> So it looks like you completely dropped the two spots that were
>> updating mtu and max_headroom with the t->hlen.  I thought you needed
>> to at least have a check that used t->encap_hlen here in order to
>> avoid overflowing the buffer or exceeding skb_headroom, or am I
>> missing something?
>>
> Sorry, you're probably right. max_headroom seems to be an absolute
> value. mtu being calculated seems relative to what is in skbuff
> already.

The second invocation of max_headroom is an absolute value.  The first
one is used to measure if there is enough space for the headers we
will need to add.  My thought is that is why we need encap_hlen to be
added to the first case so we make sure there is enough room for the
UDP header if one is present plus the IPv6 header.

Also I just found another issue in this patch.  In ip6_tnl_dev_setup
you can probably just drop all references to "t" since you only assign
the pointer but you never actually access it.  I only noticed because
I was looking at adding support for TSO to the tunnel itself.

- Alex