[PATCH net-next] net/esp4: Fix invalid esph pointer crash

[PATCH net-next] net/esp4: Fix invalid esph pointer crash

[ilant]

From: Ilan Tayari <ilant@mellanox.com>

Both esp_output and esp_xmit take a pointer to the ESP header
and place it in esp_info struct prior to calling esp_output_head.

Inside esp_output_head, the call to esp_output_udp_encap
makes sure to update the pointer if it gets invalid.
However, if esp_output_head itself calls skb_cow_data, the
pointer is not updated and stays invalid, causing a crash
after esp_output_head returns.

Update the pointer if it becomes invalid in esp_output_head

Fixes: fca11ebde3f0 ("esp4: Reorganize esp_output")
Signed-off-by: Ilan Tayari <ilant@mellanox.com>
---
 net/ipv4/esp4.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index 7f2caf71212b..65cc02bd82bc 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -317,6 +317,7 @@ int esp_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
 	if (nfrags < 0)
 		goto out;
 	tail = skb_tail_pointer(trailer);
+	esp->esph = ip_esp_hdr(skb);
 
 skip_cow:
 	esp_output_fill_trailer(tail, esp->tfclen, esp->plen, esp->proto);
-- 
2.11.0

Re: [PATCH net-next] net/esp4: Fix invalid esph pointer crash

[David Miller]

From: <ilant@mellanox.com>
Date: Sun, 30 Apr 2017 16:34:38 +0300

> From: Ilan Tayari <ilant@mellanox.com>
> 
> Both esp_output and esp_xmit take a pointer to the ESP header
> and place it in esp_info struct prior to calling esp_output_head.
> 
> Inside esp_output_head, the call to esp_output_udp_encap
> makes sure to update the pointer if it gets invalid.
> However, if esp_output_head itself calls skb_cow_data, the
> pointer is not updated and stays invalid, causing a crash
> after esp_output_head returns.
> 
> Update the pointer if it becomes invalid in esp_output_head
> 
> Fixes: fca11ebde3f0 ("esp4: Reorganize esp_output")
> Signed-off-by: Ilan Tayari <ilant@mellanox.com>

Looks good, applied, thanks.

Re: [PATCH net-next] net/esp4: Fix invalid esph pointer crash

[Steffen Klassert]

On Sun, Apr 30, 2017 at 04:34:38PM +0300, ilant@mellanox.com wrote:
> From: Ilan Tayari <ilant@mellanox.com>
> 
> Both esp_output and esp_xmit take a pointer to the ESP header
> and place it in esp_info struct prior to calling esp_output_head.
> 
> Inside esp_output_head, the call to esp_output_udp_encap
> makes sure to update the pointer if it gets invalid.
> However, if esp_output_head itself calls skb_cow_data, the
> pointer is not updated and stays invalid, causing a crash
> after esp_output_head returns.
> 
> Update the pointer if it becomes invalid in esp_output_head
> 
> Fixes: fca11ebde3f0 ("esp4: Reorganize esp_output")
> Signed-off-by: Ilan Tayari <ilant@mellanox.com>
> ---
>  net/ipv4/esp4.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> index 7f2caf71212b..65cc02bd82bc 100644
> --- a/net/ipv4/esp4.c
> +++ b/net/ipv4/esp4.c
> @@ -317,6 +317,7 @@ int esp_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
>  	if (nfrags < 0)
>  		goto out;
>  	tail = skb_tail_pointer(trailer);
> +	esp->esph = ip_esp_hdr(skb);

This is not quite right for udpencap. It fixes the crash,
but introduces a bug that we already have in v4.11.

On udpencap the esp header has an offset to skb_transport_header,
the problem was discussed last week here:

https://lkml.org/lkml/2017/4/25/937

I plan to fix this with the patch below:

Subject: [PATCH RFC] esp4: Fix udpencap for local TCP packets.

Locally generated TCP packets are usually cloned, so we
do skb_cow_data() on this packets. After that we need to
reload the pointer to the esp header. On udpencap this
header has an offset to skb_transport_header, so take this
offset into account.

Fixes: 67d349ed603 ("net/esp4: Fix invalid esph pointer crash")
Fixes: fca11ebde3f0 ("esp4: Reorganize esp_output")
Reported-by: Don Bowman <db@donbowman.ca>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv4/esp4.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index 65cc02b..93322f8 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -248,6 +248,7 @@ int esp_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
 	u8 *tail;
 	u8 *vaddr;
 	int nfrags;
+	int esph_offset;
 	struct page *page;
 	struct sk_buff *trailer;
 	int tailen = esp->tailen;
@@ -313,11 +314,13 @@ int esp_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
 	}
 
 cow:
+	esph_offset = (unsigned char *)esp->esph - skb_transport_header(skb);
+
 	nfrags = skb_cow_data(skb, tailen, &trailer);
 	if (nfrags < 0)
 		goto out;
 	tail = skb_tail_pointer(trailer);
-	esp->esph = ip_esp_hdr(skb);
+	esp->esph = (struct ip_esp_hdr *)(skb_transport_header(skb) + esph_offset);
 
 skip_cow:
 	esp_output_fill_trailer(tail, esp->tfclen, esp->plen, esp->proto);
-- 
2.7.4

RE: [PATCH net-next] net/esp4: Fix invalid esph pointer crash

[Ilan Tayari]

> -----Original Message-----
> From: Steffen Klassert [mailto:steffen.klassert@secunet.com]
> 
> On Sun, Apr 30, 2017 at 04:34:38PM +0300, ilant@mellanox.com wrote:
> > From: Ilan Tayari <ilant@mellanox.com>
> >
> > Both esp_output and esp_xmit take a pointer to the ESP header
> > and place it in esp_info struct prior to calling esp_output_head.
> >
> > Inside esp_output_head, the call to esp_output_udp_encap
> > makes sure to update the pointer if it gets invalid.
> > However, if esp_output_head itself calls skb_cow_data, the
> > pointer is not updated and stays invalid, causing a crash
> > after esp_output_head returns.
> >
> > Update the pointer if it becomes invalid in esp_output_head
> >
> > Fixes: fca11ebde3f0 ("esp4: Reorganize esp_output")
> > Signed-off-by: Ilan Tayari <ilant@mellanox.com>
> > ---
> >  net/ipv4/esp4.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> > index 7f2caf71212b..65cc02bd82bc 100644
> > --- a/net/ipv4/esp4.c
> > +++ b/net/ipv4/esp4.c
> > @@ -317,6 +317,7 @@ int esp_output_head(struct xfrm_state *x, struct
> sk_buff *skb, struct esp_info *
> >  	if (nfrags < 0)
> >  		goto out;
> >  	tail = skb_tail_pointer(trailer);
> > +	esp->esph = ip_esp_hdr(skb);
> 
> This is not quite right for udpencap. It fixes the crash,
> but introduces a bug that we already have in v4.11.
> 
> On udpencap the esp header has an offset to skb_transport_header,
> the problem was discussed last week here:
> 
> https://lkml.org/lkml/2017/4/25/937
> 
> I plan to fix this with the patch below:
> 
> Subject: [PATCH RFC] esp4: Fix udpencap for local TCP packets.
> 

This patch works for me.
I don't have udp-encap test facilities, though (yet!).

Ilan.