netdev - Re: [PATCH] ip : take care of last fragment in ip_append

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: <1285130646.6378.45.camel@edumazet-laptop>
Date:	Wed, 22 Sep 2010 06:44:06 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	nbowler@...iptictech.com, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH] ip : take care of last fragment in ip_append_data

Le mardi 21 septembre 2010 à 16:38 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@...il.com>
> Date: Tue, 21 Sep 2010 08:16:27 +0200
> 
> > [PATCH] ip : take care of last fragment in ip_append_data
> > 
> > While investigating a bit, I found ip_fragment() slow path was taken
> > because ip_append_data() provides following layout for a send(MTU +
> > N*(MTU - 20)) syscall :
> > 
> > - one skb with 1500 (mtu) bytes
> > - N fragments of 1480 (mtu-20) bytes (before adding IP header)
> > last fragment gets 17 bytes of trail data because of following bit:
> > 
> > 	if (datalen == length + fraggap)
> > 		alloclen += rt->dst.trailer_len;
> > 
> > Then esp4 adds 16 bytes of data (while trailer_len is 17... hmm...
> > another bug ?)
> > 
> > In ip_fragment(), we notice last fragment is too big (1496 + 20) > mtu,
> > so we take slow path, building another skb chain.
> > 
> > In order to avoid taking slow path, we should correct ip_append_data()
> > to make sure last fragment has real trail space, under mtu...
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
> 
> This patch largely looks fine, but:
> 
> 1) I want to find out where that "17" tailer_len comes from before
>    applying this, that doesn't make any sense.
> 
> 2) Even with #1 addressed, this function is tricky so I want to review
>    this patch some more.



The "17" (instead of probable 16 need) comes from :

net/ipv4/esp4.c line 599 :

x->props.trailer_len = align + 1 + crypto_aead_authsize(esp->aead);

In my Nick ipsec script case, 
crypto_aead_blocksize(aead) = 16, 
crypto_aead_authsize(esp->aead) = 0

-> align = 16
trailer_len = 16 + 1 + 0;

I am not sure we need the "+ 1", but I know nothing about this stuff.

Same in net/ipv6/esp6.c ?


Anyway the last frag problem is for packets with lengths :
 MTU + N*(MTU - 20) + LAST

LAST being from [(MTU - trailer_len) ... MTU], not only MTU as I wrote
in changelog



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html