Re: [PATCH v2] ax25: Fix ax25 session cleanup problem in ax25_release
From: <hidden>
Date: 2022-05-28 10:40:52
Also in:
linux-hams, lkml
Hello, On Fri, 27 May 2022 23:18:32 +0800 Duoming wrote:
quoted hunk ↗ jump to hunk
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 <redacted> Signed-off-by: Duoming Zhou <redacted> --- 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