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
| ||
|
Date: Tue, 21 Sep 2010 08:16:27 +0200 From: Eric Dumazet <eric.dumazet@...il.com> To: Nick Bowler <nbowler@...iptictech.com> Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net> Subject: [PATCH] ip : take care of last fragment in ip_append_data Le lundi 20 septembre 2010 à 23:31 +0200, Eric Dumazet a écrit : > OK, I found a bug in ip_fragment() and ip6_fragment() > > In case slow_path is hit, we have a truesize mismatch > > Could you try following patch ? > > Thanks ! > > [PATCH] ip : fix truesize mismatch in ip fragmentation > > We should not set frag->destructor to sock_wkfree() until we are sure we > dont hit slow path in ip_fragment(). Or we risk uncharging > frag->truesize twice, and in the end, having negative socket > sk_wmem_alloc counter, or even freeing socket sooner than expected. > > Many thanks to Nick Bowler, who provided a very clean bug report and > test programs. > > While Nick bisection pointed to commit 2b85a34e911bf483 (net: No more > expensive sock_hold()/sock_put() on each tx), underlying bug is older. > > Reported-and-bisected-by: Nick Bowler <nbowler@...iptictech.com> > Signed-off-by: Eric Dumazet <eric.dumazet@...il.com> > --- > net/ipv4/ip_output.c | 8 ++++---- > net/ipv6/ip6_output.c | 10 +++++----- > 2 files changed, 9 insertions(+), 9 deletions(-) This previous patch works for me. I also cooked following fix, to not enter slow path on some lengthes (MTU + N*(MTU-20)) [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> --- net/ipv4/ip_output.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index 04b6989..dfd9a0d 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -927,16 +927,19 @@ alloc_new_skb: !(rt->dst.dev->features&NETIF_F_SG)) alloclen = mtu; else - alloclen = datalen + fragheaderlen; + alloclen = fraglen; /* The last fragment gets additional space at tail. * Note, with MSG_MORE we overallocate on fragments, * because we have no idea what fragment will be * the last. */ - if (datalen == length + fraggap) + if (datalen == length + fraggap) { alloclen += rt->dst.trailer_len; - + /* make sure mtu is not reached */ + if (datalen > mtu - fragheaderlen - rt->dst.trailer_len) + datalen -= ALIGN(rt->dst.trailer_len, 8); + } if (transhdrlen) { skb = sock_alloc_send_skb(sk, alloclen + hh_len + 15, -- 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
Powered by blists - more mailing lists