Re: [PATCH] FS: JFS: Fix null-ptr-deref Read in txBegin
From: Dave Kleikamp
Date: Fri Jun 23 2023 - 09:49:19 EST
On 6/23/23 8:40AM, Immad Mir wrote:
Thanks for the feedback. I've prepared two patches to fix the bug.
>Does this test case attempt to remount a read-only file system as
read-write? I see a potential bug there.
I'm not really sure about this.
> Should be setting rc to an error here. I suggest -EROFS, but anything
is better than returning zero. Calling jfs_error() might also be in
order, as that would explicitly mark the file system to read-only. (The
default behavior at least.)
I've incorporated your suggested changes.
> It'd be nice if we could move the check to txBegin(), but it is
assumed to always succeed, so there's no good error recovery there
without changing all of the callers. Maybe we can call jfs_error() there
in case we get there from another syscall.
I am not sure what to do here. I am calling jfs_error and returning 0
which is not what the caller would expect.
jfs_error returns void. What it does is mark the superblock dirty and
either panics, marks sets the filesystem read-only, or does nothing
else, depending on a mount flag which I doubt anyone uses, so the
default action is to set it read-only. You still have to set a return
code, etc. after calling it.
I'm sorry I wasn't more specific the first time.
Thanks,
Immad.
On Thu, Jun 22, 2023 at 8:38 PM Dave Kleikamp <dave.kleikamp@xxxxxxxxxx
<mailto:dave.kleikamp@xxxxxxxxxx>> wrote:
On 6/20/23 10:53PM, Immad Mir wrote:
Hi. May I please request a review on this patch.
Sorry for the delay. See below.
Thanks,
Immad
------------------------------------------------------------------------
*From:* mirimmad@xxxxxxxxxxx <mailto:mirimmad@xxxxxxxxxxx>
<mirimmad@xxxxxxxxxxx> <mailto:mirimmad@xxxxxxxxxxx>
*Sent:* Sunday, March 26, 2023 9:51:15 PM
*Cc:* mirimmad@xxxxxxxxxxx <mailto:mirimmad@xxxxxxxxxxx>
<mirimmad@xxxxxxxxxxx> <mailto:mirimmad@xxxxxxxxxxx>;
skhan@xxxxxxxxxxxxxxxxxxx <mailto:skhan@xxxxxxxxxxxxxxxxxxx>
<skhan@xxxxxxxxxxxxxxxxxxx> <mailto:skhan@xxxxxxxxxxxxxxxxxxx>;
Immad Mir <mirimmad17@xxxxxxxxx> <mailto:mirimmad17@xxxxxxxxx>;
syzbot+f1faa20eec55e0c8644c@xxxxxxxxxxxxxxxxxxxxxxxxx
<mailto:syzbot+f1faa20eec55e0c8644c@xxxxxxxxxxxxxxxxxxxxxxxxx>
<syzbot+f1faa20eec55e0c8644c@xxxxxxxxxxxxxxxxxxxxxxxxx>
<mailto:syzbot+f1faa20eec55e0c8644c@xxxxxxxxxxxxxxxxxxxxxxxxx>;
Dave Kleikamp <shaggy@xxxxxxxxxx> <mailto:shaggy@xxxxxxxxxx>;
jfs-discussion@xxxxxxxxxxxxxxxxxxxxx
<mailto:jfs-discussion@xxxxxxxxxxxxxxxxxxxxx>
<jfs-discussion@xxxxxxxxxxxxxxxxxxxxx>
<mailto:jfs-discussion@xxxxxxxxxxxxxxxxxxxxx>;
linux-kernel@xxxxxxxxxxxxxxx <mailto:linux-kernel@xxxxxxxxxxxxxxx>
<linux-kernel@xxxxxxxxxxxxxxx> <mailto:linux-kernel@xxxxxxxxxxxxxxx>
*Subject:* [PATCH] FS: JFS: Fix null-ptr-deref Read in txBegin
From: Immad Mir <mirimmad17@xxxxxxxxx> <mailto:mirimmad17@xxxxxxxxx>
syzkaller reported the following issue:
BUG: KASAN: null-ptr-deref in instrument_atomic_read
include/linux/instrumented.h:72 [inline]
BUG: KASAN: null-ptr-deref in _test_bit
include/asm-generic/bitops/instrumented-non-atomic.h:141 [inline]
BUG: KASAN: null-ptr-deref in txBegin+0x131/0x6c0
fs/jfs/jfs_txnmgr.c:366
Read of size 8 at addr 0000000000000040 by task syz-executor.0/5098
CPU: 0 PID: 5098 Comm: syz-executor.0 Not tainted
6.3.0-rc3-syzkaller-00005-g7d31677bb7b1 #0
Hardware name: Google Compute Engine/Google Compute Engine, BIOS
Google 03/02/2023
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x1e7/0x2d0 lib/dump_stack.c:106
print_report+0xe6/0x540 mm/kasan/report.c:433
kasan_report+0x176/0x1b0 mm/kasan/report.c:536
kasan_check_range+0x283/0x290 mm/kasan/generic.c:187
instrument_atomic_read include/linux/instrumented.h:72 [inline]
_test_bit
include/asm-generic/bitops/instrumented-non-atomic.h:141 [inline]
txBegin+0x131/0x6c0 fs/jfs/jfs_txnmgr.c:366
jfs_link+0x1ac/0x5e0 fs/jfs/namei.c:802
vfs_link+0x4ed/0x680 fs/namei.c:4522
do_linkat+0x5cc/0x9e0 fs/namei.c:4593
__do_sys_linkat fs/namei.c:4621 [inline]
__se_sys_linkat fs/namei.c:4618 [inline]
__x64_sys_linkat+0xdd/0xf0 fs/namei.c:4618
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
The issue can be resolved by checking whethere "log"
for a given superblock exists in the jfs_link function
before beginning a transaction.
I'm not sure how we got here. log should only be null if the file
system is mounted read-only. Does this test case attempt to remount
a read-only file system as read-write? I see a potential bug there.
Tested with syzbot.
Reported-by: syzbot+f1faa20eec55e0c8644c@xxxxxxxxxxxxxxxxxxxxxxxxx
<mailto:syzbot+f1faa20eec55e0c8644c@xxxxxxxxxxxxxxxxxxxxxxxxx>
Link:
https://syzkaller.appspot.com/bug?id=be7e52c50c5182cc09a09ea6fc456446b2039de3 <https://syzkaller.appspot.com/bug?id=be7e52c50c5182cc09a09ea6fc456446b2039de3>
Signed-off-by: Immad Mir <mirimmad17@xxxxxxxxx>
<mailto:mirimmad17@xxxxxxxxx>
---
fs/jfs/namei.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c
index b29d68b5e..cd43b68e2 100644
--- a/fs/jfs/namei.c
+++ b/fs/jfs/namei.c
@@ -799,6 +799,8 @@ static int jfs_link(struct dentry *old_dentry,
if (rc)
goto out;
+ if (!(JFS_SBI(ip->i_sb)->log))
+ goto out;
Should be setting rc to an error here. I suggest -EROFS, but
anything is better than returning zero. Calling jfs_error() might
also be in order, as that would explicitly mark the file system to
read-only. (The default behavior at least.)
tid = txBegin(ip->i_sb, 0);
It'd be nice if we could move the check to txBegin(), but it is
assumed to always succeed, so there's no good error recovery there
without changing all of the callers. Maybe we can call jfs_error()
there in case we get there from another syscall.
mutex_lock_nested(&JFS_IP(dir)->commit_mutex, COMMIT_MUTEX_PARENT);
--
2.40.0