* [PATCH v2] ax25: Fix ax25 session cleanup problem in ax25_release
@ 2022-05-27 15:18 Duoming Zhou
2022-05-28 10:40 ` duoming
0 siblings, 1 reply; 2+ messages in thread
From: Duoming Zhou @ 2022-05-27 15:18 UTC (permalink / raw)
To: linux-hams
Cc: jreuter, ralf, davem, edumazet, kuba, pabeni, thomas, netdev,
linux-kernel, Duoming Zhou
The timers of ax25 are used for correct session cleanup.
If we use ax25_release() to close ax25 sessions and
ax25_dev is not null, the del_timer_sync() functions in
ax25_release() will execute. As a result, the sessions
could not be cleaned up correctly, because the timers
have stopped.
This patch adds a device_up flag in ax25_dev in order to
judge whether the device is up. If there are sessions to
be cleaned up, the del_timer_sync() in ax25_release() will
not execute. What's more, we add ax25_cb_del() in
ax25_kill_by_device(), because the timers have been stopped
and there are no functions that could delete ax25_cb if we
do not call ax25_release().
Fixes: 82e31755e55f ("ax25: Fix UAF bugs in ax25 timers")
Reported-and-tested-by: Thomas Osterried <thomas@osterried.de>
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
Changes in v2:
- Add ax25_cb_del() in ax25_kill_by_device().
include/net/ax25.h | 1 +
net/ax25/af_ax25.c | 15 ++++++++++-----
net/ax25/ax25_dev.c | 1 +
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/include/net/ax25.h b/include/net/ax25.h
index 0f9790c455b..a427a05672e 100644
--- a/include/net/ax25.h
+++ b/include/net/ax25.h
@@ -228,6 +228,7 @@ typedef struct ax25_dev {
ax25_dama_info dama;
#endif
refcount_t refcount;
+ bool device_up;
} ax25_dev;
typedef struct ax25_cb {
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 363d47f9453..92cbb08a6c5 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -81,6 +81,7 @@ static void ax25_kill_by_device(struct net_device *dev)
if ((ax25_dev = ax25_dev_ax25dev(dev)) == NULL)
return;
+ ax25_dev->device_up = false;
spin_lock_bh(&ax25_list_lock);
again:
@@ -91,6 +92,7 @@ static void ax25_kill_by_device(struct net_device *dev)
spin_unlock_bh(&ax25_list_lock);
ax25_disconnect(s, ENETUNREACH);
s->ax25_dev = NULL;
+ ax25_cb_del(s);
spin_lock_bh(&ax25_list_lock);
goto again;
}
@@ -104,6 +106,7 @@ static void ax25_kill_by_device(struct net_device *dev)
ax25_dev_put(ax25_dev);
}
release_sock(sk);
+ ax25_cb_del(s);
spin_lock_bh(&ax25_list_lock);
sock_put(sk);
/* The entry could have been deleted from the
@@ -1053,11 +1056,13 @@ static int ax25_release(struct socket *sock)
ax25_destroy_socket(ax25);
}
if (ax25_dev) {
- 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);
+ if (!ax25_dev->device_up) {
+ 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);
+ }
dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
ax25_dev_put(ax25_dev);
}
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index d2a244e1c26..5451be15e07 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -62,6 +62,7 @@ void ax25_dev_device_up(struct net_device *dev)
ax25_dev->dev = dev;
dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC);
ax25_dev->forward = NULL;
+ ax25_dev->device_up = true;
ax25_dev->values[AX25_VALUES_IPDEFMODE] = AX25_DEF_IPDEFMODE;
ax25_dev->values[AX25_VALUES_AXDEFMODE] = AX25_DEF_AXDEFMODE;
--
2.17.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] ax25: Fix ax25 session cleanup problem in ax25_release
2022-05-27 15:18 [PATCH v2] ax25: Fix ax25 session cleanup problem in ax25_release Duoming Zhou
@ 2022-05-28 10:40 ` duoming
0 siblings, 0 replies; 2+ messages in thread
From: duoming @ 2022-05-28 10:40 UTC (permalink / raw)
To: linux-hams
Cc: jreuter, ralf, davem, edumazet, kuba, pabeni, thomas, netdev,
linux-kernel
Hello,
On Fri, 27 May 2022 23:18:32 +0800 Duoming wrote:
> The timers of ax25 are used for correct session cleanup.
> If we use ax25_release() to close ax25 sessions and
> ax25_dev is not null, the del_timer_sync() functions in
> ax25_release() will execute. As a result, the sessions
> could not be cleaned up correctly, because the timers
> have stopped.
>
> This patch adds a device_up flag in ax25_dev in order to
> judge whether the device is up. If there are sessions to
> be cleaned up, the del_timer_sync() in ax25_release() will
> not execute. What's more, we add ax25_cb_del() in
> ax25_kill_by_device(), because the timers have been stopped
> and there are no functions that could delete ax25_cb if we
> do not call ax25_release().
>
> Fixes: 82e31755e55f ("ax25: Fix UAF bugs in ax25 timers")
> Reported-and-tested-by: Thomas Osterried <thomas@osterried.de>
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
> Changes in v2:
> - Add ax25_cb_del() in ax25_kill_by_device().
>
> include/net/ax25.h | 1 +
> net/ax25/af_ax25.c | 15 ++++++++++-----
> net/ax25/ax25_dev.c | 1 +
> 3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/ax25.h b/include/net/ax25.h
> index 0f9790c455b..a427a05672e 100644
> --- a/include/net/ax25.h
> +++ b/include/net/ax25.h
> @@ -228,6 +228,7 @@ typedef struct ax25_dev {
> ax25_dama_info dama;
> #endif
> refcount_t refcount;
> + bool device_up;
> } ax25_dev;
>
> typedef struct ax25_cb {
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 363d47f9453..92cbb08a6c5 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -81,6 +81,7 @@ static void ax25_kill_by_device(struct net_device *dev)
>
> if ((ax25_dev = ax25_dev_ax25dev(dev)) == NULL)
> return;
> + ax25_dev->device_up = false;
>
> spin_lock_bh(&ax25_list_lock);
> again:
> @@ -91,6 +92,7 @@ static void ax25_kill_by_device(struct net_device *dev)
> spin_unlock_bh(&ax25_list_lock);
> ax25_disconnect(s, ENETUNREACH);
> s->ax25_dev = NULL;
> + ax25_cb_del(s);
> spin_lock_bh(&ax25_list_lock);
> goto again;
> }
> @@ -104,6 +106,7 @@ static void ax25_kill_by_device(struct net_device *dev)
> ax25_dev_put(ax25_dev);
> }
> release_sock(sk);
> + ax25_cb_del(s);
> spin_lock_bh(&ax25_list_lock);
> sock_put(sk);
> /* The entry could have been deleted from the
There is a "refcount_t: underflow" problem, the call trace is shown below:
refcount_t: underflow; use-after-free.
WARNING: CPU: 1 PID: 15997 at lib/refcount.c:28 refcount_warn_saturate+0xc5/0x110
RIP: 0010:refcount_warn_saturate+0xc5/0x110
Code: 1b e0 d6 02 01 e8 46 82 1d 01 0f 0b eb 99 80 3d 08 e0 d6 02 00 75 90 48 c7 c7 80 87
RSP: 0018:ffff88800ab37db0 EFLAGS: 00000286
RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000000
RDX: 0000000000000001 RSI: 0000000000000008 RDI: ffffed1001566fa8
RBP: ffff88800a3bb410 R08: ffffffff810ffe2f R09: ffff88800ab37a37
R10: ffffed1001566f46 R11: 0000000000000001 R12: ffff888008960000
R13: ffff88800953f2c0 R14: ffff888006500018 R15: ffff888008960080
FS: 00007f46981f3700(0000) GS:ffff88806c600000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000042270a CR3: 0000000009b64000 CR4: 00000000000006e0
Call Trace:
<TASK>
__sk_destruct+0x2c/0x350
ax25_release+0x34e/0x4a0
__sock_release+0x6d/0x120
sock_close+0xf/0x20
__fput+0x10e/0x410
task_work_run+0x86/0xc0
exit_to_user_mode_prepare+0x194/0x1a0
syscall_exit_to_user_mode+0x19/0x50
do_syscall_64+0x48/0x90
entry_SYSCALL_64_after_hwframe+0x44/0xae
The race condition is shown below:
(Thread 1) | (Thread 2)
ax25_create() |
refcount_set(&ax25->refcount, 1) |
ax25_bind() |
ax25_cb_add() |
ax25_cb_hold(ax25) //refcnt = 2 |
ax25_kill_by_device() | ax25_release()
... | ...
release_sock(); |
// no locks protect ax25_cb_del | lock_sock()
ax25_cb_del() | ax25_destroy_socket()
if (!hlist_unhashed(..)) | ax25_cb_del()
... | if (!hlist_unhashed(..))
hlist_del_init() |
ax25_cb_put(ax25) //refcnt = 1| ...
| ax25_cb_put(ax25) //refcnt = 0
| ...
| sock_put(sk)
| sk_free()
| sk_destruct()
| __sk_destruct()
| ax25_free_sock()
| ax25_cb_put(ax25) // refcount_t: underflow!
Moving ax25_cb_del() into lock_sock() can solve this problem,
because there is a check in ax25_cb_del(). If we delete ax25 node
in hlist, the check will not be satisfied.
if (!hlist_unhashed(&ax25->ax25_node)) { //check
spin_lock_bh(&ax25_list_lock);
hlist_del_init(&ax25->ax25_node); //delete ax25 node
spin_unlock_bh(&ax25_list_lock);
ax25_cb_put(ax25);
}
My successful test was this:
@@ -103,6 +105,7 @@ static void ax25_kill_by_device(struct net_device *dev)
dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
ax25_dev_put(ax25_dev);
}
+ ax25_cb_del(s);
release_sock(sk);
spin_lock_bh(&ax25_list_lock);
sock_put(sk);
Best regards,
Duoming Zhou
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-05-28 10:40 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-27 15:18 [PATCH v2] ax25: Fix ax25 session cleanup problem in ax25_release Duoming Zhou
2022-05-28 10:40 ` duoming
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.