diff mbox

esp: Fix ESN generation under UDP encapsulation

Message ID 20160618050336.GA12205@gondor.apana.org.au
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Herbert Xu June 18, 2016, 5:03 a.m. UTC 
On Fri, Jun 17, 2016 at 12:24:29PM +0200, Steffen Klassert wrote:
> On Wed, Jun 15, 2016 at 12:44:54AM +0000, Blair Steven wrote:
> > The restoration is happening - but being actioned on the wrong location.
> > 
> > The destination IP address is being saved and restored, and the SPI 
> > being written directly after the destination IP address. From my 
> > understanding though, the ESN shuffling should have saved and restored 
> > the UDP source / dest ports + SPI.
> 
> Yes, looks like we copy with a wrong offset if udp encapsulation
> is used, skb_transport_header() does not point to the esp header
> in this case. Ccing Herbert, he changed this part when switching
> to the new AEAD interface with
> commit 7021b2e1cddd ("esp4: Switch to new AEAD interface").

Thanks for catching this!

I think rather than changing the transport header (which isn't
quite right because UDP still is the transport protocol), we can
just save the offset locally.  Something like this:

---8<---
Blair Steven noticed that ESN in conjunction with UDP encapsulation
is broken because we set the temporary ESP header to the wrong spot.

This patch fixes this by first of all using the right spot, i.e.,
4 bytes off the real ESP header, and then saving this information
so that after encryption we can restore it properly.

Fixes: 7021b2e1cddd ("esp4: Switch to new AEAD interface")
Reported-by: Blair Steven <Blair.Steven@alliedtelesis.co.nz>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Steffen Klassert June 20, 2016, 10:59 a.m. UTC | #1 
On Sat, Jun 18, 2016 at 01:03:36PM +0800, Herbert Xu wrote:
> On Fri, Jun 17, 2016 at 12:24:29PM +0200, Steffen Klassert wrote:
> > On Wed, Jun 15, 2016 at 12:44:54AM +0000, Blair Steven wrote:
> > > The restoration is happening - but being actioned on the wrong location.
> > > 
> > > The destination IP address is being saved and restored, and the SPI 
> > > being written directly after the destination IP address. From my 
> > > understanding though, the ESN shuffling should have saved and restored 
> > > the UDP source / dest ports + SPI.
> > 
> > Yes, looks like we copy with a wrong offset if udp encapsulation
> > is used, skb_transport_header() does not point to the esp header
> > in this case. Ccing Herbert, he changed this part when switching
> > to the new AEAD interface with
> > commit 7021b2e1cddd ("esp4: Switch to new AEAD interface").
> 
> Thanks for catching this!
> 
> I think rather than changing the transport header (which isn't
> quite right because UDP still is the transport protocol), we can
> just save the offset locally.  Something like this:
> 
> ---8<---
> Blair Steven noticed that ESN in conjunction with UDP encapsulation
> is broken because we set the temporary ESP header to the wrong spot.
> 
> This patch fixes this by first of all using the right spot, i.e.,
> 4 bytes off the real ESP header, and then saving this information
> so that after encryption we can restore it properly.
> 
> Fixes: 7021b2e1cddd ("esp4: Switch to new AEAD interface")
> Reported-by: Blair Steven <Blair.Steven@alliedtelesis.co.nz>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Looks good.
Blair could you please test this?

Thanks!
Blair Steven June 23, 2016, 4:25 a.m. UTC | #2 
This change tests okay in my setup.

Thanks very much
-Blair

On 06/20/2016 10:59 PM, Steffen Klassert wrote:
> On Sat, Jun 18, 2016 at 01:03:36PM +0800, Herbert Xu wrote:
>> On Fri, Jun 17, 2016 at 12:24:29PM +0200, Steffen Klassert wrote:
>>> On Wed, Jun 15, 2016 at 12:44:54AM +0000, Blair Steven wrote:
>>>> The restoration is happening - but being actioned on the wrong location.
>>>>
>>>> The destination IP address is being saved and restored, and the SPI
>>>> being written directly after the destination IP address. From my
>>>> understanding though, the ESN shuffling should have saved and restored
>>>> the UDP source / dest ports + SPI.
>>> Yes, looks like we copy with a wrong offset if udp encapsulation
>>> is used, skb_transport_header() does not point to the esp header
>>> in this case. Ccing Herbert, he changed this part when switching
>>> to the new AEAD interface with
>>> commit 7021b2e1cddd ("esp4: Switch to new AEAD interface").
>> Thanks for catching this!
>>
>> I think rather than changing the transport header (which isn't
>> quite right because UDP still is the transport protocol), we can
>> just save the offset locally.  Something like this:
>>
>> ---8<---
>> Blair Steven noticed that ESN in conjunction with UDP encapsulation
>> is broken because we set the temporary ESP header to the wrong spot.
>>
>> This patch fixes this by first of all using the right spot, i.e.,
>> 4 bytes off the real ESP header, and then saving this information
>> so that after encryption we can restore it properly.
>>
>> Fixes: 7021b2e1cddd ("esp4: Switch to new AEAD interface")
>> Reported-by: Blair Steven <Blair.Steven@alliedtelesis.co.nz>
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> Looks good.
> Blair could you please test this?
>
> Thanks!
Steffen Klassert June 23, 2016, 10:40 a.m. UTC | #3 
On Thu, Jun 23, 2016 at 04:25:21AM +0000, Blair Steven wrote:
> This change tests okay in my setup.
> 
> Thanks very much
> -Blair

David, can you please take this patch directly in the net tree?
This is a candidate for stable.

Acked-by: Steffen Klassert <steffen.klassert@secunet.com>
David Miller June 23, 2016, 3:52 p.m. UTC | #4 
From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Thu, 23 Jun 2016 12:40:07 +0200

> On Thu, Jun 23, 2016 at 04:25:21AM +0000, Blair Steven wrote:
>> This change tests okay in my setup.
>> 
>> Thanks very much
>> -Blair
> 
> David, can you please take this patch directly in the net tree?
> This is a candidate for stable.
> 
> Acked-by: Steffen Klassert <steffen.klassert@secunet.com>

Applied, thanks everyone.

Does the ipv6 side need the same fix?
Herbert Xu June 24, 2016, 1:52 a.m. UTC | #5 
On Thu, Jun 23, 2016 at 11:52:45AM -0400, David Miller wrote:
> 
> Does the ipv6 side need the same fix?

Last I checked IPv6 didn't do IPsec UDP-encapsulation so we're
safe for now.

Cheers,
diff mbox

Patch

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index 4779374..d95631d 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -23,6 +23,11 @@  struct esp_skb_cb {
 	void *tmp;
 };
 
+struct esp_output_extra {
+	__be32 seqhi;
+	u32 esphoff;
+};
+
 #define ESP_SKB_CB(__skb) ((struct esp_skb_cb *)&((__skb)->cb[0]))
 
 static u32 esp4_get_mtu(struct xfrm_state *x, int mtu);
@@ -35,11 +40,11 @@  static u32 esp4_get_mtu(struct xfrm_state *x, int mtu);
  *
  * TODO: Use spare space in skb for this where possible.
  */
-static void *esp_alloc_tmp(struct crypto_aead *aead, int nfrags, int seqhilen)
+static void *esp_alloc_tmp(struct crypto_aead *aead, int nfrags, int extralen)
 {
 	unsigned int len;
 
-	len = seqhilen;
+	len = extralen;
 
 	len += crypto_aead_ivsize(aead);
 
@@ -57,15 +62,16 @@  static void *esp_alloc_tmp(struct crypto_aead *aead, int nfrags, int seqhilen)
 	return kmalloc(len, GFP_ATOMIC);
 }
 
-static inline __be32 *esp_tmp_seqhi(void *tmp)
+static inline void *esp_tmp_extra(void *tmp)
 {
-	return PTR_ALIGN((__be32 *)tmp, __alignof__(__be32));
+	return PTR_ALIGN(tmp, __alignof__(struct esp_output_extra));
 }
-static inline u8 *esp_tmp_iv(struct crypto_aead *aead, void *tmp, int seqhilen)
+
+static inline u8 *esp_tmp_iv(struct crypto_aead *aead, void *tmp, int extralen)
 {
 	return crypto_aead_ivsize(aead) ?
-	       PTR_ALIGN((u8 *)tmp + seqhilen,
-			 crypto_aead_alignmask(aead) + 1) : tmp + seqhilen;
+	       PTR_ALIGN((u8 *)tmp + extralen,
+			 crypto_aead_alignmask(aead) + 1) : tmp + extralen;
 }
 
 static inline struct aead_request *esp_tmp_req(struct crypto_aead *aead, u8 *iv)
@@ -99,7 +105,7 @@  static void esp_restore_header(struct sk_buff *skb, unsigned int offset)
 {
 	struct ip_esp_hdr *esph = (void *)(skb->data + offset);
 	void *tmp = ESP_SKB_CB(skb)->tmp;
-	__be32 *seqhi = esp_tmp_seqhi(tmp);
+	__be32 *seqhi = esp_tmp_extra(tmp);
 
 	esph->seq_no = esph->spi;
 	esph->spi = *seqhi;
@@ -107,7 +113,11 @@  static void esp_restore_header(struct sk_buff *skb, unsigned int offset)
 
 static void esp_output_restore_header(struct sk_buff *skb)
 {
-	esp_restore_header(skb, skb_transport_offset(skb) - sizeof(__be32));
+	void *tmp = ESP_SKB_CB(skb)->tmp;
+	struct esp_output_extra *extra = esp_tmp_extra(tmp);
+
+	esp_restore_header(skb, skb_transport_offset(skb) + extra->esphoff -
+				sizeof(__be32));
 }
 
 static void esp_output_done_esn(struct crypto_async_request *base, int err)
@@ -121,6 +131,7 @@  static void esp_output_done_esn(struct crypto_async_request *base, int err)
 static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 {
 	int err;
+	struct esp_output_extra *extra;
 	struct ip_esp_hdr *esph;
 	struct crypto_aead *aead;
 	struct aead_request *req;
@@ -137,8 +148,7 @@  static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	int tfclen;
 	int nfrags;
 	int assoclen;
-	int seqhilen;
-	__be32 *seqhi;
+	int extralen;
 	__be64 seqno;
 
 	/* skb is pure payload to encrypt */
@@ -166,21 +176,21 @@  static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	nfrags = err;
 
 	assoclen = sizeof(*esph);
-	seqhilen = 0;
+	extralen = 0;
 
 	if (x->props.flags & XFRM_STATE_ESN) {
-		seqhilen += sizeof(__be32);
-		assoclen += seqhilen;
+		extralen += sizeof(*extra);
+		assoclen += sizeof(__be32);
 	}
 
-	tmp = esp_alloc_tmp(aead, nfrags, seqhilen);
+	tmp = esp_alloc_tmp(aead, nfrags, extralen);
 	if (!tmp) {
 		err = -ENOMEM;
 		goto error;
 	}
 
-	seqhi = esp_tmp_seqhi(tmp);
-	iv = esp_tmp_iv(aead, tmp, seqhilen);
+	extra = esp_tmp_extra(tmp);
+	iv = esp_tmp_iv(aead, tmp, extralen);
 	req = esp_tmp_req(aead, iv);
 	sg = esp_req_sg(aead, req);
 
@@ -247,8 +257,10 @@  static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	 * encryption.
 	 */
 	if ((x->props.flags & XFRM_STATE_ESN)) {
-		esph = (void *)(skb_transport_header(skb) - sizeof(__be32));
-		*seqhi = esph->spi;
+		extra->esphoff = (unsigned char *)esph -
+				 skb_transport_header(skb);
+		esph = (struct ip_esp_hdr *)((unsigned char *)esph - 4);
+		extra->seqhi = esph->spi;
 		esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
 		aead_request_set_callback(req, 0, esp_output_done_esn, skb);
 	}
@@ -445,7 +457,7 @@  static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
 		goto out;
 
 	ESP_SKB_CB(skb)->tmp = tmp;
-	seqhi = esp_tmp_seqhi(tmp);
+	seqhi = esp_tmp_extra(tmp);
 	iv = esp_tmp_iv(aead, tmp, seqhilen);
 	req = esp_tmp_req(aead, iv);
 	sg = esp_req_sg(aead, req);