[syzbot] [fs?] KASAN: null-ptr-deref Write in get_block (2)

[syzbot] [fs?] KASAN: null-ptr-deref Write in get_block (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    4d6d4c7f541d Merge tag 'linux-kselftest-fixes-6.4-rc3' of ..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=17b34a5a280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=94af80bb8ddd23c4
dashboard link: https://syzkaller.appspot.com/bug?extid=aad58150cbc64ba41bdc
compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1615fbe9280000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1282842e280000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/dcd8898335fc/disk-4d6d4c7f.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/6a1f7abe57aa/vmlinux-4d6d4c7f.xz
kernel image: https://storage.googleapis.com/syzbot-assets/b485f41c18e6/bzImage-4d6d4c7f.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/19be7546cd7d/mount_1.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+aad58150cbc64ba41bdc@syzkaller.appspotmail.com

memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=4996 'syz-executor412'
loop0: detected capacity change from 0 to 128
VFS: Found a Xenix FS (block size = 512) on device loop0
sysv_free_block: trying to free block not in datazone
==================================================================
BUG: KASAN: null-ptr-deref in instrument_atomic_read_write include/linux/instrumented.h:96 [inline]
BUG: KASAN: null-ptr-deref in test_and_set_bit_lock include/asm-generic/bitops/instrumented-lock.h:57 [inline]
BUG: KASAN: null-ptr-deref in trylock_buffer include/linux/buffer_head.h:399 [inline]
BUG: KASAN: null-ptr-deref in lock_buffer include/linux/buffer_head.h:405 [inline]
BUG: KASAN: null-ptr-deref in alloc_branch fs/sysv/itree.c:148 [inline]
BUG: KASAN: null-ptr-deref in get_block+0x567/0x16a0 fs/sysv/itree.c:251
Write of size 8 at addr 0000000000000000 by task syz-executor412/4996

CPU: 1 PID: 4996 Comm: syz-executor412 Not tainted 6.4.0-rc2-syzkaller-00018-g4d6d4c7f541d #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/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:465
 kasan_report+0x176/0x1b0 mm/kasan/report.c:572
 kasan_check_range+0x283/0x290 mm/kasan/generic.c:187
 instrument_atomic_read_write include/linux/instrumented.h:96 [inline]
 test_and_set_bit_lock include/asm-generic/bitops/instrumented-lock.h:57 [inline]
 trylock_buffer include/linux/buffer_head.h:399 [inline]
 lock_buffer include/linux/buffer_head.h:405 [inline]
 alloc_branch fs/sysv/itree.c:148 [inline]
 get_block+0x567/0x16a0 fs/sysv/itree.c:251
 __block_write_begin_int+0x548/0x1a50 fs/buffer.c:2064
 __block_write_begin fs/buffer.c:2114 [inline]
 block_write_begin+0x9c/0x1f0 fs/buffer.c:2175
 sysv_write_begin+0x31/0x70 fs/sysv/itree.c:485
 generic_perform_write+0x300/0x5e0 mm/filemap.c:3923
 __generic_file_write_iter+0x17a/0x400 mm/filemap.c:4051
 generic_file_write_iter+0xaf/0x310 mm/filemap.c:4083
 do_iter_write+0x7b1/0xcb0 fs/read_write.c:860
 vfs_writev fs/read_write.c:933 [inline]
 do_pwritev+0x21a/0x360 fs/read_write.c:1030
 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
RIP: 0033:0x7f3233222b19
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 51 14 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffecf15f268 EFLAGS: 00000246 ORIG_RAX: 0000000000000128
RAX: ffffffffffffffda RBX: 0031656c69662f2e RCX: 00007f3233222b19
RDX: 0000000000000005 RSI: 0000000020000480 RDI: 0000000000000004
RBP: 00007f32331e2150 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000007fff R11: 0000000000000246 R12: 00007f32331e21e0
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
 </TASK>
==================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[PATCH] Null check to prevent null-ptr-deref bug

[Prince Kumar Maurya]

sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on that leads to the null-ptr-deref bug.

Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
---
 fs/sysv/itree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
index b22764fe669c..3a6b66e719fd 100644
--- a/fs/sysv/itree.c
+++ b/fs/sysv/itree.c
@@ -145,6 +145,8 @@ static int alloc_branch(struct inode *inode,
 		 */
 		parent = block_to_cpu(SYSV_SB(inode->i_sb), branch[n-1].key);
 		bh = sb_getblk(inode->i_sb, parent);
+		if (!bh)
+			break;
 		lock_buffer(bh);
 		memset(bh->b_data, 0, blocksize);
 		branch[n].bh = bh;
-- 
2.40.1

Re: [PATCH] Null check to prevent null-ptr-deref bug

[Greg KH]

On Sat, May 27, 2023 at 06:25:16PM -0700, Prince Kumar Maurya wrote:
> sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on that leads to the null-ptr-deref bug.

Please wrap your changelog comments at 72 columns.

> Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
> ---
>  fs/sysv/itree.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
> index b22764fe669c..3a6b66e719fd 100644
> --- a/fs/sysv/itree.c
> +++ b/fs/sysv/itree.c
> @@ -145,6 +145,8 @@ static int alloc_branch(struct inode *inode,
>  		 */
>  		parent = block_to_cpu(SYSV_SB(inode->i_sb), branch[n-1].key);
>  		bh = sb_getblk(inode->i_sb, parent);
> +		if (!bh)
> +			break;

How have you tested this?  Have you reproduced this failure and has this
patch resolved the issue?  This function should never really fail in
normal operation, so it's odd that you were able to hit this somehow.

thanks,

greg k-h

[PATCH] Null check to prevent null-ptr-deref bug

[Prince Kumar Maurya]

sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on that leads to the null-ptr-deref bug.

Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
---
 fs/sysv/itree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
index b22764fe669c..3a6b66e719fd 100644
--- a/fs/sysv/itree.c
+++ b/fs/sysv/itree.c
@@ -145,6 +145,8 @@ static int alloc_branch(struct inode *inode,
 		 */
 		parent = block_to_cpu(SYSV_SB(inode->i_sb), branch[n-1].key);
 		bh = sb_getblk(inode->i_sb, parent);
+		if (!bh)
+			break;
 		lock_buffer(bh);
 		memset(bh->b_data, 0, blocksize);
 		branch[n].bh = bh;
-- 
2.40.1

Re: [PATCH] Null check to prevent null-ptr-deref bug

[Greg KH]

On Sun, May 28, 2023 at 09:44:00AM -0700, Prince Kumar Maurya wrote:
> sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on that leads to the null-ptr-deref bug.
> 
> Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
> ---
>  fs/sysv/itree.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
> index b22764fe669c..3a6b66e719fd 100644
> --- a/fs/sysv/itree.c
> +++ b/fs/sysv/itree.c
> @@ -145,6 +145,8 @@ static int alloc_branch(struct inode *inode,
>  		 */
>  		parent = block_to_cpu(SYSV_SB(inode->i_sb), branch[n-1].key);
>  		bh = sb_getblk(inode->i_sb, parent);
> +		if (!bh)
> +			break;
>  		lock_buffer(bh);
>  		memset(bh->b_data, 0, blocksize);
>  		branch[n].bh = bh;
> -- 
> 2.40.1

Why resend this when I already responded:
	https://lore.kernel.org/r/2023052803-pucker-depress-5452@gregkh

confused,

greg k-h

[PATCH] Null check to prevent null-ptr-deref bug

[Prince Kumar Maurya]

sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on
that leads to the null-ptr-deref bug.

Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
---
Change since v1: update the commit message.
The bug was reproducible using the reproducer code and assets found in
bug report:https://syzkaller.appspot.com/bug?extid=aad58150cbc64ba41bdc
I used qemu to reproduce the bug and after the code fix I rebooted the 
qemu with updated bzImage containing the fix.

qemu-system-x86_64 -m 4G -nographic -drive \
file=./asset/disk-4d6d4c7f.raw,format=raw \
-enable-kvm -net nic -net user,hostfwd=tcp::2222-:22

 fs/sysv/itree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
index b22764fe669c..3a6b66e719fd 100644
--- a/fs/sysv/itree.c
+++ b/fs/sysv/itree.c
@@ -145,6 +145,8 @@ static int alloc_branch(struct inode *inode,
 		 */
 		parent = block_to_cpu(SYSV_SB(inode->i_sb), branch[n-1].key);
 		bh = sb_getblk(inode->i_sb, parent);
+		if (!bh)
+			break;
 		lock_buffer(bh);
 		memset(bh->b_data, 0, blocksize);
 		branch[n].bh = bh;
-- 
2.40.1

Re: [PATCH] Null check to prevent null-ptr-deref bug

[Greg KH]

On Sun, May 28, 2023 at 10:35:46AM -0700, Prince Kumar Maurya wrote:
> sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on
> that leads to the null-ptr-deref bug.
> 
> Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
> ---
> Change since v1: update the commit message.

Your subject: line needs to also be fixed up.  Please see the kernel
documentation for how to do this properly.

thanks

greg k-h

Re: [PATCH] Null check to prevent null-ptr-deref bug

[Greg KH]

On Sun, May 28, 2023 at 10:35:46AM -0700, Prince Kumar Maurya wrote:
> sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on
> that leads to the null-ptr-deref bug.
> 
> Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
> ---
> Change since v1: update the commit message.
> The bug was reproducible using the reproducer code and assets found in
> bug report:https://syzkaller.appspot.com/bug?extid=aad58150cbc64ba41bdc

Also, did you forget to properly credit the syzbot tool like it asks you
to?

[PATCH v3] fs/sysv: Null check to prevent null-ptr-deref bug

[Prince Kumar Maurya]

sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on
that leads to the null-ptr-deref bug.

Reported-by: syzbot+aad58150cbc64ba41bdc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=aad58150cbc64ba41bdc 
Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
---
Change since v2: Updated subject and added Reported-by and closes tags.

 fs/sysv/itree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
index b22764fe669c..3a6b66e719fd 100644
--- a/fs/sysv/itree.c
+++ b/fs/sysv/itree.c
@@ -145,6 +145,8 @@ static int alloc_branch(struct inode *inode,
 		 */
 		parent = block_to_cpu(SYSV_SB(inode->i_sb), branch[n-1].key);
 		bh = sb_getblk(inode->i_sb, parent);
+		if (!bh)
+			break;
 		lock_buffer(bh);
 		memset(bh->b_data, 0, blocksize);
 		branch[n].bh = bh;
-- 
2.40.1

Re: [PATCH v3] fs/sysv: Null check to prevent null-ptr-deref bug

[Christian Brauner]

On Sun, May 28, 2023 at 11:44:22AM -0700, Prince Kumar Maurya wrote:
> sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on
> that leads to the null-ptr-deref bug.
> 
> Reported-by: syzbot+aad58150cbc64ba41bdc@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=aad58150cbc64ba41bdc 
> Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
> ---
> Change since v2: Updated subject and added Reported-by and closes tags.
> 
>  fs/sysv/itree.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
> index b22764fe669c..3a6b66e719fd 100644
> --- a/fs/sysv/itree.c
> +++ b/fs/sysv/itree.c
> @@ -145,6 +145,8 @@ static int alloc_branch(struct inode *inode,
>  		 */
>  		parent = block_to_cpu(SYSV_SB(inode->i_sb), branch[n-1].key);
>  		bh = sb_getblk(inode->i_sb, parent);
> +		if (!bh)
> +			break;

When you break here you'll hit:

/* Allocation failed, free what we already allocated */
for (i = 1; i < n; i++)
        bforget(branch[i].bh);
for (i = 0; i < n; i++)
        sysv_free_block(inode->i_sb, branch[i].key);

below. The cleanup paths were coded in the assumption that sb_getblk()
can't fail. So bforget() can assume that branch[i].bh has been allocated
and set up. So that bforget(branch[i].bh) is your next pending NULL
deref afaict.

Re: [PATCH v3] fs/sysv: Null check to prevent null-ptr-deref bug

[Prince Kumar Maurya]

On Tue, May 30, 2023 at 1:26 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Sun, May 28, 2023 at 11:44:22AM -0700, Prince Kumar Maurya wrote:
> > sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on
> > that leads to the null-ptr-deref bug.
> >
> > Reported-by: syzbot+aad58150cbc64ba41bdc@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=aad58150cbc64ba41bdc
> > Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
> > ---
> > Change since v2: Updated subject and added Reported-by and closes tags.
> >
> >  fs/sysv/itree.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
> > index b22764fe669c..3a6b66e719fd 100644
> > --- a/fs/sysv/itree.c
> > +++ b/fs/sysv/itree.c
> > @@ -145,6 +145,8 @@ static int alloc_branch(struct inode *inode,
> >                */
> >               parent = block_to_cpu(SYSV_SB(inode->i_sb), branch[n-1].key);
> >               bh = sb_getblk(inode->i_sb, parent);
> > +             if (!bh)
> > +                     break;
>
> When you break here you'll hit:
>
> /* Allocation failed, free what we already allocated */
> for (i = 1; i < n; i++)
>         bforget(branch[i].bh);
> for (i = 0; i < n; i++)
>         sysv_free_block(inode->i_sb, branch[i].key);
>
> below. The cleanup paths were coded in the assumption that sb_getblk()
> can't fail. So bforget() can assume that branch[i].bh has been allocated
> and set up. So that bforget(branch[i].bh) is your next pending NULL
> deref afaict.


I doubt that would happen. There is a break above as well, before we do
sb_getblk().

/* Allocate the next block */
branch[n].key = sysv_new_block(inode->i_sb);
if (!branch[n].key)
   break;

The clean up code path runs till i is less than n not equal to n which
would have caused the problem.

Re: [PATCH v3] fs/sysv: Null check to prevent null-ptr-deref bug

[Christian Brauner]

On Tue, May 30, 2023 at 07:59:16AM -0700, Prince Kumar Maurya wrote:
> On Tue, May 30, 2023 at 1:26 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Sun, May 28, 2023 at 11:44:22AM -0700, Prince Kumar Maurya wrote:
> > > sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on
> > > that leads to the null-ptr-deref bug.
> > >
> > > Reported-by: syzbot+aad58150cbc64ba41bdc@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=aad58150cbc64ba41bdc
> > > Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
> > > ---
> > > Change since v2: Updated subject and added Reported-by and closes tags.
> > >
> > >  fs/sysv/itree.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
> > > index b22764fe669c..3a6b66e719fd 100644
> > > --- a/fs/sysv/itree.c
> > > +++ b/fs/sysv/itree.c
> > > @@ -145,6 +145,8 @@ static int alloc_branch(struct inode *inode,
> > >                */
> > >               parent = block_to_cpu(SYSV_SB(inode->i_sb), branch[n-1].key);
> > >               bh = sb_getblk(inode->i_sb, parent);
> > > +             if (!bh)
> > > +                     break;
> >
> > When you break here you'll hit:
> >
> > /* Allocation failed, free what we already allocated */
> > for (i = 1; i < n; i++)
> >         bforget(branch[i].bh);
> > for (i = 0; i < n; i++)
> >         sysv_free_block(inode->i_sb, branch[i].key);
> >
> > below. The cleanup paths were coded in the assumption that sb_getblk()
> > can't fail. So bforget() can assume that branch[i].bh has been allocated
> > and set up. So that bforget(branch[i].bh) is your next pending NULL
> > deref afaict.
> 
> 
> I doubt that would happen. There is a break above as well, before we do
> sb_getblk().
> 
> /* Allocate the next block */
> branch[n].key = sysv_new_block(inode->i_sb);
> if (!branch[n].key)
>    break;
> 
> The clean up code path runs till i is less than n not equal to n which
> would have caused the problem.

But then aren't you leaking branch[n].key if you break after failed sb_getblk()
after sysv_new_block() succeeded?

[PATCH v4] fs/sysv: Null check to prevent null-ptr-deref bug

[Prince Kumar Maurya]

sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on
that leads to the null-ptr-deref bug.

Reported-by: syzbot+aad58150cbc64ba41bdc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=aad58150cbc64ba41bdc 
Signed-off-by: Prince Kumar Maurya <princekumarmaurya06@gmail.com>
---
Change since v3: Added cleanup code for the branch[n].key on 
failure code path.

 fs/sysv/itree.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
index b22764fe669c..58d7f43a1371 100644
--- a/fs/sysv/itree.c
+++ b/fs/sysv/itree.c
@@ -145,6 +145,10 @@ static int alloc_branch(struct inode *inode,
 		 */
 		parent = block_to_cpu(SYSV_SB(inode->i_sb), branch[n-1].key);
 		bh = sb_getblk(inode->i_sb, parent);
+		if (!bh) {
+			sysv_free_block(inode->i_sb, branch[n].key);
+			break;
+		}
 		lock_buffer(bh);
 		memset(bh->b_data, 0, blocksize);
 		branch[n].bh = bh;
-- 
2.40.1

Re: [PATCH v4] fs/sysv: Null check to prevent null-ptr-deref bug

[Christian Brauner]

On Sun, 28 May 2023 11:44:22 -0700, Prince Kumar Maurya wrote:
> sb_getblk(inode->i_sb, parent) return a null ptr and taking lock on
> that leads to the null-ptr-deref bug.
> 
> 

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] fs/sysv: Null check to prevent null-ptr-deref bug
      https://git.kernel.org/vfs/vfs/c/47f9da4bc5e6