Message ID | 20160618050336.GA12205@gondor.apana.org.au |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
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!
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!
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>
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?
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 --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);