[PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption

[PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption

[Subash Abhinov Kasiviswanathan]

A crash is observed when a decrypted packet is processed in receive
path. get_rps_cpus() tries to dereference the skb->dev fields but it
appears that the device is freed from the poison pattern.

[<ffffffc000af58ec>] get_rps_cpu+0x94/0x2f0
[<ffffffc000af5f94>] netif_rx_internal+0x140/0x1cc
[<ffffffc000af6094>] netif_rx+0x74/0x94
[<ffffffc000bc0b6c>] xfrm_input+0x754/0x7d0
[<ffffffc000bc0bf8>] xfrm_input_resume+0x10/0x1c
[<ffffffc000ba6eb8>] esp_input_done+0x20/0x30
[<ffffffc0000b64c8>] process_one_work+0x244/0x3fc
[<ffffffc0000b7324>] worker_thread+0x2f8/0x418
[<ffffffc0000bb40c>] kthread+0xe0/0xec

-013|get_rps_cpu(
     |    dev = 0xFFFFFFC08B688000,
     |    skb = 0xFFFFFFC0C76AAC00 -> (
     |      dev = 0xFFFFFFC08B688000 -> (
     |        name =
"......................................................
     |        name_hlist = (next = 0xAAAAAAAAAAAAAAAA, pprev =
0xAAAAAAAAAAA

Following are the sequence of events observed -

- Encrypted packet in receive path from netdevice is queued
- Encrypted packet queued for decryption (asynchronous)
- Netdevice brought down and freed
- Packet is decrypted and returned through callback in esp_input_done
- Packet is queued again for process in network stack using netif_rx

Since the device appears to have been freed, the dereference of
skb->dev in get_rps_cpus() leads to an unhandled page fault
exception.

Fix this by holding on to device reference when queueing packets
asynchronously and releasing the reference on call back return.

v2: Make the change generic to xfrm as mentioned by Steffen and
update the title to xfrm

Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Jerome Stanislaus <jeromes@codeaurora.org>
Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 net/xfrm/xfrm_input.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index ad7f5b3..1c4ad47 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -292,12 +292,15 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32
spi, int encap_type)
 		XFRM_SKB_CB(skb)->seq.input.hi = seq_hi;
 
 		skb_dst_force(skb);
+		dev_hold(skb->dev);
 
 		nexthdr = x->type->input(x, skb);
 
 		if (nexthdr == -EINPROGRESS)
 			return 0;
 resume:
+		dev_put(skb->dev);
+
 		spin_lock(&x->lock);
 		if (nexthdr <= 0) {
 			if (nexthdr == -EBADMSG) {

Re: [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption

[Steffen Klassert]

On Tue, Mar 22, 2016 at 06:29:48PM -0600, Subash Abhinov Kasiviswanathan wrote:
> A crash is observed when a decrypted packet is processed in receive
> path. get_rps_cpus() tries to dereference the skb->dev fields but it
> appears that the device is freed from the poison pattern.
> 
> [<ffffffc000af58ec>] get_rps_cpu+0x94/0x2f0
> [<ffffffc000af5f94>] netif_rx_internal+0x140/0x1cc
> [<ffffffc000af6094>] netif_rx+0x74/0x94
> [<ffffffc000bc0b6c>] xfrm_input+0x754/0x7d0
> [<ffffffc000bc0bf8>] xfrm_input_resume+0x10/0x1c
> [<ffffffc000ba6eb8>] esp_input_done+0x20/0x30
> [<ffffffc0000b64c8>] process_one_work+0x244/0x3fc
> [<ffffffc0000b7324>] worker_thread+0x2f8/0x418
> [<ffffffc0000bb40c>] kthread+0xe0/0xec
> 
> -013|get_rps_cpu(
>      |    dev = 0xFFFFFFC08B688000,
>      |    skb = 0xFFFFFFC0C76AAC00 -> (
>      |      dev = 0xFFFFFFC08B688000 -> (
>      |        name =
> "......................................................
>      |        name_hlist = (next = 0xAAAAAAAAAAAAAAAA, pprev =
> 0xAAAAAAAAAAA
> 
> Following are the sequence of events observed -
> 
> - Encrypted packet in receive path from netdevice is queued
> - Encrypted packet queued for decryption (asynchronous)
> - Netdevice brought down and freed
> - Packet is decrypted and returned through callback in esp_input_done
> - Packet is queued again for process in network stack using netif_rx
> 
> Since the device appears to have been freed, the dereference of
> skb->dev in get_rps_cpus() leads to an unhandled page fault
> exception.
> 
> Fix this by holding on to device reference when queueing packets
> asynchronously and releasing the reference on call back return.
> 
> v2: Make the change generic to xfrm as mentioned by Steffen and
> update the title to xfrm
> 
> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Jerome Stanislaus <jeromes@codeaurora.org>
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>

Looks good.

David, in case you want to take it directly into the net tree:

Acked-by: Steffen Klassert <steffen.klassert@secunet.com>

Re: [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption

[Eric Dumazet]

On Tue, 2016-03-22 at 18:29 -0600, Subash Abhinov Kasiviswanathan wrote:
> A crash is observed when a decrypted packet is processed in receive
> path. get_rps_cpus() tries to dereference the skb->dev fields but it
> appears that the device is freed from the poison pattern.
> 
> [<ffffffc000af58ec>] get_rps_cpu+0x94/0x2f0
> [<ffffffc000af5f94>] netif_rx_internal+0x140/0x1cc
> [<ffffffc000af6094>] netif_rx+0x74/0x94
> [<ffffffc000bc0b6c>] xfrm_input+0x754/0x7d0
> [<ffffffc000bc0bf8>] xfrm_input_resume+0x10/0x1c
> [<ffffffc000ba6eb8>] esp_input_done+0x20/0x30
> [<ffffffc0000b64c8>] process_one_work+0x244/0x3fc
> [<ffffffc0000b7324>] worker_thread+0x2f8/0x418
> [<ffffffc0000bb40c>] kthread+0xe0/0xec
> 
> -013|get_rps_cpu(
>      |    dev = 0xFFFFFFC08B688000,
>      |    skb = 0xFFFFFFC0C76AAC00 -> (
>      |      dev = 0xFFFFFFC08B688000 -> (
>      |        name =
> "......................................................
>      |        name_hlist = (next = 0xAAAAAAAAAAAAAAAA, pprev =
> 0xAAAAAAAAAAA
> 
> Following are the sequence of events observed -
> 
> - Encrypted packet in receive path from netdevice is queued
> - Encrypted packet queued for decryption (asynchronous)
> - Netdevice brought down and freed
> - Packet is decrypted and returned through callback in esp_input_done
> - Packet is queued again for process in network stack using netif_rx
> 
> Since the device appears to have been freed, the dereference of
> skb->dev in get_rps_cpus() leads to an unhandled page fault
> exception.
> 
> Fix this by holding on to device reference when queueing packets
> asynchronously and releasing the reference on call back return.
> 
> v2: Make the change generic to xfrm as mentioned by Steffen and
> update the title to xfrm
> 
> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Jerome Stanislaus <jeromes@codeaurora.org>
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> ---
>  net/xfrm/xfrm_input.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index ad7f5b3..1c4ad47 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -292,12 +292,15 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32
> spi, int encap_type)
>  		XFRM_SKB_CB(skb)->seq.input.hi = seq_hi;
>  
>  		skb_dst_force(skb);
> +		dev_hold(skb->dev);
>  
>  		nexthdr = x->type->input(x, skb);
>  
>  		if (nexthdr == -EINPROGRESS)
>  			return 0;
>  resume:
> +		dev_put(skb->dev);
> +
>  		spin_lock(&x->lock);
>  		if (nexthdr <= 0) {
>  			if (nexthdr == -EBADMSG) {
> --
> 

Wont this prevent device from being dismantled ?

Where is this xfrm queue purged at device dismantle ?

dev_put() is probably missing, if you add a dev_hold() for every packet
in it.

Re: [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption

[Herbert Xu]

On Wed, Mar 23, 2016 at 06:25:47AM -0700, Eric Dumazet wrote:
>
> Wont this prevent device from being dismantled ?

This is only held while the crypto processing is ongoing.

> Where is this xfrm queue purged at device dismantle ?

There is no way to cancel an ongoing crypto processing so you'll
just have to wait it out.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption

[Eric Dumazet]

On Wed, 2016-03-23 at 21:29 +0800, Herbert Xu wrote:
> On Wed, Mar 23, 2016 at 06:25:47AM -0700, Eric Dumazet wrote:
> >
> > Wont this prevent device from being dismantled ?
> 
> This is only held while the crypto processing is ongoing.
> 
> > Where is this xfrm queue purged at device dismantle ?
> 
> There is no way to cancel an ongoing crypto processing so you'll
> just have to wait it out.

OK, but before calling netif_rx() are we properly testing dev->flags
IFF_UP status ?

Otherwise, we still allow packets being queued after flush_backlog() had
been called.

Thanks.

Re: [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption

[Herbert Xu]

On Wed, Mar 23, 2016 at 10:29:25AM -0700, Eric Dumazet wrote:
>
> OK, but before calling netif_rx() are we properly testing dev->flags
> IFF_UP status ?
> 
> Otherwise, we still allow packets being queued after flush_backlog() had
> been called.

That's the first thing enqueue_to_backlog tests.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption

[Eric Dumazet]

On Thu, 2016-03-24 at 08:45 +0800, Herbert Xu wrote:
> On Wed, Mar 23, 2016 at 10:29:25AM -0700, Eric Dumazet wrote:
> >
> > OK, but before calling netif_rx() are we properly testing dev->flags
> > IFF_UP status ?
> > 
> > Otherwise, we still allow packets being queued after flush_backlog() had
> > been called.
> 
> That's the first thing enqueue_to_backlog tests.
> 
> Cheers,

Seems to be very recent stuff ( commit
e9e4dd3267d0c5234c5c0f47440456b10875dec9 in linux-4.2)

In the old days the test was done in callers, since in most cases NIC
drivers do not need it.

Lets make sure this was backported to all stable trees.

And then we probably can cleanup some callers as well.

Re: [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 23 Mar 2016 18:39:57 -0700

> On Thu, 2016-03-24 at 08:45 +0800, Herbert Xu wrote:
>> On Wed, Mar 23, 2016 at 10:29:25AM -0700, Eric Dumazet wrote:
>> >
>> > OK, but before calling netif_rx() are we properly testing dev->flags
>> > IFF_UP status ?
>> > 
>> > Otherwise, we still allow packets being queued after flush_backlog() had
>> > been called.
>> 
>> That's the first thing enqueue_to_backlog tests.
>> 
>> Cheers,
> 
> Seems to be very recent stuff ( commit
> e9e4dd3267d0c5234c5c0f47440456b10875dec9 in linux-4.2)
> 
> In the old days the test was done in callers, since in most cases NIC
> drivers do not need it.
> 
> Lets make sure this was backported to all stable trees.
> 
> And then we probably can cleanup some callers as well.

Anyways this patch needs to be redone because it is corrupted by the
submitter's email client.

I'll queue it up and make sure e9e4dd3267d0c5234c5c0f47440456b10875dec9
ends up in -stable where needed.

Thanks.