[PATCH 0/2] AH fixes for asynchronous hash algorithms.

[PATCH 0/2] AH fixes for asynchronous hash algorithms.

[Nick Bowler]

Here are two fixes for AH when using an asynchronous hmac driver.  Both
are -stable candidates as these problems appear to have been present
since AH was converted to use ahash way back in 2.6.33.

These code paths are not exercised when using the default software hash
implementations which do not use the ahash callbacks, but the issues can be
reproduced by using cryptd to create an asynchronous hash algorithm for
testing.

This driver could probably do with some cleanups to reduce the code
duplication (and thus test coverage) between the asynchronous callbacks
and synchronous code paths, which should help avoid these kind of
problems in the future.  These code paths apparently do not see a
lot of testing.  But that's for a later patch series.

Nick Bowler (2):
  ah: Correctly pass error codes in ahash output callback.
  ah: Read nexthdr value before overwriting it in ahash input callback.

 net/ipv4/ah4.c |    6 ++----
 net/ipv6/ah6.c |    6 ++----
 2 files changed, 4 insertions(+), 8 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] ah: Correctly pass error codes in ahash output callback.

[Nick Bowler]

The AH4/6 ahash output callbacks pass nexthdr to xfrm_output_resume
instead of the error code.  This appears to be a copy+paste error from
the input case, where nexthdr is expected.  This causes the driver to
continuously add AH headers to the datagram until either an allocation
fails and the packet is dropped or the ahash driver hits a synchronous
fallback and the resulting monstrosity is transmitted.

Correct this issue by simply passing the error code unadulterated.

Signed-off-by: Nick Bowler <nbowler@elliptictech.com>
---
 net/ipv4/ah4.c |    2 --
 net/ipv6/ah6.c |    2 --
 2 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index c1f4154..33ca186 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -136,8 +136,6 @@ static void ah_output_done(struct crypto_async_request *base, int err)
 		memcpy(top_iph+1, iph+1, top_iph->ihl*4 - sizeof(struct iphdr));
 	}
 
-	err = ah->nexthdr;
-
 	kfree(AH_SKB_CB(skb)->tmp);
 	xfrm_output_resume(skb, err);
 }
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 2195ae6..ede4d9d 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -324,8 +324,6 @@ static void ah6_output_done(struct crypto_async_request *base, int err)
 #endif
 	}
 
-	err = ah->nexthdr;
-
 	kfree(AH_SKB_CB(skb)->tmp);
 	xfrm_output_resume(skb, err);
 }
-- 
1.7.3.4

[PATCH 2/2] ah: Read nexthdr value before overwriting it in ahash input callback.

[Nick Bowler]

The AH4/6 ahash input callbacks read out the nexthdr field from the AH
header *after* they overwrite that header.  This is obviously not going
to end well.  Fix it up.

Signed-off-by: Nick Bowler <nbowler@elliptictech.com>
---
 net/ipv4/ah4.c |    4 ++--
 net/ipv6/ah6.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 33ca186..c7056b2 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -262,12 +262,12 @@ static void ah_input_done(struct crypto_async_request *base, int err)
 	if (err)
 		goto out;
 
+	err = ah->nexthdr;
+
 	skb->network_header += ah_hlen;
 	memcpy(skb_network_header(skb), work_iph, ihl);
 	__skb_pull(skb, ah_hlen + ihl);
 	skb_set_transport_header(skb, -ihl);
-
-	err = ah->nexthdr;
 out:
 	kfree(AH_SKB_CB(skb)->tmp);
 	xfrm_input_resume(skb, err);
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index ede4d9d..7a33aaa 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -464,12 +464,12 @@ static void ah6_input_done(struct crypto_async_request *base, int err)
 	if (err)
 		goto out;
 
+	err = ah->nexthdr;
+
 	skb->network_header += ah_hlen;
 	memcpy(skb_network_header(skb), work_iph, hdr_len);
 	__skb_pull(skb, ah_hlen + hdr_len);
 	skb_set_transport_header(skb, -hdr_len);
-
-	err = ah->nexthdr;
 out:
 	kfree(AH_SKB_CB(skb)->tmp);
 	xfrm_input_resume(skb, err);
-- 
1.7.3.4

Re: [PATCH 0/2] AH fixes for asynchronous hash algorithms.

[David Miller]

From: Nick Bowler <nbowler@elliptictech.com>
Date: Tue,  8 Nov 2011 17:12:43 -0500

> Here are two fixes for AH when using an asynchronous hmac driver.  Both
> are -stable candidates as these problems appear to have been present
> since AH was converted to use ahash way back in 2.6.33.
> 
> These code paths are not exercised when using the default software hash
> implementations which do not use the ahash callbacks, but the issues can be
> reproduced by using cryptd to create an asynchronous hash algorithm for
> testing.
> 
> This driver could probably do with some cleanups to reduce the code
> duplication (and thus test coverage) between the asynchronous callbacks
> and synchronous code paths, which should help avoid these kind of
> problems in the future.  These code paths apparently do not see a
> lot of testing.  But that's for a later patch series.
> 
> Nick Bowler (2):
>   ah: Correctly pass error codes in ahash output callback.
>   ah: Read nexthdr value before overwriting it in ahash input callback.

Thanks a lot for these bug fixes Nick, both applied.

Also queued up for -stable.