netdev - RE: [PATCH] ipv6: address issue in __ip6_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] [day] [month] [year] [list]

Date:	Mon, 23 Feb 2015 14:11:58 +0000
From:	Madalin-Cristian Bucur <madalin.bucur@...escale.com>
To:	Sabrina Dubroca <sd@...asysnail.net>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"vyasevich@...il.com" <vyasevich@...il.com>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: RE: [PATCH] ipv6: address issue in __ip6_append_data

> From: Sabrina Dubroca [mailto:sd@...asysnail.net]
> Sent: Saturday, February 21, 2015 8:34 PM
> 2015-02-13, 17:30:59 +0200, Madalin Bucur wrote:
> > Hello,
> >
> > I think I've found a problem that allows generic IPv6 traffic to be
> > sent by the stack with CHECKSUM_PARTIAL to a netdevice that declares
> > NETIF_F_IPV6_CSUM. The NETIF_F_IPV6_CSUM flag is based on the
> > NETIF_F_IPV6_CSUM_BIT that is described as referring only to TCP and
> > UDP but in my test ICMPv6 frames with CHECKSUM_PARTIAL are seen:
> 
> This should be fixed by:
> 
>     bf250a1fa769 ("ipv6: Partial checksum only UDP packets")
> 
> Have you tested it?

I've sampled the tree between the problem was introduced and your fix was applied.
Next I've checked for the rt->dst.dev->features & NETIF_F_V6_CSUM that was still
there and sent the fix I had prepared locally. Your patch restricts CHECKSUM_PARTIAL
to UDP and thus solved the problem. 

> > NETIF_F_IPV6_CSUM_BIT,          /* Can checksum TCP/UDP over IPV6 */
> >
> > I've traced the issue to a recent commit that includes this check:
> >
> > 	rt->dst.dev->features & NETIF_F_V6_CSUM
> >
> > The problem with this is that NETIF_F_V6_CSUM is more than one bit:
> >
> > #define NETIF_F_V6_CSUM         (NETIF_F_GEN_CSUM |
> NETIF_F_IPV6_CSUM)
> >
> > Thus the above check should be either:
> >
> > 	(rt->dst.dev->features & NETIF_F_V6_CSUM) == NETIF_F_V6_CSUM
> 
> I think this disables HW checksumming that Vlad's patch enabled.  As
> Vlad pointed out in another similar thread [1], the two bits are
> mutually exclusive, so you can't have
> 
>     (dev->features & NETIF_F_V6_CSUM) == NETIF_F_V6_CSUM
> 
> [1] http://www.spinics.net/lists/netdev/msg316341.html
> 

I was unsure about Vlad's intention, the fact that his patch introduced the problem
made me stop at this quick explanation as the code looked suspicious.

> > or probably should use NETIF_F_HW_CSUM only:
> >
> > 	rt->dst.dev->features & NETIF_F_HW_CSUM
> 
> Maybe.  But I just tried this, and it doesn't work with qemu's e1000
> emulation (could be a driver/qemu problem, I can't check with other
> devices).

My concern was to stop receiving ICMPv6 with CHECKSUM_PARTIAL on a driver that 
declared NETIF_F_IPV6_CSUM only (not NETIF_F_HW_CSUM) so I could not have
validated this second approach on my hw.

Thank you for your fix and reply,
Madalin

PS: sorry for the late reply, I've been away last week