[syzbot] [jfs?] UBSAN: shift-out-of-bounds in extAlloc (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 9221b2819b8a Add linux-next specific files for 20240503
git tree: linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=14631754980000
kernel config: https://syzkaller.appspot.com/x/.config?x=8ab537f51a6a0d98
dashboard link: https://syzkaller.appspot.com/bug?extid=13e8cd4926977f8337b6
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15123b1f180000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16b7da2f180000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/3e67dbdc3c37/disk-9221b281.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/ade618fa19f8/vmlinux-9221b281.xz
kernel image: https://storage.googleapis.com/syzbot-assets/df12e5073c97/bzImage-9221b281.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/41dea5c977c2/mount_0.gz

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

loop0: detected capacity change from 0 to 32768
------------[ cut here ]------------
UBSAN: shift-out-of-bounds in fs/jfs/jfs_extent.c:319:16
shift exponent 108 is too large for 64-bit type 's64' (aka 'long long')
CPU: 0 PID: 5090 Comm: syz-executor421 Not tainted 6.9.0-rc6-next-20240503-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x241/0x360 lib/dump_stack.c:114
ubsan_epilogue lib/ubsan.c:231 [inline]
__ubsan_handle_shift_out_of_bounds+0x3c8/0x420 lib/ubsan.c:468
extBalloc fs/jfs/jfs_extent.c:319 [inline]
extAlloc+0xe5c/0x1010 fs/jfs/jfs_extent.c:122
jfs_get_block+0x41b/0xe60 fs/jfs/inode.c:248
__block_write_begin_int+0x50c/0x1a70 fs/buffer.c:2128
__block_write_begin fs/buffer.c:2177 [inline]
block_write_begin+0x9b/0x1e0 fs/buffer.c:2236
jfs_write_begin+0x31/0x70 fs/jfs/inode.c:299
generic_perform_write+0x322/0x640 mm/filemap.c:4016
generic_file_write_iter+0xaf/0x310 mm/filemap.c:4137
new_sync_write fs/read_write.c:497 [inline]
vfs_write+0xa72/0xc90 fs/read_write.c:590
ksys_write+0x1a0/0x2c0 fs/read_write.c:643
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f4d15f6f639
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 61 17 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 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fff3dae85f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007fff3dae87c8 RCX: 00007f4d15f6f639
RDX: 00000000fffffef2 RSI: 0000000020000240 RDI: 0000000000000004
RBP: 00007f4d15fe8610 R08: 0000000000000000 R09: 00007fff3dae87c8
R10: 0000000000006162 R11: 0000000000000246 R12: 0000000000000001
R13: 00007fff3dae87b8 R14: 0000000000000001 R15: 0000000000000001
</TASK>
---[ end trace ]---


---
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 syzk...@googlegroups.com.

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

If the report is already addressed, 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 overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

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

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

[Jeongjun Park]

please test shift-out-of-bounds in extAlloc

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

---
fs/jfs/jfs_extent.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/jfs/jfs_extent.c b/fs/jfs/jfs_extent.c
index 63d21822d309..7701159422a1 100644
--- a/fs/jfs/jfs_extent.c
+++ b/fs/jfs/jfs_extent.c
@@ -313,7 +313,7 @@ extBalloc(struct inode *ip, s64 hint, s64 * nblocks, s64 * blkno)
*/

/* give up if no space left */
- if (bmp->db_maxfreebud == -1)
+ if (bmp->db_maxfreebud >= 0 && bmp->db_maxfreebud < 63)
return -ENOSPC;

max = (s64) 1 << bmp->db_maxfreebud;
--
2.34.1

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+13e8cd...@syzkaller.appspotmail.com

Tested on:

commit: 4a4be1ad Revert "vfs: Delete the associated dentry whe..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14f8f016980000
kernel config: https://syzkaller.appspot.com/x/.config?x=b9016f104992d69c

dashboard link: https://syzkaller.appspot.com/bug?extid=13e8cd4926977f8337b6
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=1001cc4a980000

Note: testing is done by a robot and is best-effort only.

[Jeongjun Park]

Hi,

Thanks to the points you pointed out, I was able to confirm that there
was a rudimentary mistake in my patch. And as you suggested, I think
it would be a good idea to handle the two situations separately.

However, if you only check when there is a value of sizeof(s64) * 8 or
more in bmp->db_maxfreebud, the value will change to a negative number
when bmp->db_maxfreebud is 63 because max is an s64 data type.
Therefore, I think it is right to slightly modify the conditions.

Regards.

---
fs/jfs/jfs_extent.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/fs/jfs/jfs_extent.c b/fs/jfs/jfs_extent.c
index 63d21822d309..3d1273d35b13 100644
--- a/fs/jfs/jfs_extent.c
+++ b/fs/jfs/jfs_extent.c
@@ -316,6 +316,9 @@ extBalloc(struct inode *ip, s64 hint, s64 * nblocks, s64 * blkno)

if (bmp->db_maxfreebud == -1)

return -ENOSPC;

+ if (bmp->db_maxfreebud >= sizeof(s64) * 8 - 1)
+ return -EINVAL;
+

max = (s64) 1 << bmp->db_maxfreebud;

if (*nblocks >= max && *nblocks > nbperpage)
nb = nblks = (max > nbperpage) ? max : nbperpage;
--
2.34.1

[Jeongjun Park]

please test shift-out-of-bounds in extAlloc

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+13e8cd...@syzkaller.appspotmail.com

Tested on:

commit: cc8ed4d0 Merge tag 'drm-fixes-2024-06-01' of https://g..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=139c8426980000

kernel config: https://syzkaller.appspot.com/x/.config?x=b9016f104992d69c
dashboard link: https://syzkaller.appspot.com/bug?extid=13e8cd4926977f8337b6
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=15851dd6980000

[Changheon LEE]

Hello, park.

Your proposed patch seems to have some glaring flaws, which I'd like
to quickly point out and then suggest a simpler and clearer patch.

The part of the source code that is the subject of your patch performs
a function that checks for free space in the block map and returns
-ENOSPC if there is no free space.

Also, based on the stack trace, it is clear that this issue is caused
by using a shift exponent (128) that is too large for S64.

Your proposed patch doesn't check the shift exponent, but rather makes
it return -ENOSPC if 'bmp->db_maxfreebud' is greater than 0 and less
than 63, which is causing a logical error to return -ENOSPC when there
is actually no free space.

Also, it seems like it would be better to separate checking for free
space in the block map from checking the range of the shift index.

Based on the suggestions above, I propose the following patch for this issue.

---.
diff --git a/fs/jfs/jfs_extent.c b/fs/jfs/jfs_extent.c
Index 63d21822d309..b1a2c3d4e5f6 100644
--- a/fs/jfs/jfs_extent.c
+++ b/fs/jfs/jfs_extent.c
@@ -315,6 +315,10 @@ extBalloc(struct inode *ip, s64 hint, s64 *
nblocks, s64 * blkno)
Returns -ENOSPC;

+ /* Check if the shift exponent is within a valid range */.
+ if (bmp->db_maxfreebud >= sizeof(s64) * 8)
+ return -EINVAL; /* Invalid index */ + }
+ }

max = (s64) 1 << bmp->db_maxfreebud;

if (*nblocks >= max && *nblocks > nbperpage)
nb = nblks = (max > nbperpage) ? max : nbperpage;

else
---

best regards.

Changheon Lee.

[ChangHeon LEE]

Hello, park.

Your proposed patch seems to have some glaring flaws, which I'd like
to quickly point out and then suggest a simpler and clearer patch.

The part of the source code that is the subject of your patch performs
a function that checks for free space in the block map and returns
-ENOSPC if there is no free space.

Also, based on the stack trace, it is clear that this issue is caused
by using a shift exponent (128) that is too large for S64.

Your proposed patch doesn't check the shift exponent, but rather makes
it return -ENOSPC if 'bmp->db_maxfreebud' is greater than 0 and less
than 63, which is causing a logical error to return -ENOSPC when there
is actually no free space.

Also, it seems like it would be better to separate checking for free
space in the block map from checking the range of the shift index.

Based on the suggestions above, I propose the following patch for this issue.

---.
diff --git a/fs/jfs/jfs_extent.c b/fs/jfs/jfs_extent.c

Index 63d21822d309..b1a2c3d4e5f6 100644
--- a/fs/jfs/jfs_extent.c
+++ b/fs/jfs/jfs_extent.c
@@ -315,6 +315,10 @@ extBalloc(struct inode *ip, s64 hint, s64 *
nblocks, s64 * blkno)

         Returns -ENOSPC;

+ /* Check if the shift exponent is within a valid range */.
+ if (bmp->db_maxfreebud >= sizeof(s64) * 8)
+ return -EINVAL; /* Invalid index */ + }
+ }

     max = (s64) 1 << bmp->db_maxfreebud;

     if (*nblocks >= max && *nblocks > nbperpage)
         nb = nblks = (max > nbperpage) ? max : nbperpage;
     else
---

best regards.

Changheon Lee.

2024년 6월 1일 토요일 오전 3시 18분 5초 UTC+9에 syzbot님이 작성: