Skip to content

Commit 11b3f5e

Browse files
PlaidCatPlaidCat
committed
net: usb: lan78xx: reorder cleanup operations to avoid UAF bugs
jira LE-1907 Rebuild_History Non-Buildable kernel-5.14.0-427.37.1.el9_4 commit-author Duoming Zhou <duoming@zju.edu.cn> commit 1e7417c Empty-Commit: Cherry-Pick Conflicts during history rebuild. Will be included in final tarball splat. Ref for failed cherry-pick at: ciq/ciq_backports/kernel-5.14.0-427.37.1.el9_4/1e7417c1.failed The timer dev->stat_monitor can schedule the delayed work dev->wq and the delayed work dev->wq can also arm the dev->stat_monitor timer. When the device is detaching, the net_device will be deallocated. but the net_device private data could still be dereferenced in delayed work or timer handler. As a result, the UAF bugs will happen. One racy situation is shown below: (Thread 1) | (Thread 2) lan78xx_stat_monitor() | ... | lan78xx_disconnect() lan78xx_defer_kevent() | ... ... | cancel_delayed_work_sync(&dev->wq); schedule_delayed_work() | ... (wait some time) | free_netdev(net); //free net_device lan78xx_delayedwork() | //use net_device private data | dev-> //use | Although we use cancel_delayed_work_sync() to cancel the delayed work in lan78xx_disconnect(), it could still be scheduled in timer handler lan78xx_stat_monitor(). Another racy situation is shown below: (Thread 1) | (Thread 2) lan78xx_delayedwork | mod_timer() | lan78xx_disconnect() | cancel_delayed_work_sync() (wait some time) | if (timer_pending(&dev->stat_monitor)) | del_timer_sync(&dev->stat_monitor); lan78xx_stat_monitor() | ... lan78xx_defer_kevent() | free_netdev(net); //free //use net_device private data| dev-> //use | Although we use del_timer_sync() to delete the timer, the function timer_pending() returns 0 when the timer is activated. As a result, the del_timer_sync() will not be executed and the timer could be re-armed. In order to mitigate this bug, We use timer_shutdown_sync() to shutdown the timer and then use cancel_delayed_work_sync() to cancel the delayed work. As a result, the net_device could be deallocated safely. What's more, the dev->flags is set to EVENT_DEV_DISCONNECT in lan78xx_disconnect(). But it could still be set to EVENT_STAT_UPDATE in lan78xx_stat_monitor(). So this patch put the set_bit() behind timer_shutdown_sync(). Fixes: 77dfff5 ("lan78xx: Fix race condition in disconnect handling") Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 1e7417c) Signed-off-by: Jonathan Maple <jmaple@ciq.com> # Conflicts: # drivers/net/usb/lan78xx.c
1 parent 52a8611 commit 11b3f5e

File tree

1 file changed

+101
-0
lines changed

1 file changed

+101
-0
lines changed

‎ciq/ciq_backports/kernel-5.14.0-427.37.1.el9_4/1e7417c1.failed‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
net: usb: lan78xx: reorder cleanup operations to avoid UAF bugs
2+
3+
jira LE-1907
4+
Rebuild_History Non-Buildable kernel-5.14.0-427.37.1.el9_4
5+
commit-author Duoming Zhou <duoming@zju.edu.cn>
6+
commit 1e7417c188d0a83fb385ba2dbe35fd2563f2b6f3
7+
Empty-Commit: Cherry-Pick Conflicts during history rebuild.
8+
Will be included in final tarball splat. Ref for failed cherry-pick at:
9+
ciq/ciq_backports/kernel-5.14.0-427.37.1.el9_4/1e7417c1.failed
10+
11+
The timer dev->stat_monitor can schedule the delayed work dev->wq and
12+
the delayed work dev->wq can also arm the dev->stat_monitor timer.
13+
14+
When the device is detaching, the net_device will be deallocated. but
15+
the net_device private data could still be dereferenced in delayed work
16+
or timer handler. As a result, the UAF bugs will happen.
17+
18+
One racy situation is shown below:
19+
20+
(Thread 1) | (Thread 2)
21+
lan78xx_stat_monitor() |
22+
... | lan78xx_disconnect()
23+
lan78xx_defer_kevent() | ...
24+
... | cancel_delayed_work_sync(&dev->wq);
25+
schedule_delayed_work() | ...
26+
(wait some time) | free_netdev(net); //free net_device
27+
lan78xx_delayedwork() |
28+
//use net_device private data |
29+
dev-> //use |
30+
31+
Although we use cancel_delayed_work_sync() to cancel the delayed work
32+
in lan78xx_disconnect(), it could still be scheduled in timer handler
33+
lan78xx_stat_monitor().
34+
35+
Another racy situation is shown below:
36+
37+
(Thread 1) | (Thread 2)
38+
lan78xx_delayedwork |
39+
mod_timer() | lan78xx_disconnect()
40+
| cancel_delayed_work_sync()
41+
(wait some time) | if (timer_pending(&dev->stat_monitor))
42+
| del_timer_sync(&dev->stat_monitor);
43+
lan78xx_stat_monitor() | ...
44+
lan78xx_defer_kevent() | free_netdev(net); //free
45+
//use net_device private data|
46+
dev-> //use |
47+
48+
Although we use del_timer_sync() to delete the timer, the function
49+
timer_pending() returns 0 when the timer is activated. As a result,
50+
the del_timer_sync() will not be executed and the timer could be
51+
re-armed.
52+
53+
In order to mitigate this bug, We use timer_shutdown_sync() to shutdown
54+
the timer and then use cancel_delayed_work_sync() to cancel the delayed
55+
work. As a result, the net_device could be deallocated safely.
56+
57+
What's more, the dev->flags is set to EVENT_DEV_DISCONNECT in
58+
lan78xx_disconnect(). But it could still be set to EVENT_STAT_UPDATE
59+
in lan78xx_stat_monitor(). So this patch put the set_bit() behind
60+
timer_shutdown_sync().
61+
62+
Fixes: 77dfff5bb7e2 ("lan78xx: Fix race condition in disconnect handling")
63+
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
64+
Signed-off-by: David S. Miller <davem@davemloft.net>
65+
(cherry picked from commit 1e7417c188d0a83fb385ba2dbe35fd2563f2b6f3)
66+
Signed-off-by: Jonathan Maple <jmaple@ciq.com>
67+
68+
# Conflicts:
69+
# drivers/net/usb/lan78xx.c
70+
diff --cc drivers/net/usb/lan78xx.c
71+
index b75e97330f79,59cde06aa7f6..000000000000
72+
--- a/drivers/net/usb/lan78xx.c
73+
+++ b/drivers/net/usb/lan78xx.c
74+
@@@ -3910,7 -4224,7 +3910,11 @@@ static void lan78xx_disconnect(struct u
75+
if (!dev)
76+
return;
77+
78+
++<<<<<<< HEAD
79+
+ set_bit(EVENT_DEV_DISCONNECT, &dev->flags);
80+
++=======
81+
+ netif_napi_del(&dev->napi);
82+
++>>>>>>> 1e7417c188d0 (net: usb: lan78xx: reorder cleanup operations to avoid UAF bugs)
83+
84+
udev = interface_to_usbdev(intf);
85+
net = dev->net;
86+
@@@ -3931,11 -4247,11 +3937,8 @@@
87+
88+
usb_scuttle_anchored_urbs(&dev->deferred);
89+
90+
- if (timer_pending(&dev->stat_monitor))
91+
- del_timer_sync(&dev->stat_monitor);
92+
-
93+
lan78xx_unbind(dev, intf);
94+
95+
- lan78xx_free_tx_resources(dev);
96+
- lan78xx_free_rx_resources(dev);
97+
-
98+
usb_kill_urb(dev->urb_intr);
99+
usb_free_urb(dev->urb_intr);
100+
101+
* Unmerged path drivers/net/usb/lan78xx.c

0 commit comments

Comments
 (0)