netdev - Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets

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-next>] [day] [month] [year] [list]

Message-ID: <20170418090919.GD30104@secunet.com>
Date:   Tue, 18 Apr 2017 11:09:19 +0200
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Ansis Atteka <ansisatteka@...il.com>
CC:     Ansis Atteka <aatteka@....org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case
 before encrypting packets

On Thu, Apr 13, 2017 at 07:45:08PM -0700, Ansis Atteka wrote:
> On 11 April 2017 at 00:07, Steffen Klassert <steffen.klassert@...unet.com>
> wrote:
> >
> > What's wrong with the checksum provided by the GSO layer and
> > why we have to do this unconditionally here?
> >
> 
> I believe with "GSO layer" you meant the skb_gso_segment() function
> invocation from xfrm_output_gso()?
> 
> If so, then the problem with that is that the list of the skb's returned by
> that function could be in CHECKSUM_PARTIAL state if skb came from a UDP
> tunnel such as Geneve:

This should not happen. We don't announce checksum capabilities,
so the GSO layer should generate the full checksum.

__skb_udp_tunnel_segment() and udp4_ufo_fragment() add the
NETIF_F_HW_CSUM flag to features. This is certainly wrong
if the packet undergoes an IPsec transformation.

I don't have a testcase for this, so not sure if this is
your problem. Could you try the (untested) patch below?

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index b2be1d9..cc0c89c 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -74,7 +74,7 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
 	 * outer one so strip the existing checksum feature flags and
 	 * instead set the flag based on our outer checksum offload value.
 	 */
-	if (remcsum || ufo) {
+	if ((remcsum || ufo) && !(skb_dst(skb) && dst_xfrm(skb_dst(skb)))) {
 		features &= ~NETIF_F_CSUM_MASK;
 		if (!need_csum || offload_csum)
 			features |= NETIF_F_HW_CSUM;
@@ -238,7 +238,7 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 	 * due to the fact that we have already done the checksum in
 	 * software prior to segmenting the frame.
 	 */
-	if (!skb->encap_hdr_csum)
+	if (!skb->encap_hdr_csum && !(skb_dst(skb) && dst_xfrm(skb_dst(skb))))
 		features |= NETIF_F_HW_CSUM;
 
 	/* Fragment the skb. IP headers of the fragments are updated in