2023-05-15 10:11:28

by Zheng Wang

[permalink] [raw]
Subject: [PATCH] fs/jfs: Add a mutex named txEnd_lmLogClose_mutex to prevent a race condition between txEnd and lmLogClose functions

Syzkaller reported an error "slab-use-after-free Write in txEnd".

crash report:

==================================================================
BUG: KASAN: slab-use-after-free in instrument_atomic_write include/linux/instrumented.h:87 [inline]
BUG: KASAN: slab-use-after-free in clear_bit include/asm-generic/bitops/instrumented-atomic.h:41 [inline]
BUG: KASAN: slab-use-after-free in txEnd+0x2a3/0x5a0 fs/jfs/jfs_txnmgr.c:535
Write of size 8 at addr ffff888021bee840 by task jfsCommit/130

CPU: 3 PID: 130 Comm: jfsCommit Not tainted 6.3.0-rc7-pasta #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd9/0x150 lib/dump_stack.c:106
print_address_description mm/kasan/report.c:319 [inline]
print_report+0xc1/0x5e0 mm/kasan/report.c:430
kasan_report+0xc0/0xf0 mm/kasan/report.c:536
check_region_inline mm/kasan/generic.c:181 [inline]
kasan_check_range+0x144/0x190 mm/kasan/generic.c:187
instrument_atomic_write include/linux/instrumented.h:87 [inline]
clear_bit include/asm-generic/bitops/instrumented-atomic.h:41 [inline]
txEnd+0x2a3/0x5a0 fs/jfs/jfs_txnmgr.c:535
txLazyCommit fs/jfs/jfs_txnmgr.c:2679 [inline]
jfs_lazycommit+0x75f/0xb10 fs/jfs/jfs_txnmgr.c:2727
kthread+0x2e8/0x3a0 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
</TASK>

Allocated by task 86743:
kasan_save_stack+0x22/0x40 mm/kasan/common.c:45
kasan_set_track+0x25/0x30 mm/kasan/common.c:52
____kasan_kmalloc mm/kasan/common.c:374 [inline]
____kasan_kmalloc mm/kasan/common.c:333 [inline]
__kasan_kmalloc+0xa3/0xb0 mm/kasan/common.c:383
kmalloc include/linux/slab.h:580 [inline]
kzalloc include/linux/slab.h:720 [inline]
open_inline_log fs/jfs/jfs_logmgr.c:1159 [inline]
lmLogOpen+0x562/0x1430 fs/jfs/jfs_logmgr.c:1069
jfs_mount_rw+0x2f6/0x6d0 fs/jfs/jfs_mount.c:257
jfs_fill_super+0xa00/0xd40 fs/jfs/super.c:565
mount_bdev+0x351/0x410 fs/super.c:1380
legacy_get_tree+0x109/0x220 fs/fs_context.c:610
vfs_get_tree+0x8d/0x350 fs/super.c:1510
do_new_mount fs/namespace.c:3042 [inline]
path_mount+0x675/0x1e30 fs/namespace.c:3372
do_mount fs/namespace.c:3385 [inline]
__do_sys_mount fs/namespace.c:3594 [inline]
__se_sys_mount fs/namespace.c:3571 [inline]
__x64_sys_mount+0x283/0x300 fs/namespace.c:3571
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

Freed by task 16288:
kasan_save_stack+0x22/0x40 mm/kasan/common.c:45
kasan_set_track+0x25/0x30 mm/kasan/common.c:52
kasan_save_free_info+0x2b/0x40 mm/kasan/generic.c:521
____kasan_slab_free mm/kasan/common.c:236 [inline]
____kasan_slab_free+0x13b/0x1a0 mm/kasan/common.c:200
kasan_slab_free include/linux/kasan.h:162 [inline]
__cache_free mm/slab.c:3390 [inline]
__do_kmem_cache_free mm/slab.c:3577 [inline]
__kmem_cache_free+0xcd/0x2c0 mm/slab.c:3584
lmLogClose+0x595/0x720 fs/jfs/jfs_logmgr.c:1461
jfs_umount+0x2ef/0x430 fs/jfs/jfs_umount.c:114
jfs_put_super+0x85/0x1d0 fs/jfs/super.c:194
generic_shutdown_super+0x158/0x480 fs/super.c:500
kill_block_super+0x9b/0xf0 fs/super.c:1407
deactivate_locked_super+0x98/0x160 fs/super.c:331
deactivate_super+0xb1/0xd0 fs/super.c:362
cleanup_mnt+0x2ae/0x3d0 fs/namespace.c:1177
task_work_run+0x168/0x260 kernel/task_work.c:179
resume_user_mode_work include/linux/resume_user_mode.h:49 [inline]
exit_to_user_mode_loop kernel/entry/common.c:171 [inline]
exit_to_user_mode_prepare+0x210/0x240 kernel/entry/common.c:204
__syscall_exit_to_user_mode_work kernel/entry/common.c:286 [inline]
syscall_exit_to_user_mode+0x1d/0x50 kernel/entry/common.c:297
do_syscall_64+0x46/0xb0 arch/x86/entry/common.c:86
entry_SYSCALL_64_after_hwframe+0x63/0xcd

Last potentially related work creation:
kasan_save_stack+0x22/0x40 mm/kasan/common.c:45
__kasan_record_aux_stack+0x7b/0x90 mm/kasan/generic.c:491
kvfree_call_rcu+0x70/0xad0 kernel/rcu/tree.c:3327
neigh_destroy+0x431/0x660 net/core/neighbour.c:941
neigh_release include/net/neighbour.h:449 [inline]
neigh_cleanup_and_release+0x1f8/0x280 net/core/neighbour.c:103
neigh_periodic_work+0x60c/0xac0 net/core/neighbour.c:1025
process_one_work+0x98a/0x15c0 kernel/workqueue.c:2390
worker_thread+0x669/0x1090 kernel/workqueue.c:2537
kthread+0x2e8/0x3a0 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308

The buggy address belongs to the object at ffff888021bee800
which belongs to the cache kmalloc-1k of size 1024
The buggy address is located 64 bytes inside of
freed 1024-byte region [ffff888021bee800, ffff888021beec00)

The buggy address belongs to the physical page:
page:ffffea000086fb80 refcount:1 mapcount:0 mapping:0000000000000000
flags: 0xfff00000000200(slab|node=0|zone=1|lastcpupid=0x7ff)
raw: 00fff00000000200 ffff888012440700 ffffea0000583ed0 ffffea0000609790
raw: 0000000000000000 ffff888021bee000 0000000100000002 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
prep_new_page mm/page_alloc.c:2553 [inline]
get_page_from_freelist+0x119c/0x2d40 mm/page_alloc.c:4326
__alloc_pages+0x1cb/0x4a0 mm/page_alloc.c:5592
__alloc_pages_node include/linux/gfp.h:237 [inline]
kmem_getpages mm/slab.c:1360 [inline]
cache_grow_begin+0x9b/0x3b0 mm/slab.c:2570
cache_alloc_refill+0x27e/0x380 mm/slab.c:2943
____cache_alloc mm/slab.c:3019 [inline]
____cache_alloc mm/slab.c:3002 [inline]
__do_cache_alloc mm/slab.c:3202 [inline]
slab_alloc_node mm/slab.c:3250 [inline]
__kmem_cache_alloc_node+0x360/0x3f0 mm/slab.c:3541
__do_kmalloc_node mm/slab_common.c:966 [inline]
__kmalloc+0x4e/0x190 mm/slab_common.c:980
kmalloc include/linux/slab.h:584 [inline]
kzalloc include/linux/slab.h:720 [inline]
ieee802_11_parse_elems_full+0x106/0x1320 net/mac80211/util.c:1609
ieee802_11_parse_elems_crc.constprop.0+0x99/0xd0
net/mac80211/ieee80211_i.h:2265
ieee802_11_parse_elems net/mac80211/ieee80211_i.h:2272 [inline]
ieee80211_bss_info_update+0x410/0xb50 net/mac80211/scan.c:212
ieee80211_rx_bss_info net/mac80211/ibss.c:1120 [inline]
ieee80211_rx_mgmt_probe_beacon net/mac80211/ibss.c:1609 [inline]
ieee80211_ibss_rx_queued_mgmt+0x18b2/0x2d30 net/mac80211/ibss.c:1638
ieee80211_iface_process_skb net/mac80211/iface.c:1583 [inline]
ieee80211_iface_work+0xa47/0xd60 net/mac80211/iface.c:1637
process_one_work+0x98a/0x15c0 kernel/workqueue.c:2390
worker_thread+0x669/0x1090 kernel/workqueue.c:2537
kthread+0x2e8/0x3a0 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
page last free stack trace:
reset_page_owner include/linux/page_owner.h:24 [inline]
free_pages_prepare mm/page_alloc.c:1454 [inline]
free_pcp_prepare+0x4e3/0x920 mm/page_alloc.c:1504
free_unref_page_prepare mm/page_alloc.c:3388 [inline]
free_unref_page+0x1d/0x4e0 mm/page_alloc.c:3483
slab_destroy mm/slab.c:1613 [inline]
slabs_destroy+0x85/0xc0 mm/slab.c:1633
__cache_free_alien mm/slab.c:773 [inline]
cache_free_alien mm/slab.c:789 [inline]
___cache_free+0x203/0x3e0 mm/slab.c:3417
qlink_free mm/kasan/quarantine.c:168 [inline]
qlist_free_all+0x4f/0x1a0 mm/kasan/quarantine.c:187
kasan_quarantine_reduce+0x188/0x1d0 mm/kasan/quarantine.c:294
__kasan_slab_alloc+0x63/0x90 mm/kasan/common.c:305
kasan_slab_alloc include/linux/kasan.h:186 [inline]
slab_post_alloc_hook mm/slab.h:769 [inline]
slab_alloc_node mm/slab.c:3257 [inline]
slab_alloc mm/slab.c:3266 [inline]
__kmem_cache_alloc_lru mm/slab.c:3443 [inline]
kmem_cache_alloc+0x1bd/0x3f0 mm/slab.c:3452
vm_area_alloc+0x20/0x100 kernel/fork.c:458
mmap_region+0x389/0x2270 mm/mmap.c:2553
do_mmap+0x853/0xf20 mm/mmap.c:1364
vm_mmap_pgoff+0x1af/0x280 mm/util.c:542
ksys_mmap_pgoff+0x41f/0x5a0 mm/mmap.c:1410
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

Memory state around the buggy address:
ffff888021bee700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff888021bee780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff888021bee800: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff888021bee880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888021bee900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Through analysis, it was found that a race condition occurred between two
functions lmLogClose and txEnd, which were executed in different threads.
The possible sequence is as follows:

-------------------------------------------------------------------------
cpu1(free thread) | cpu2(use thread)
-------------------------------------------------------------------------
lmLogClose | txEnd
| log = JFS_SBI(tblk->sb)->log;
sbi->log = NULL; |
kfree(log); [1] free log |
| clear_bit(log_FLUSH, &log->flag); [2] UAF

Fix it by add a mutex lock between lmLogClose and txEnd:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Zheng Wang <[email protected]>
---
fs/jfs/jfs_debug.c | 1 +
fs/jfs/jfs_debug.h | 1 +
fs/jfs/jfs_logmgr.c | 2 ++
fs/jfs/jfs_txnmgr.c | 8 ++++++++
4 files changed, 12 insertions(+)

diff --git a/fs/jfs/jfs_debug.c b/fs/jfs/jfs_debug.c
index 44b62b3c322e..a9c9266b222b 100644
--- a/fs/jfs/jfs_debug.c
+++ b/fs/jfs/jfs_debug.c
@@ -78,3 +78,4 @@ void jfs_proc_clean(void)
}

#endif /* PROC_FS_JFS */
+DEFINE_MUTEX(txEnd_lmLogClose_mutex);
\ No newline at end of file
diff --git a/fs/jfs/jfs_debug.h b/fs/jfs/jfs_debug.h
index 48e2150c092e..25da8ee71f5e 100644
--- a/fs/jfs/jfs_debug.h
+++ b/fs/jfs/jfs_debug.h
@@ -107,3 +107,4 @@ int jfs_xtstat_proc_show(struct seq_file *m, void *v);
#endif /* CONFIG_JFS_STATISTICS */

#endif /* _H_JFS_DEBUG */
+extern struct mutex txEnd_lmLogClose_mutex;
\ No newline at end of file
diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
index 695415cbfe98..05c5391075db 100644
--- a/fs/jfs/jfs_logmgr.c
+++ b/fs/jfs/jfs_logmgr.c
@@ -1442,6 +1442,7 @@ int lmLogClose(struct super_block *sb)
jfs_info("lmLogClose: log:0x%p", log);

mutex_lock(&jfs_log_mutex);
+ mutex_lock(&txEnd_lmLogClose_mutex);
LOG_LOCK(log);
list_del(&sbi->log_list);
LOG_UNLOCK(log);
@@ -1490,6 +1491,7 @@ int lmLogClose(struct super_block *sb)
kfree(log);

out:
+ mutex_unlock(&txEnd_lmLogClose_mutex);
mutex_unlock(&jfs_log_mutex);
jfs_info("lmLogClose: exit(%d)", rc);
return rc;
diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c
index ffd4feece078..5b4351e59535 100644
--- a/fs/jfs/jfs_txnmgr.c
+++ b/fs/jfs/jfs_txnmgr.c
@@ -490,6 +490,7 @@ void txEnd(tid_t tid)
struct jfs_log *log;

jfs_info("txEnd: tid = %d", tid);
+ mutex_lock(&txEnd_lmLogClose_mutex);
TXN_LOCK();

/*
@@ -500,6 +501,11 @@ void txEnd(tid_t tid)

log = JFS_SBI(tblk->sb)->log;

+ if (!log) {
+ mutex_unlock(&txEnd_lmLogClose_mutex);
+ return;
+ }
+
/*
* Lazy commit thread can't free this guy until we mark it UNLOCKED,
* otherwise, we would be left with a transaction that may have been
@@ -515,6 +521,7 @@ void txEnd(tid_t tid)
spin_lock_irq(&log->gclock); // LOGGC_LOCK
tblk->flag |= tblkGC_UNLOCKED;
spin_unlock_irq(&log->gclock); // LOGGC_UNLOCK
+ mutex_unlock(&txEnd_lmLogClose_mutex);
return;
}

@@ -561,6 +568,7 @@ void txEnd(tid_t tid)
* wakeup all waitors for a free tblock
*/
TXN_WAKEUP(&TxAnchor.freewait);
+ mutex_unlock(&txEnd_lmLogClose_mutex);
}

/*
--
2.25.1



2023-07-20 10:40:17

by Michal Koutný

[permalink] [raw]
Subject: Re: [PATCH] fs/jfs: Add a mutex named txEnd_lmLogClose_mutex to prevent a race condition between txEnd and lmLogClose functions

Hello Zheng.

On Mon, May 15, 2023 at 05:59:56PM +0800, Zheng Wang <[email protected]> wrote:
> ==================================================================
> BUG: KASAN: slab-use-after-free in instrument_atomic_write include/linux/instrumented.h:87 [inline]
> BUG: KASAN: slab-use-after-free in clear_bit include/asm-generic/bitops/instrumented-atomic.h:41 [inline]
> BUG: KASAN: slab-use-after-free in txEnd+0x2a3/0x5a0 fs/jfs/jfs_txnmgr.c:535
> Write of size 8 at addr ffff888021bee840 by task jfsCommit/130
>
> CPU: 3 PID: 130 Comm: jfsCommit Not tainted 6.3.0-rc7-pasta #1

Is this still pertinent with the current mainline? (There were some
changes to jfs.)

> Through analysis, it was found that a race condition occurred between two
> functions lmLogClose and txEnd, which were executed in different threads.
> The possible sequence is as follows:
>
> -------------------------------------------------------------------------
> cpu1(free thread) | cpu2(use thread)
> -------------------------------------------------------------------------
> lmLogClose | txEnd
> | log = JFS_SBI(tblk->sb)->log;
> sbi->log = NULL; |
> kfree(log); [1] free log |
> | clear_bit(log_FLUSH, &log->flag); [2] UAF

That looks sane to a by-passer.

> Fix it by add a mutex lock between lmLogClose and txEnd:

It doesn't feel right wrt "lock data, not code" heuristics.
And when I apply that, it turns out there's already jfs_log_mutex.
I'd suggest you explain more why a new lock is needed (if that's the
preferred solutino).

Thanks,
Michal


Attachments:
(No filename) (1.64 kB)
signature.asc (235.00 B)
Download all attachments

2023-07-21 08:28:51

by Zheng Hacker

[permalink] [raw]
Subject: Re: [PATCH] fs/jfs: Add a mutex named txEnd_lmLogClose_mutex to prevent a race condition between txEnd and lmLogClose functions

Hello Michal,


Michal Koutný <[email protected]> 于2023年7月20日周四 17:56写道:
>
> Hello Zheng.
>
> On Mon, May 15, 2023 at 05:59:56PM +0800, Zheng Wang <[email protected]> wrote:
> > ==================================================================
> > BUG: KASAN: slab-use-after-free in instrument_atomic_write include/linux/instrumented.h:87 [inline]
> > BUG: KASAN: slab-use-after-free in clear_bit include/asm-generic/bitops/instrumented-atomic.h:41 [inline]
> > BUG: KASAN: slab-use-after-free in txEnd+0x2a3/0x5a0 fs/jfs/jfs_txnmgr.c:535
> > Write of size 8 at addr ffff888021bee840 by task jfsCommit/130
> >
> > CPU: 3 PID: 130 Comm: jfsCommit Not tainted 6.3.0-rc7-pasta #1
>
> Is this still pertinent with the current mainline? (There were some
> changes to jfs.)

Thank you very much for your reply and suggestion. I thought that this
BUG still exists in the current mainline kernel version. Since I am
not very familiar with this part of the code, I am not sure if the
proposed fix is correct.

>
> > Through analysis, it was found that a race condition occurred between two
> > functions lmLogClose and txEnd, which were executed in different threads.
> > The possible sequence is as follows:
> >
> > -------------------------------------------------------------------------
> > cpu1(free thread) | cpu2(use thread)
> > -------------------------------------------------------------------------
> > lmLogClose | txEnd
> > | log = JFS_SBI(tblk->sb)->log;
> > sbi->log = NULL; |
> > kfree(log); [1] free log |
> > | clear_bit(log_FLUSH, &log->flag); [2] UAF
>
> That looks sane to a by-passer.
>
> > Fix it by add a mutex lock between lmLogClose and txEnd:
>
> It doesn't feel right wrt "lock data, not code" heuristics.
> And when I apply that, it turns out there's already jfs_log_mutex.
> I'd suggest you explain more why a new lock is needed (if that's the
> preferred solutino).

You're right, I think my fix method is not a good solution. Hoping you
and other developers can help fix it.

>
> Thanks,
> Michal

Once again, I appreciate your help and will take your feedback into
consideration when working on a solution.

Best regards,
Zheng