[PATCH net 1/2] xfrm6: Fix IPv6 payload_len in xfrm6_transport_finish

[PATCH net 1/2] xfrm6: Fix IPv6 payload_len in xfrm6_transport_finish

[yossiku]

From: Yossi Kuperman <yossiku@mellanox.com>

IPv6 payload length indicates the size of the payload, including any
extension headers. In xfrm6_transport_finish, ipv6_hdr(skb)->payload_len
is set to the payload size only, regardless of the presence of any
extension headers.

After ESP GRO transport mode decapsulation, ipv6_rcv trims the packet
according to the wrong payload_len, thus corrupting the packet.

Set payload_len to account for extension headers as well.

Fixes: 716062fd4c2f ("[IPSEC]: Merge most of the input path")
Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
---
 net/ipv6/xfrm6_input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
index 08a807b..3ef5d91 100644
--- a/net/ipv6/xfrm6_input.c
+++ b/net/ipv6/xfrm6_input.c
@@ -43,8 +43,8 @@ int xfrm6_transport_finish(struct sk_buff *skb, int async)
 		return 1;
 #endif
 
-	ipv6_hdr(skb)->payload_len = htons(skb->len);
 	__skb_push(skb, skb->data - skb_network_header(skb));
+	ipv6_hdr(skb)->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
 
 	if (xo && (xo->flags & XFRM_GRO)) {
 		skb_mac_header_rebuild(skb);
-- 
2.8.1

[PATCH net 2/2] esp6_offload: Fix IP6CB(skb)->nhoff for ESP GRO

[yossiku]

From: Yossi Kuperman <yossiku@mellanox.com>

IP6CB(skb)->nhoff is the offset of the nexthdr field in an IPv6
header, unless there are extension headers present, in which case
nhoff points to the nexthdr field of the last extension header.

In non-GRO code path, nhoff is set by ipv6_rcv before any XFRM code
is executed. Conversely, in GRO code path (when esp6_offload is loaded),
nhoff is not set. The following functions fail to read the correct value
and eventually the packet is dropped:

    xfrm6_transport_finish
    xfrm6_tunnel_input
    xfrm6_rcv_tnl

Set nhoff to the proper offset of nexthdr in esp6_gro_receive.

Fixes: 7785bba299a8 ("esp: Add a software GRO codepath")
Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
---
 net/ipv6/esp6_offload.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
index d950d43..f02f131 100644
--- a/net/ipv6/esp6_offload.c
+++ b/net/ipv6/esp6_offload.c
@@ -30,6 +30,25 @@
 #include <net/ipv6.h>
 #include <linux/icmpv6.h>
 
+static __u16 esp6_nexthdr_esp_offset(struct ipv6hdr *ipv6_hdr, int nhlen)
+{
+	int off = sizeof(struct ipv6hdr);
+	struct ipv6_opt_hdr *exthdr;
+
+	if (likely(ipv6_hdr->nexthdr == NEXTHDR_ESP))
+		return offsetof(struct ipv6hdr, nexthdr);
+
+	while (off < nhlen) {
+		exthdr = (void *)ipv6_hdr + off;
+		if (exthdr->nexthdr == NEXTHDR_ESP)
+			return off;
+
+		off += ipv6_optlen(exthdr);
+	}
+
+	return 0;
+}
+
 static struct sk_buff **esp6_gro_receive(struct sk_buff **head,
 					 struct sk_buff *skb)
 {
@@ -38,6 +57,7 @@ static struct sk_buff **esp6_gro_receive(struct sk_buff **head,
 	struct xfrm_state *x;
 	__be32 seq;
 	__be32 spi;
+	int nhoff;
 	int err;
 
 	skb_pull(skb, offset);
@@ -72,6 +92,11 @@ static struct sk_buff **esp6_gro_receive(struct sk_buff **head,
 
 	xo->flags |= XFRM_GRO;
 
+	nhoff = esp6_nexthdr_esp_offset(ipv6_hdr(skb), offset);
+	if (!nhoff)
+		goto out;
+
+	IP6CB(skb)->nhoff = nhoff;
 	XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip6 = NULL;
 	XFRM_SPI_SKB_CB(skb)->family = AF_INET6;
 	XFRM_SPI_SKB_CB(skb)->daddroff = offsetof(struct ipv6hdr, daddr);
-- 
2.8.1

Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in xfrm6_transport_finish

[Steffen Klassert]

On Mon, Jun 19, 2017 at 11:33:20AM +0300, yossiku@mellanox.com wrote:
> From: Yossi Kuperman <yossiku@mellanox.com>
> 
> IPv6 payload length indicates the size of the payload, including any
> extension headers. In xfrm6_transport_finish, ipv6_hdr(skb)->payload_len
> is set to the payload size only, regardless of the presence of any
> extension headers.
> 
> After ESP GRO transport mode decapsulation, ipv6_rcv trims the packet
> according to the wrong payload_len, thus corrupting the packet.
> 
> Set payload_len to account for extension headers as well.
> 
> Fixes: 716062fd4c2f ("[IPSEC]: Merge most of the input path")
> Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
> ---
>  net/ipv6/xfrm6_input.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
> index 08a807b..3ef5d91 100644
> --- a/net/ipv6/xfrm6_input.c
> +++ b/net/ipv6/xfrm6_input.c
> @@ -43,8 +43,8 @@ int xfrm6_transport_finish(struct sk_buff *skb, int async)
>  		return 1;
>  #endif
>  
> -	ipv6_hdr(skb)->payload_len = htons(skb->len);
>  	__skb_push(skb, skb->data - skb_network_header(skb));
> +	ipv6_hdr(skb)->payload_len = htons(skb->len - sizeof(struct ipv6hdr));

You mentioned that the bug happens with ESP GRO. Does this bug also
happen in the standard codepath? If not, you might better move the
above line into the 'if' section below.

>  
>  	if (xo && (xo->flags & XFRM_GRO)) {
>  		skb_mac_header_rebuild(skb);
> -- 
> 2.8.1

RE: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in xfrm6_transport_finish

[Yossi Kuperman]

> -----Original Message-----
> From: Steffen Klassert [mailto:steffen.klassert@secunet.com]
> Sent: Wednesday, June 21, 2017 8:37 AM
> To: Yossi Kuperman <yossiku@mellanox.com>
> Cc: netdev@vger.kernel.org; Herbert Xu <herbert@gondor.apana.org.au>;
> Yevgeny Kliteynik <kliteyn@mellanox.com>; Boris Pismenny
> <borisp@mellanox.com>; Ilan Tayari <ilant@mellanox.com>
> Subject: Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in
> xfrm6_transport_finish
> 
> On Mon, Jun 19, 2017 at 11:33:20AM +0300, yossiku@mellanox.com wrote:
> > From: Yossi Kuperman <yossiku@mellanox.com>
> >
> > IPv6 payload length indicates the size of the payload, including any
> > extension headers. In xfrm6_transport_finish,
> > ipv6_hdr(skb)->payload_len is set to the payload size only, regardless
> > of the presence of any extension headers.
> >
> > After ESP GRO transport mode decapsulation, ipv6_rcv trims the packet
> > according to the wrong payload_len, thus corrupting the packet.
> >
> > Set payload_len to account for extension headers as well.
> >
> > Fixes: 716062fd4c2f ("[IPSEC]: Merge most of the input path")
> > Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
> > ---
> >  net/ipv6/xfrm6_input.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c index
> > 08a807b..3ef5d91 100644
> > --- a/net/ipv6/xfrm6_input.c
> > +++ b/net/ipv6/xfrm6_input.c
> > @@ -43,8 +43,8 @@ int xfrm6_transport_finish(struct sk_buff *skb, int
> async)
> >  		return 1;
> >  #endif
> >
> > -	ipv6_hdr(skb)->payload_len = htons(skb->len);
> >  	__skb_push(skb, skb->data - skb_network_header(skb));
> > +	ipv6_hdr(skb)->payload_len = htons(skb->len - sizeof(struct
> > +ipv6hdr));
> 
> You mentioned that the bug happens with ESP GRO. Does this bug also
> happen in the standard codepath? If not, you might better move the above
> line into the 'if' section below.
> 

Yes, it happens only with ESP GRO.
Normally, ipv6_rcv happens first which trims the packet correctly and then comes xfrm6_transport_finish.
Now, with ESP GRO introduced, xfrm6_transport_finish, which comes first, modifies the packet's payload_len,
and followed by a call to ipv6_rcv which trims it wrongly.

If I understand you right, you suggest to move the new line under the "if" section and restore the removed line?

> >
> >  	if (xo && (xo->flags & XFRM_GRO)) {
> >  		skb_mac_header_rebuild(skb);
> > --
> > 2.8.1

Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in xfrm6_transport_finish

[Steffen Klassert]

On Wed, Jun 21, 2017 at 11:24:05AM +0000, Yossi Kuperman wrote:
> 
> 
> > -----Original Message-----
> > From: Steffen Klassert [mailto:steffen.klassert@secunet.com]
> > Sent: Wednesday, June 21, 2017 8:37 AM
> > To: Yossi Kuperman <yossiku@mellanox.com>
> > Cc: netdev@vger.kernel.org; Herbert Xu <herbert@gondor.apana.org.au>;
> > Yevgeny Kliteynik <kliteyn@mellanox.com>; Boris Pismenny
> > <borisp@mellanox.com>; Ilan Tayari <ilant@mellanox.com>
> > Subject: Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in
> > xfrm6_transport_finish
> > 
> > On Mon, Jun 19, 2017 at 11:33:20AM +0300, yossiku@mellanox.com wrote:
> > > From: Yossi Kuperman <yossiku@mellanox.com>
> > >
> > > IPv6 payload length indicates the size of the payload, including any
> > > extension headers. In xfrm6_transport_finish,
> > > ipv6_hdr(skb)->payload_len is set to the payload size only, regardless
> > > of the presence of any extension headers.
> > >
> > > After ESP GRO transport mode decapsulation, ipv6_rcv trims the packet
> > > according to the wrong payload_len, thus corrupting the packet.
> > >
> > > Set payload_len to account for extension headers as well.
> > >
> > > Fixes: 716062fd4c2f ("[IPSEC]: Merge most of the input path")
> > > Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
> > > ---
> > >  net/ipv6/xfrm6_input.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c index
> > > 08a807b..3ef5d91 100644
> > > --- a/net/ipv6/xfrm6_input.c
> > > +++ b/net/ipv6/xfrm6_input.c
> > > @@ -43,8 +43,8 @@ int xfrm6_transport_finish(struct sk_buff *skb, int
> > async)
> > >  		return 1;
> > >  #endif
> > >
> > > -	ipv6_hdr(skb)->payload_len = htons(skb->len);
> > >  	__skb_push(skb, skb->data - skb_network_header(skb));
> > > +	ipv6_hdr(skb)->payload_len = htons(skb->len - sizeof(struct
> > > +ipv6hdr));
> > 
> > You mentioned that the bug happens with ESP GRO. Does this bug also
> > happen in the standard codepath? If not, you might better move the above
> > line into the 'if' section below.
> > 
> 
> Yes, it happens only with ESP GRO.

In this case your Fixes tag is wrong.

> Normally, ipv6_rcv happens first which trims the packet correctly and then comes xfrm6_transport_finish.
> Now, with ESP GRO introduced, xfrm6_transport_finish, which comes first, modifies the packet's payload_len,
> and followed by a call to ipv6_rcv which trims it wrongly.
> 
> If I understand you right, you suggest to move the new line under the "if" section and restore the removed line?

I just want to make sure that the standard codepath works with this cange.
Did you test this with the standard codepath too?

RE: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in xfrm6_transport_finish

[Yossi Kuperman]

> -----Original Message-----
> From: Steffen Klassert [mailto:steffen.klassert@secunet.com]
> Sent: Wednesday, June 21, 2017 2:38 PM
> To: Yossi Kuperman <yossiku@mellanox.com>
> Cc: netdev@vger.kernel.org; Herbert Xu <herbert@gondor.apana.org.au>;
> Yevgeny Kliteynik <kliteyn@mellanox.com>; Boris Pismenny
> <borisp@mellanox.com>; Ilan Tayari <ilant@mellanox.com>
> Subject: Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in
> xfrm6_transport_finish
> 
> On Wed, Jun 21, 2017 at 11:24:05AM +0000, Yossi Kuperman wrote:
> >
> >
> > > -----Original Message-----
> > > From: Steffen Klassert [mailto:steffen.klassert@secunet.com]
> > > Sent: Wednesday, June 21, 2017 8:37 AM
> > > To: Yossi Kuperman <yossiku@mellanox.com>
> > > Cc: netdev@vger.kernel.org; Herbert Xu
> > > <herbert@gondor.apana.org.au>; Yevgeny Kliteynik
> > > <kliteyn@mellanox.com>; Boris Pismenny <borisp@mellanox.com>; Ilan
> > > Tayari <ilant@mellanox.com>
> > > Subject: Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in
> > > xfrm6_transport_finish
> > >
> > > On Mon, Jun 19, 2017 at 11:33:20AM +0300, yossiku@mellanox.com
> wrote:
> > > > From: Yossi Kuperman <yossiku@mellanox.com>
> > > >
> > > > IPv6 payload length indicates the size of the payload, including
> > > > any extension headers. In xfrm6_transport_finish,
> > > > ipv6_hdr(skb)->payload_len is set to the payload size only,
> > > > regardless of the presence of any extension headers.
> > > >
> > > > After ESP GRO transport mode decapsulation, ipv6_rcv trims the
> > > > packet according to the wrong payload_len, thus corrupting the packet.
> > > >
> > > > Set payload_len to account for extension headers as well.
> > > >
> > > > Fixes: 716062fd4c2f ("[IPSEC]: Merge most of the input path")
> > > > Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
> > > > ---
> > > >  net/ipv6/xfrm6_input.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c index
> > > > 08a807b..3ef5d91 100644
> > > > --- a/net/ipv6/xfrm6_input.c
> > > > +++ b/net/ipv6/xfrm6_input.c
> > > > @@ -43,8 +43,8 @@ int xfrm6_transport_finish(struct sk_buff *skb,
> > > > int
> > > async)
> > > >  		return 1;
> > > >  #endif
> > > >
> > > > -	ipv6_hdr(skb)->payload_len = htons(skb->len);
> > > >  	__skb_push(skb, skb->data - skb_network_header(skb));
> > > > +	ipv6_hdr(skb)->payload_len = htons(skb->len - sizeof(struct
> > > > +ipv6hdr));
> > >
> > > You mentioned that the bug happens with ESP GRO. Does this bug also
> > > happen in the standard codepath? If not, you might better move the
> > > above line into the 'if' section below.
> > >
> >
> > Yes, it happens only with ESP GRO.
> 
> In this case your Fixes tag is wrong.
> 
> > Normally, ipv6_rcv happens first which trims the packet correctly and then
> comes xfrm6_transport_finish.
> > Now, with ESP GRO introduced, xfrm6_transport_finish, which comes
> > first, modifies the packet's payload_len, and followed by a call to ipv6_rcv
> which trims it wrongly.
> >
> > If I understand you right, you suggest to move the new line under the "if"
> section and restore the removed line?
> 
> I just want to make sure that the standard codepath works with this cange.
> Did you test this with the standard codepath too?

Sure, it works when GRO is disabled (esp6_offload is not loaded).
Tested: ping, iperf (TCP stream) and injected manually created packets with extension headers.

RE: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in xfrm6_transport_finish

[Yossi Kuperman]

> -----Original Message-----
> From: Yossi Kuperman
> Sent: Wednesday, June 21, 2017 7:00 PM
> To: 'Steffen Klassert' <steffen.klassert@secunet.com>
> Cc: netdev@vger.kernel.org; Herbert Xu <herbert@gondor.apana.org.au>;
> Yevgeny Kliteynik <kliteyn@mellanox.com>; Boris Pismenny
> <borisp@mellanox.com>; Ilan Tayari <ilant@mellanox.com>
> Subject: RE: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in
> xfrm6_transport_finish
> 
> 
> 
> > -----Original Message-----
> > From: Steffen Klassert [mailto:steffen.klassert@secunet.com]
> > Sent: Wednesday, June 21, 2017 2:38 PM
> > To: Yossi Kuperman <yossiku@mellanox.com>
> > Cc: netdev@vger.kernel.org; Herbert Xu <herbert@gondor.apana.org.au>;
> > Yevgeny Kliteynik <kliteyn@mellanox.com>; Boris Pismenny
> > <borisp@mellanox.com>; Ilan Tayari <ilant@mellanox.com>
> > Subject: Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in
> > xfrm6_transport_finish
> >
> > On Wed, Jun 21, 2017 at 11:24:05AM +0000, Yossi Kuperman wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Steffen Klassert [mailto:steffen.klassert@secunet.com]
> > > > Sent: Wednesday, June 21, 2017 8:37 AM
> > > > To: Yossi Kuperman <yossiku@mellanox.com>
> > > > Cc: netdev@vger.kernel.org; Herbert Xu
> > > > <herbert@gondor.apana.org.au>; Yevgeny Kliteynik
> > > > <kliteyn@mellanox.com>; Boris Pismenny <borisp@mellanox.com>;
> Ilan
> > > > Tayari <ilant@mellanox.com>
> > > > Subject: Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in
> > > > xfrm6_transport_finish
> > > >
> > > > On Mon, Jun 19, 2017 at 11:33:20AM +0300, yossiku@mellanox.com
> > wrote:
> > > > > From: Yossi Kuperman <yossiku@mellanox.com>
> > > > >
> > > > > IPv6 payload length indicates the size of the payload, including
> > > > > any extension headers. In xfrm6_transport_finish,
> > > > > ipv6_hdr(skb)->payload_len is set to the payload size only,
> > > > > regardless of the presence of any extension headers.
> > > > >
> > > > > After ESP GRO transport mode decapsulation, ipv6_rcv trims the
> > > > > packet according to the wrong payload_len, thus corrupting the
> packet.
> > > > >
> > > > > Set payload_len to account for extension headers as well.
> > > > >
> > > > > Fixes: 716062fd4c2f ("[IPSEC]: Merge most of the input path")
> > > > > Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
> > > > > ---
> > > > >  net/ipv6/xfrm6_input.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
> > > > > index
> > > > > 08a807b..3ef5d91 100644
> > > > > --- a/net/ipv6/xfrm6_input.c
> > > > > +++ b/net/ipv6/xfrm6_input.c
> > > > > @@ -43,8 +43,8 @@ int xfrm6_transport_finish(struct sk_buff
> > > > > *skb, int
> > > > async)
> > > > >  		return 1;
> > > > >  #endif
> > > > >
> > > > > -	ipv6_hdr(skb)->payload_len = htons(skb->len);
> > > > >  	__skb_push(skb, skb->data - skb_network_header(skb));
> > > > > +	ipv6_hdr(skb)->payload_len = htons(skb->len - sizeof(struct
> > > > > +ipv6hdr));
> > > >
> > > > You mentioned that the bug happens with ESP GRO. Does this bug
> > > > also happen in the standard codepath? If not, you might better
> > > > move the above line into the 'if' section below.
> > > >
> > >
> > > Yes, it happens only with ESP GRO.
> >
> > In this case your Fixes tag is wrong.

The bug is there nonetheless, it just doesn't seem to affect anything.
I guess no one was looking inside the IP header thereafter.  

Should I use this instead: 7785bba299a8 ("esp: Add a software GRO codepath")?

> >
> > > Normally, ipv6_rcv happens first which trims the packet correctly
> > > and then
> > comes xfrm6_transport_finish.
> > > Now, with ESP GRO introduced, xfrm6_transport_finish, which comes
> > > first, modifies the packet's payload_len, and followed by a call to
> > > ipv6_rcv
> > which trims it wrongly.
> > >
> > > If I understand you right, you suggest to move the new line under the "if"
> > section and restore the removed line?
> >
> > I just want to make sure that the standard codepath works with this cange.
> > Did you test this with the standard codepath too?
> 
> Sure, it works when GRO is disabled (esp6_offload is not loaded).
> Tested: ping, iperf (TCP stream) and injected manually created packets with
> extension headers.

Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in xfrm6_transport_finish

[Steffen Klassert]

On Wed, Jun 21, 2017 at 08:38:13PM +0000, Yossi Kuperman wrote:
> 
> The bug is there nonetheless, it just doesn't seem to affect anything.
> I guess no one was looking inside the IP header thereafter.  
> 
> Should I use this instead: 7785bba299a8 ("esp: Add a software GRO codepath")?

Yes, please do this. The Fixes tag tells which stable kernels need
this patch. If we did not have problems before we added GRO, then
there is no need to backport it any further.

Thanks!