* [PATCH V5 0/2] Fix refcount leak and NPD bugs in ax25
@ 2022-03-18 0:54 Duoming Zhou
2022-03-18 0:54 ` [PATCH V5 1/2] ax25: Fix refcount leaks caused by ax25_cb_del() Duoming Zhou
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Duoming Zhou @ 2022-03-18 0:54 UTC (permalink / raw)
To: linux-hams
Cc: netdev, linux-kernel, kuba, davem, ralf, jreuter, eric.dumazet,
dan.carpenter, Duoming Zhou
The first patch fixes refcount leak in ax25 that could cause
ax25-ex-connected-session-now-listening-state-bug.
The second patch fixes NPD bugs in ax25 timers.
Duoming Zhou (2):
ax25: Fix refcount leaks caused by ax25_cb_del()
ax25: Fix NULL pointer dereferences in ax25 timers
net/ax25/af_ax25.c | 18 +++++++++++++-----
net/ax25/ax25_subr.c | 20 ++++++++++++++------
2 files changed, 27 insertions(+), 11 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH V5 1/2] ax25: Fix refcount leaks caused by ax25_cb_del()
2022-03-18 0:54 [PATCH V5 0/2] Fix refcount leak and NPD bugs in ax25 Duoming Zhou
@ 2022-03-18 0:54 ` Duoming Zhou
2022-03-18 0:54 ` [PATCH V6 2/2] ax25: Fix NULL pointer dereferences in ax25 timers Duoming Zhou
2022-03-21 11:00 ` [PATCH V5 0/2] Fix refcount leak and NPD bugs in ax25 patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Duoming Zhou @ 2022-03-18 0:54 UTC (permalink / raw)
To: linux-hams
Cc: netdev, linux-kernel, kuba, davem, ralf, jreuter, eric.dumazet,
dan.carpenter, Duoming Zhou
The previous commit d01ffb9eee4a ("ax25: add refcount in ax25_dev to
avoid UAF bugs") and commit feef318c855a ("ax25: fix UAF bugs of
net_device caused by rebinding operation") increase the refcounts of
ax25_dev and net_device in ax25_bind() and decrease the matching refcounts
in ax25_kill_by_device() in order to prevent UAF bugs, but there are
reference count leaks.
The root cause of refcount leaks is shown below:
(Thread 1) | (Thread 2)
ax25_bind() |
... |
ax25_addr_ax25dev() |
ax25_dev_hold() //(1) |
... |
dev_hold_track() //(2) |
... | ax25_destroy_socket()
| ax25_cb_del()
| ...
| hlist_del_init() //(3)
|
|
(Thread 3) |
ax25_kill_by_device() |
... |
ax25_for_each(s, &ax25_list) { |
if (s->ax25_dev == ax25_dev) //(4) |
... |
Firstly, we use ax25_bind() to increase the refcount of ax25_dev in
position (1) and increase the refcount of net_device in position (2).
Then, we use ax25_cb_del() invoked by ax25_destroy_socket() to delete
ax25_cb in hlist in position (3) before calling ax25_kill_by_device().
Finally, the decrements of refcounts in ax25_kill_by_device() will not
be executed, because no s->ax25_dev equals to ax25_dev in position (4).
This patch adds decrements of refcounts in ax25_release() and use
lock_sock() to do synchronization. If refcounts decrease in ax25_release(),
the decrements of refcounts in ax25_kill_by_device() will not be
executed and vice versa.
Fixes: d01ffb9eee4a ("ax25: add refcount in ax25_dev to avoid UAF bugs")
Fixes: 87563a043cef ("ax25: fix reference count leaks of ax25_dev")
Fixes: feef318c855a ("ax25: fix UAF bugs of net_device caused by rebinding operation")
Reported-by: Thomas Osterried <thomas@osterried.de>
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
Changes in V5:
- Use sk->sk_socket to check in ax25_kill_by_device().
net/ax25/af_ax25.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 6bd09718077..cf8847cfc66 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -98,8 +98,10 @@ static void ax25_kill_by_device(struct net_device *dev)
spin_unlock_bh(&ax25_list_lock);
lock_sock(sk);
s->ax25_dev = NULL;
- dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
- ax25_dev_put(ax25_dev);
+ if (sk->sk_socket) {
+ dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
+ ax25_dev_put(ax25_dev);
+ }
ax25_disconnect(s, ENETUNREACH);
release_sock(sk);
spin_lock_bh(&ax25_list_lock);
@@ -979,14 +981,20 @@ static int ax25_release(struct socket *sock)
{
struct sock *sk = sock->sk;
ax25_cb *ax25;
+ ax25_dev *ax25_dev;
if (sk == NULL)
return 0;
sock_hold(sk);
- sock_orphan(sk);
lock_sock(sk);
+ sock_orphan(sk);
ax25 = sk_to_ax25(sk);
+ ax25_dev = ax25->ax25_dev;
+ if (ax25_dev) {
+ dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
+ ax25_dev_put(ax25_dev);
+ }
if (sk->sk_type == SOCK_SEQPACKET) {
switch (ax25->state) {
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH V6 2/2] ax25: Fix NULL pointer dereferences in ax25 timers
2022-03-18 0:54 [PATCH V5 0/2] Fix refcount leak and NPD bugs in ax25 Duoming Zhou
2022-03-18 0:54 ` [PATCH V5 1/2] ax25: Fix refcount leaks caused by ax25_cb_del() Duoming Zhou
@ 2022-03-18 0:54 ` Duoming Zhou
2022-03-21 11:00 ` [PATCH V5 0/2] Fix refcount leak and NPD bugs in ax25 patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Duoming Zhou @ 2022-03-18 0:54 UTC (permalink / raw)
To: linux-hams
Cc: netdev, linux-kernel, kuba, davem, ralf, jreuter, eric.dumazet,
dan.carpenter, Duoming Zhou
The previous commit 7ec02f5ac8a5 ("ax25: fix NPD bug in ax25_disconnect")
move ax25_disconnect into lock_sock() in order to prevent NPD bugs. But
there are race conditions that may lead to null pointer dereferences in
ax25_heartbeat_expiry(), ax25_t1timer_expiry(), ax25_t2timer_expiry(),
ax25_t3timer_expiry() and ax25_idletimer_expiry(), when we use
ax25_kill_by_device() to detach the ax25 device.
One of the race conditions that cause null pointer dereferences can be
shown as below:
(Thread 1) | (Thread 2)
ax25_connect() |
ax25_std_establish_data_link() |
ax25_start_t1timer() |
mod_timer(&ax25->t1timer,..) |
| ax25_kill_by_device()
(wait a time) | ...
| s->ax25_dev = NULL; //(1)
ax25_t1timer_expiry() |
ax25->ax25_dev->values[..] //(2)| ...
... |
We set null to ax25_cb->ax25_dev in position (1) and dereference
the null pointer in position (2).
The corresponding fail log is shown below:
===============================================================
BUG: kernel NULL pointer dereference, address: 0000000000000050
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.17.0-rc6-00794-g45690b7d0
RIP: 0010:ax25_t1timer_expiry+0x12/0x40
...
Call Trace:
call_timer_fn+0x21/0x120
__run_timers.part.0+0x1ca/0x250
run_timer_softirq+0x2c/0x60
__do_softirq+0xef/0x2f3
irq_exit_rcu+0xb6/0x100
sysvec_apic_timer_interrupt+0xa2/0xd0
...
This patch moves ax25_disconnect() before s->ax25_dev = NULL
and uses del_timer_sync() to delete timers in ax25_disconnect().
If ax25_disconnect() is called by ax25_kill_by_device() or
ax25->ax25_dev is NULL, the reason in ax25_disconnect() will be
equal to ENETUNREACH, it will wait all timers to stop before we
set null to s->ax25_dev in ax25_kill_by_device().
Fixes: 7ec02f5ac8a5 ("ax25: fix NPD bug in ax25_disconnect")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
Changes in V6:
- Add Fixes tag.
- Make commit message more clearer.
net/ax25/af_ax25.c | 4 ++--
net/ax25/ax25_subr.c | 20 ++++++++++++++------
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index cf8847cfc66..992b6e5d85d 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -89,20 +89,20 @@ static void ax25_kill_by_device(struct net_device *dev)
sk = s->sk;
if (!sk) {
spin_unlock_bh(&ax25_list_lock);
- s->ax25_dev = NULL;
ax25_disconnect(s, ENETUNREACH);
+ s->ax25_dev = NULL;
spin_lock_bh(&ax25_list_lock);
goto again;
}
sock_hold(sk);
spin_unlock_bh(&ax25_list_lock);
lock_sock(sk);
+ ax25_disconnect(s, ENETUNREACH);
s->ax25_dev = NULL;
if (sk->sk_socket) {
dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
ax25_dev_put(ax25_dev);
}
- ax25_disconnect(s, ENETUNREACH);
release_sock(sk);
spin_lock_bh(&ax25_list_lock);
sock_put(sk);
diff --git a/net/ax25/ax25_subr.c b/net/ax25/ax25_subr.c
index 15ab812c4fe..3a476e4f6cd 100644
--- a/net/ax25/ax25_subr.c
+++ b/net/ax25/ax25_subr.c
@@ -261,12 +261,20 @@ void ax25_disconnect(ax25_cb *ax25, int reason)
{
ax25_clear_queues(ax25);
- if (!ax25->sk || !sock_flag(ax25->sk, SOCK_DESTROY))
- ax25_stop_heartbeat(ax25);
- ax25_stop_t1timer(ax25);
- ax25_stop_t2timer(ax25);
- ax25_stop_t3timer(ax25);
- ax25_stop_idletimer(ax25);
+ if (reason == ENETUNREACH) {
+ del_timer_sync(&ax25->timer);
+ del_timer_sync(&ax25->t1timer);
+ del_timer_sync(&ax25->t2timer);
+ del_timer_sync(&ax25->t3timer);
+ del_timer_sync(&ax25->idletimer);
+ } else {
+ if (!ax25->sk || !sock_flag(ax25->sk, SOCK_DESTROY))
+ ax25_stop_heartbeat(ax25);
+ ax25_stop_t1timer(ax25);
+ ax25_stop_t2timer(ax25);
+ ax25_stop_t3timer(ax25);
+ ax25_stop_idletimer(ax25);
+ }
ax25->state = AX25_STATE_0;
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH V5 0/2] Fix refcount leak and NPD bugs in ax25
2022-03-18 0:54 [PATCH V5 0/2] Fix refcount leak and NPD bugs in ax25 Duoming Zhou
2022-03-18 0:54 ` [PATCH V5 1/2] ax25: Fix refcount leaks caused by ax25_cb_del() Duoming Zhou
2022-03-18 0:54 ` [PATCH V6 2/2] ax25: Fix NULL pointer dereferences in ax25 timers Duoming Zhou
@ 2022-03-21 11:00 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-03-21 11:00 UTC (permalink / raw)
To: Duoming Zhou
Cc: linux-hams, netdev, linux-kernel, kuba, davem, ralf, jreuter,
eric.dumazet, dan.carpenter
Hello:
This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:
On Fri, 18 Mar 2022 08:54:03 +0800 you wrote:
> The first patch fixes refcount leak in ax25 that could cause
> ax25-ex-connected-session-now-listening-state-bug.
>
> The second patch fixes NPD bugs in ax25 timers.
>
> Duoming Zhou (2):
> ax25: Fix refcount leaks caused by ax25_cb_del()
> ax25: Fix NULL pointer dereferences in ax25 timers
>
> [...]
Here is the summary with links:
- [V5,1/2] ax25: Fix refcount leaks caused by ax25_cb_del()
https://git.kernel.org/netdev/net/c/9fd75b66b8f6
- [V6,2/2] ax25: Fix NULL pointer dereferences in ax25 timers
https://git.kernel.org/netdev/net/c/fc6d01ff9ef0
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-21 11:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-18 0:54 [PATCH V5 0/2] Fix refcount leak and NPD bugs in ax25 Duoming Zhou
2022-03-18 0:54 ` [PATCH V5 1/2] ax25: Fix refcount leaks caused by ax25_cb_del() Duoming Zhou
2022-03-18 0:54 ` [PATCH V6 2/2] ax25: Fix NULL pointer dereferences in ax25 timers Duoming Zhou
2022-03-21 11:00 ` [PATCH V5 0/2] Fix refcount leak and NPD bugs in ax25 patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.