xfrm: Fix crash observed during device unregistration and decryption

PATCH Superseded Subash Abhinov Kasiviswanathan 5 participants 8 messages Mar 23, 2016 – Mar 24, 2016

Subsystems (3)

Networking Network drivers Networking IPsec

lore

Share & tools

lore

Subash Abhinov Kasiviswanathan

author

2016-03-23 00:29 UTC #0 lore raw

PATCH v2 1/1 xfrm: Fix crash observed during device unregistration and decryption

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

Signed-off-by Jerome Stanislaus <[email protected]>

Signed-off-by Subash Abhinov Kasiviswanathan <[email protected]>

---
 net/xfrm/xfrm_input.c | 3 +++
 1 file changed, 3 insertions(+)

net/xfrm/xfrm_input.c +0 -0

index ad7f5b3..1c4ad47 100644
@@ -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) {

Steffen Klassert

maintainer Acked-by on v2 1/1

2016-03-23 12:50 UTC #1 lore raw

Quoted 38 lines · 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.

Quoted 6 lines

v2: Make the change generic to xfrm as mentioned by Steffen and update the title to xfrm
Suggested-by Herbert Xu <[email protected]>
Signed-off-by Jerome Stanislaus <[email protected]>
Signed-off-by Subash Abhinov Kasiviswanathan <[email protected]>

Looks good.

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

Acked-by Steffen Klassert <[email protected]>

Eric Dumazet

2016-03-23 13:26 UTC #2 lore raw

In reply to Subash Abhinov Kasiviswanathan

Quoted 68 lines · 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 <[email protected]>
Signed-off-by Jerome Stanislaus <[email protected]>
Signed-off-by Subash Abhinov Kasiviswanathan <[email protected]>
---
 net/xfrm/xfrm_input.c | 3 +++
 1 file changed, 3 insertions(+)
net/xfrm/xfrm_input.c +3 -0
index ad7f5b3..1c4ad47 100644
@@ -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) {

Quoted 3 lines

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.

Herbert Xu

maintainer

2016-03-23 13:29 UTC #3 lore raw

Quoted 2 lines · Eric Dumazet

Wont this prevent device from being dismantled ?

This is only held while the crypto processing is ongoing.

Quoted 1 line

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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Eric Dumazet

2016-03-23 17:29 UTC #4 lore raw

Quoted 10 lines · Herbert Xu

Quoted 2 lines · Eric Dumazet
Wont this prevent device from being dismantled ?
This is only held while the crypto processing is ongoing.
Quoted 1 line
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.

Herbert Xu

maintainer

2016-03-24 00:45 UTC #5 lore raw

Quoted 6 lines · Eric Dumazet

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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Eric Dumazet

2016-03-24 01:40 UTC #6 lore raw

Quoted 11 lines · Herbert Xu

Quoted 6 lines · Eric Dumazet
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.

David Miller

maintainer

2016-03-24 02:08 UTC #7 lore raw

From: Eric Dumazet <[email protected]>
Date: Wed, 23 Mar 2016 18:39:57 -0700

Quoted 16 lines · Herbert Xu

Quoted 11 lines · Eric Dumazet
Quoted 6 lines
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)

Quoted 6 lines

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.