netdev - RE: [PATCH 07/16] esp4: Reorganize esp

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

Message-ID: <AM4PR0501MB1940C8509F15D99381D19E2CDB110@AM4PR0501MB1940.eurprd05.prod.outlook.com>
Date:   Wed, 26 Apr 2017 11:52:47 +0000
From:   Ilan Tayari <ilant@...lanox.com>
To:     Steffen Klassert <steffen.klassert@...unet.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH 07/16] esp4: Reorganize esp_output

> -----Original Message-----
> From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org]
> Subject: [PATCH 07/16] esp4: Reorganize esp_output
> 
> We need a fallback for ESP at layer 2, so split esp_output into generic
> functions that can be used at layer 3 and layer 2 and use them in
> esp_output. We also add esp_xmit which is used for the layer 2 fallback.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@...unet.com>
> ---
>  include/net/esp.h       |  16 +++
>  net/ipv4/esp4.c         | 341 ++++++++++++++++++++++++++-----------------
> -----
>  net/ipv4/esp4_offload.c | 102 +++++++++++++++
>  3 files changed, 301 insertions(+), 158 deletions(-)
> 

Steffen,

...

In the two places marked below:

> +static int esp_output(struct xfrm_state *x, struct sk_buff *skb) {
> +	int alen;
> +	int blksize;
> +	struct ip_esp_hdr *esph;
> +	struct crypto_aead *aead;
> +	struct esp_info esp;
> +
> +	esp.inplace = true;
> +
> +	esp.proto = *skb_mac_header(skb);
> +	*skb_mac_header(skb) = IPPROTO_ESP;
> +
> +	/* skb is pure payload to encrypt */
> +
> +	aead = x->data;
> +	alen = crypto_aead_authsize(aead);
> +
> +	esp.tfclen = 0;
> +	if (x->tfcpad) {
> +		struct xfrm_dst *dst = (struct xfrm_dst *)skb_dst(skb);
> +		u32 padto;
> +
> +		padto = min(x->tfcpad, esp4_get_mtu(x, dst-
> >child_mtu_cached));
> +		if (skb->len < padto)
> +			esp.tfclen = padto - skb->len;
> +	}
> +	blksize = ALIGN(crypto_aead_blocksize(aead), 4);
> +	esp.clen = ALIGN(skb->len + 2 + esp.tfclen, blksize);
> +	esp.plen = esp.clen - skb->len - esp.tfclen;
> +	esp.tailen = esp.tfclen + esp.plen + alen;
> +
> +	esp.esph = ip_esp_hdr(skb);
> +
> +	esp.nfrags = esp_output_head(x, skb, &esp);
> +	if (esp.nfrags < 0)
> +		return esp.nfrags;
> +
> +	esph = esp.esph;

Here

> +	esph->spi = x->id.spi;
> +
> +	esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);
> +	esp.seqno = cpu_to_be64(XFRM_SKB_CB(skb)->seq.output.low +
> +				 ((u64)XFRM_SKB_CB(skb)->seq.output.hi << 32));
> +
> +	skb_push(skb, -skb_network_offset(skb));
> +
> +	return esp_output_tail(x, skb, &esp);
> +}
> 

...

> +static int esp_xmit(struct xfrm_state *x, struct sk_buff *skb,
> +netdev_features_t features) {
> +	int err;
> +	int alen;
> +	int blksize;
> +	struct xfrm_offload *xo;
> +	struct ip_esp_hdr *esph;
> +	struct crypto_aead *aead;
> +	struct esp_info esp;
> +	bool hw_offload = true;
> +
> +	esp.inplace = true;
> +
> +	xo = xfrm_offload(skb);
> +
> +	if (!xo)
> +		return -EINVAL;
> +
> +	if (!(features & NETIF_F_HW_ESP) ||
> +	    (x->xso.offload_handle &&  x->xso.dev != skb->dev)) {
> +		xo->flags |= CRYPTO_FALLBACK;
> +		hw_offload = false;
> +	}
> +
> +	esp.proto = xo->proto;
> +
> +	/* skb is pure payload to encrypt */
> +
> +	aead = x->data;
> +	alen = crypto_aead_authsize(aead);
> +
> +	esp.tfclen = 0;
> +	/* XXX: Add support for tfc padding here. */
> +
> +	blksize = ALIGN(crypto_aead_blocksize(aead), 4);
> +	esp.clen = ALIGN(skb->len + 2 + esp.tfclen, blksize);
> +	esp.plen = esp.clen - skb->len - esp.tfclen;
> +	esp.tailen = esp.tfclen + esp.plen + alen;
> +
> +	esp.esph = ip_esp_hdr(skb);
> +
> +
> +	if (!hw_offload || (hw_offload && !skb_is_gso(skb))) {
> +		esp.nfrags = esp_output_head(x, skb, &esp);
> +		if (esp.nfrags < 0)
> +			return esp.nfrags;
> +	}
> +
> +	esph = esp.esph;

And here.

esp_output_head() might do an skb_cow, which then invalidates the esp.esph pointer and causes a crash later on.
I would expect the ip_esp_hdr() call to be after the esp_output_head() call.

But it seems like this pointer was saved here around the call to esp_output_head() on purpose.
Is that really so? 

Also, esp6/esp6_offload don't make use of esp_info.esph
Only esp_output_tail() uses it, and could have done everything it does without it.
So maybe it's un-needed?

I am still testing a fix patch for the crash, there may be also something similar on the RX path, though.

> +	esph->spi = x->id.spi;
> +
> +	skb_push(skb, -skb_network_offset(skb));
> +
> +	if (xo->flags & XFRM_GSO_SEGMENT) {
> +		esph->seq_no = htonl(xo->seq.low);
> +	} else {
> +		ip_hdr(skb)->tot_len = htons(skb->len);
> +		ip_send_check(ip_hdr(skb));
> +	}
> +
> +	if (hw_offload)
> +		return 0;
> +
> +	esp.seqno = cpu_to_be64(xo->seq.low + ((u64)xo->seq.hi << 32));
> +
> +	err = esp_output_tail(x, skb, &esp);
> +	if (err < 0)
> +		return err;
> +
> +	secpath_reset(skb);
> +
> +	return 0;
> +}
> +