[PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll

Subsystems: networking drivers, the rest, usb networking drivers

STALE3429d

11 messages, 4 authors, 2017-03-21 · open the first message on its own page

[PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll

From: Petr Vorel <hidden>
Date: 2017-03-13 12:47:37

commit 7489bdadb7d1 (r8152: check rx after napi is enabled) causes null
pointer dereference when using device as under root:

 # rmmod r8152 # or lsusb -v
NOHZ: local_softirq_pending 08
BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: r8152_poll+0x125/0x570 [r8152]
PGD 89b4cf067
PUD 898ff2067
PMD 0
Oops: 0002 [#1] PREEMPT SMP

Signed-off-by: Petr Vorel <redacted>
---
NOTE: This is just a workaround, I suppose, there is better way how to fix that
(which allows keeping scheduling the napi for rx after napi_enable()).
---
 drivers/net/usb/r8152.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 986243c932cc..79c665a89a47 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -3703,8 +3703,6 @@ static int rtl8152_resume(struct usb_interface *intf)
 			napi_enable(&tp->napi);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			smp_mb__after_atomic();
-			if (!list_empty(&tp->rx_done))
-				napi_schedule(&tp->napi);
 		} else {
 			tp->rtl_ops.up(tp);
 			netif_carrier_off(tp->netdev);

-- 
2.12.0

Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll

From: Eric Dumazet <hidden>
Date: 2017-03-13 13:18:06

On Mon, 2017-03-13 at 13:47 +0100, Petr Vorel wrote:

quoted hunk

commit 7489bdadb7d1 (r8152: check rx after napi is enabled) causes null
pointer dereference when using device as under root:

 # rmmod r8152 # or lsusb -v
NOHZ: local_softirq_pending 08
BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: r8152_poll+0x125/0x570 [r8152]
PGD 89b4cf067
PUD 898ff2067
PMD 0
Oops: 0002 [#1] PREEMPT SMP

Signed-off-by: Petr Vorel <redacted>
---
NOTE: This is just a workaround, I suppose, there is better way how to fix that
(which allows keeping scheduling the napi for rx after napi_enable()).
---
 drivers/net/usb/r8152.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 986243c932cc..79c665a89a47 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -3703,8 +3703,6 @@ static int rtl8152_resume(struct usb_interface *intf)
 			napi_enable(&tp->napi);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			smp_mb__after_atomic();
-			if (!list_empty(&tp->rx_done))
-				napi_schedule(&tp->napi);
 		} else {
 			tp->rtl_ops.up(tp);
 			netif_carrier_off(tp->netdev);


The proper work around is to enclose the napi_schedule() in a
local_bh_enable()/local_bh_disable()  pair.

Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll

From: Eric Dumazet <hidden>
Date: 2017-03-13 13:26:06

On Mon, 2017-03-13 at 06:18 -0700, Eric Dumazet wrote:

On Mon, 2017-03-13 at 13:47 +0100, Petr Vorel wrote:

quoted

commit 7489bdadb7d1 (r8152: check rx after napi is enabled) causes null
pointer dereference when using device as under root:

 # rmmod r8152 # or lsusb -v
NOHZ: local_softirq_pending 08
BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: r8152_poll+0x125/0x570 [r8152]
PGD 89b4cf067
PUD 898ff2067
PMD 0
Oops: 0002 [#1] PREEMPT SMP

Signed-off-by: Petr Vorel <redacted>
---
NOTE: This is just a workaround, I suppose, there is better way how to fix that
(which allows keeping scheduling the napi for rx after napi_enable()).
---
 drivers/net/usb/r8152.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 986243c932cc..79c665a89a47 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -3703,8 +3703,6 @@ static int rtl8152_resume(struct usb_interface *intf)
 			napi_enable(&tp->napi);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			smp_mb__after_atomic();
-			if (!list_empty(&tp->rx_done))
-				napi_schedule(&tp->napi);
 		} else {
 			tp->rtl_ops.up(tp);
 			netif_carrier_off(tp->netdev);


The proper work around is to enclose the napi_schedule() in a
local_bh_enable()/local_bh_disable()  pair.

Something like :

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 986243c932ccd6fe19c592805c1c63274f5e5555..b6bb1720c383946ea6142ec2f79f5b7a69031d7f 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -3703,8 +3703,10 @@ static int rtl8152_resume(struct usb_interface *intf)
 			napi_enable(&tp->napi);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			smp_mb__after_atomic();
+			local_bh_disable();
 			if (!list_empty(&tp->rx_done))
 				napi_schedule(&tp->napi);
+			local_bh_enable();
 		} else {
 			tp->rtl_ops.up(tp);
 			netif_carrier_off(tp->netdev);

Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll

From: Petr Vorel <hidden>
Date: 2017-03-13 15:37:49

Hi Eric,

quoted

The proper work around is to enclose the napi_schedule() in a
local_bh_enable()/local_bh_disable()  pair.

quoted hunk

Something like :

--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -3703,8 +3703,10 @@ static int rtl8152_resume(struct usb_interface *intf)
 			napi_enable(&tp->napi);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			smp_mb__after_atomic();
+			local_bh_disable();
 			if (!list_empty(&tp->rx_done))
 				napi_schedule(&tp->napi);
+			local_bh_enable();

Unfortunately this doesn't work. Code in r8152.c doesn't use local_bh_enable()/local_bh_disable(). I tried to lock it with spin_lock_bh()/spin_unlock_bh() and with mutex_lock()/mutex_unlock() but neither work.


Kind regards,
Petr

Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll

From: Eric Dumazet <hidden>
Date: 2017-03-13 15:44:12

On Mon, 2017-03-13 at 16:37 +0100, Petr Vorel wrote:

Hi Eric,

quoted

The proper work around is to enclose the napi_schedule() in a
local_bh_enable()/local_bh_disable()  pair.

quoted

Something like :

--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -3703,8 +3703,10 @@ static int rtl8152_resume(struct usb_interface *intf)
 			napi_enable(&tp->napi);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			smp_mb__after_atomic();
+			local_bh_disable();
 			if (!list_empty(&tp->rx_done))
 				napi_schedule(&tp->napi);
+			local_bh_enable();

Unfortunately this doesn't work. Code in r8152.c doesn't use
local_bh_enable()/local_bh_disable(). I tried to lock it with
spin_lock_bh()/spin_unlock_bh() and with mutex_lock()/mutex_unlock()
but neither work.

The local_bh_disable() / local_bh_enable() definitely is the right
answer to the issue you described.

It does not matter what code in r8152.c currently does.

https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=8cf699ec849f4ca1413cea01289bd7d37dbcc626

Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll

From: Eric Dumazet <hidden>
Date: 2017-03-13 15:46:44

On Mon, 2017-03-13 at 08:44 -0700, Eric Dumazet wrote:

On Mon, 2017-03-13 at 16:37 +0100, Petr Vorel wrote:

quoted

Hi Eric,

quoted

The proper work around is to enclose the napi_schedule() in a
local_bh_enable()/local_bh_disable()  pair.

quoted

Something like :

--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -3703,8 +3703,10 @@ static int rtl8152_resume(struct usb_interface *intf)
 			napi_enable(&tp->napi);
 			clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 			smp_mb__after_atomic();
+			local_bh_disable();
 			if (!list_empty(&tp->rx_done))
 				napi_schedule(&tp->napi);
+			local_bh_enable();

Unfortunately this doesn't work. Code in r8152.c doesn't use
local_bh_enable()/local_bh_disable(). I tried to lock it with
spin_lock_bh()/spin_unlock_bh() and with mutex_lock()/mutex_unlock()
but neither work.

The local_bh_disable() / local_bh_enable() definitely is the right
answer to the issue you described.

It does not matter what code in r8152.c currently does.

https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=8cf699ec849f4ca1413cea01289bd7d37dbcc626


You also have to protect other napi_schedule(), like the ones in
rtl_work_func_t() or rtl8152_post_reset()

Re: [PATCH 1/1] r8152: fix NULL pointer dereference in r8152_poll

From: Petr Vorel <hidden>
Date: 2017-03-13 21:57:18

quoted

Unfortunately this doesn't work. Code in r8152.c doesn't use
local_bh_enable()/local_bh_disable(). I tried to lock it with
spin_lock_bh()/spin_unlock_bh() and with mutex_lock()/mutex_unlock()
but neither work.

quoted

The local_bh_disable() / local_bh_enable() definitely is the right
answer to the issue you described.

quoted

It does not matter what code in r8152.c currently does.

quoted

https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=8cf699ec849f4ca1413cea01289bd7d37dbcc626

You also have to protect other napi_schedule(), like the ones in
rtl_work_func_t() or rtl8152_post_reset()

I've tested that before :-). I'll be more precise what "not working" means: it fixes
invalid pointer issue, but kernel crashes for different reason:

 ...
Call Trace:
 <IRQ>
 net_rx_action+0x23c/0x3f0
 __do_softirq+0x104/0x2e1
 ? usb_runtime_suspend+0x70/0x70 [usbcore]
 do_softirq_own_stack+8x1c/0x30
 </IRQ>
 do_softirq.part.18+0x41/0x50
 __local_bh_enable_ip+0x88/0xa0
 rtl8152_resume+0xe2/0x1a0 [r8152]
 usb_resume_interface.isra.60x99/0xf0 [usbcore]
 usb_resume_both+0x6a/0x130 [usbcore]
 __rpm_callback+0xb9/0x1f0
 rpm_callback+Ox5f/0x80
 ? usb_runtime_suspend+0x70/0x70 [usbcore]
 usb_resume+0x495/0x6b0
 ? update_load_avg+Ox79/0x520
 ? update_load_avg+Ox79/0x520
 ? refcount_dec_and_test+0x11/0x20
 __pm_runtime_resume+0x3f/0x60
 usb_autoresume_device+0x23/0x50 [usbcore]
 usb_dev_open+0xe7/0x250 [usbcore]
 chrdev_open+0xa1/0x200
 do_dentry_open+0x20a/0x2f0
 ? cdev_put+0x30/0x30
 vfs_open+0x4c/0x70
 ? may_open+0x9b/0x100
 path_openat+0x5ec/0x1430
 do_filp_open+0x7e/0xe0
 ? __vfs_write+0x28/0x140
 ? __alloc_fd+0xb2/0x160
 do_sys_open+0x123/0x200
 SyS_open+0x1e/0x20
 entry_SYSCALL_64_fastpath+0x1e/0xad
 ...
 Kernel panic - not syncing: Fatal exception in interrupt
 ...

Patch: http://pastebin.com/Uejjc0Bh (I don't post patch here, as it's not working).


Kind regards,
Petr

[PATCH net] r8152: fix the list rx_done may be used without initialization

From: Hayes Wang <hidden>
Date: 2017-03-14 06:17:16

The list rx_done would be initialized when the linking on occurs.
Therefore, if a napi is scheduled without any linking on before,
the following kernel panic would happen.

	BUG: unable to handle kernel NULL pointer dereference at 000000000000008
	IP: [<ffffffffc085efde>] r8152_poll+0xe1e/0x1210 [r8152]
	PGD 0
	Oops: 0002 [#1] SMP

Signed-off-by: Hayes Wang <redacted>
---
 drivers/net/usb/r8152.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 986243c..bb3eedd 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -1362,6 +1362,7 @@ static int alloc_all_mem(struct r8152 *tp)
 	spin_lock_init(&tp->rx_lock);
 	spin_lock_init(&tp->tx_lock);
 	INIT_LIST_HEAD(&tp->tx_free);
+	INIT_LIST_HEAD(&tp->rx_done);
 	skb_queue_head_init(&tp->tx_queue);
 	skb_queue_head_init(&tp->rx_queue);

-- 
2.7.4

Re: [PATCH net] r8152: fix the list rx_done may be used without initialization

From: Petr Vorel <hidden>
Date: 2017-03-14 08:53:40

Hi Hayes,

The list rx_done would be initialized when the linking on occurs.
Therefore, if a napi is scheduled without any linking on before,
the following kernel panic would happen.

	BUG: unable to handle kernel NULL pointer dereference at 000000000000008
	IP: [<ffffffffc085efde>] r8152_poll+0xe1e/0x1210 [r8152]
	PGD 0
	Oops: 0002 [#1] SMP

thanks for fixing!

Kind regards,
Petr

RE: [PATCH net] r8152: fix the list rx_done may be used without initialization

From: Hayes Wang <hidden>
Date: 2017-03-14 09:17:33

Petr Vorel [mailto:petr.vorel@gmail.com]

Sent: Tuesday, March 14, 2017 4:54 PM

[...]

thanks for fixing!

Does it work?

Best Regards,
Hayes

Re: [PATCH net] r8152: fix the list rx_done may be used without initialization

From: David Miller <davem@davemloft.net>
Date: 2017-03-21 21:46:45

From: Hayes Wang <redacted>
Date: Tue, 14 Mar 2017 14:15:20 +0800

The list rx_done would be initialized when the linking on occurs.
Therefore, if a napi is scheduled without any linking on before,
the following kernel panic would happen.

	BUG: unable to handle kernel NULL pointer dereference at 000000000000008
	IP: [<ffffffffc085efde>] r8152_poll+0xe1e/0x1210 [r8152]
	PGD 0
	Oops: 0002 [#1] SMP

Signed-off-by: Hayes Wang <redacted>

Applied.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help