[syzbot] [nilfs?] KASAN: use-after-free Read in nilfs_find_entry

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 906bd684e4b1 Merge tag 'spi-fix-v6.12-rc6' of git://git.ke..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1418ee30580000
kernel config: https://syzkaller.appspot.com/x/.config?x=64aa0d9945bd5c1
dashboard link: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
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=1218ee30580000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13647f40580000

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/7feb34a89c2a/non_bootable_disk-906bd684.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/88c5c4ba7e33/vmlinux-906bd684.xz
kernel image: https://storage.googleapis.com/syzbot-assets/07094e69f47b/bzImage-906bd684.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/b427c083d0d8/mount_0.gz

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

=======================================================
==================================================================
BUG: KASAN: use-after-free in nilfs_find_entry+0x29c/0x660 fs/nilfs2/dir.c:321
Read of size 2 at addr ffff888048f39008 by task syz-executor334/5310

CPU: 0 UID: 0 PID: 5310 Comm: syz-executor334 Not tainted 6.12.0-rc6-syzkaller-00169-g906bd684e4b1 #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:94 [inline]
dump_stack_lvl+0x241/0x360 lib/dump_stack.c:120
print_address_description mm/kasan/report.c:377 [inline]
print_report+0x169/0x550 mm/kasan/report.c:488
kasan_report+0x143/0x180 mm/kasan/report.c:601
nilfs_find_entry+0x29c/0x660 fs/nilfs2/dir.c:321
nilfs_inode_by_name+0xad/0x240 fs/nilfs2/dir.c:394
nilfs_lookup+0xed/0x210 fs/nilfs2/namei.c:63
lookup_open fs/namei.c:3573 [inline]
open_last_lookups fs/namei.c:3694 [inline]
path_openat+0x11a7/0x3590 fs/namei.c:3930
do_filp_open+0x235/0x490 fs/namei.c:3960
do_sys_openat2+0x13e/0x1d0 fs/open.c:1415
do_sys_open fs/open.c:1430 [inline]
__do_sys_openat fs/open.c:1446 [inline]
__se_sys_openat fs/open.c:1441 [inline]
__x64_sys_openat+0x247/0x2a0 fs/open.c:1441
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fd472823b99
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 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:00007fff91507d48 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
RAX: ffffffffffffffda RBX: 6569727261626f6e RCX: 00007fd472823b99
RDX: 000000000000275a RSI: 0000000020000080 RDI: 00000000ffffff9c
RBP: 00007fd4728975f0 R08: 0000000000000ee3 R09: 00005555568904c0
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff91507d70
R13: 00007fff91507f98 R14: 431bde82d7b634db R15: 00007fd47286c03b
</TASK>

The buggy address belongs to the physical page:
page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x7f0a3df02 pfn:0x48f39
flags: 0x4fff00000000000(node=1|zone=1|lastcpupid=0x7ff)
raw: 04fff00000000000 ffffea000123ce88 ffff88801fc44cb0 0000000000000000
raw: 00000007f0a3df02 0000000000000000 00000000ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as freed
page last allocated via order 0, migratetype Movable, gfp_mask 0x140dca(GFP_HIGHUSER_MOVABLE|__GFP_COMP|__GFP_ZERO), pid 5300, tgid 5300 (sshd), ts 68164329439, free_ts 68237870503
set_page_owner include/linux/page_owner.h:32 [inline]
post_alloc_hook+0x1f3/0x230 mm/page_alloc.c:1537
prep_new_page mm/page_alloc.c:1545 [inline]
get_page_from_freelist+0x303f/0x3190 mm/page_alloc.c:3457
__alloc_pages_noprof+0x292/0x710 mm/page_alloc.c:4733
alloc_pages_mpol_noprof+0x3e8/0x680 mm/mempolicy.c:2265
folio_alloc_mpol_noprof mm/mempolicy.c:2283 [inline]
vma_alloc_folio_noprof+0x12e/0x230 mm/mempolicy.c:2314
folio_prealloc+0x31/0x170
alloc_anon_folio mm/memory.c:4727 [inline]
do_anonymous_page mm/memory.c:4784 [inline]
do_pte_missing mm/memory.c:3963 [inline]
handle_pte_fault+0x24dd/0x6820 mm/memory.c:5766
__handle_mm_fault mm/memory.c:5909 [inline]
handle_mm_fault+0x1106/0x1bb0 mm/memory.c:6077
do_user_addr_fault arch/x86/mm/fault.c:1338 [inline]
handle_page_fault arch/x86/mm/fault.c:1481 [inline]
exc_page_fault+0x459/0x8c0 arch/x86/mm/fault.c:1539
asm_exc_page_fault+0x26/0x30 arch/x86/include/asm/idtentry.h:623
page last free pid 5300 tgid 5300 stack trace:
reset_page_owner include/linux/page_owner.h:25 [inline]
free_pages_prepare mm/page_alloc.c:1108 [inline]
free_unref_folios+0xf12/0x18d0 mm/page_alloc.c:2686
folios_put_refs+0x76c/0x860 mm/swap.c:1007
free_pages_and_swap_cache+0x2ea/0x690 mm/swap_state.c:332
__tlb_batch_free_encoded_pages mm/mmu_gather.c:136 [inline]
tlb_batch_pages_flush mm/mmu_gather.c:149 [inline]
tlb_flush_mmu_free mm/mmu_gather.c:366 [inline]
tlb_flush_mmu+0x3a3/0x680 mm/mmu_gather.c:373
tlb_finish_mmu+0xd4/0x200 mm/mmu_gather.c:465
vms_clear_ptes+0x437/0x530 mm/vma.c:1103
vms_complete_munmap_vmas+0x208/0x910 mm/vma.c:1147
do_vmi_align_munmap+0x613/0x730 mm/vma.c:1356
do_vmi_munmap+0x24e/0x2d0 mm/vma.c:1404
__vm_munmap+0x24c/0x480 mm/mmap.c:1613
__do_sys_munmap mm/mmap.c:1630 [inline]
__se_sys_munmap mm/mmap.c:1627 [inline]
__x64_sys_munmap+0x60/0x70 mm/mmap.c:1627
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f

Memory state around the buggy address:
ffff888048f38f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff888048f38f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff888048f39000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
^
ffff888048f39080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff888048f39100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
==================================================================


---
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

[Edward Adam Davis]

next de space is not enough

#syz test

diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
index a8602729586a..09a24c81dc7d 100644
--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -317,7 +317,7 @@ struct nilfs_dir_entry *nilfs_find_entry(struct inode *dir,

de = (struct nilfs_dir_entry *)kaddr;
kaddr += nilfs_last_byte(dir, n) - reclen;
- while ((char *)de <= kaddr) {
+ while ((char *)de + sizeof(*de) <= kaddr) {
if (de->rec_len == 0) {
nilfs_error(dir->i_sb,
"zero-length directory entry");

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
KASAN: use-after-free Read in nilfs_find_entry

=======================================================
==================================================================
BUG: KASAN: use-after-free in nilfs_find_entry+0x2ad/0x670 fs/nilfs2/dir.c:321
Read of size 2 at addr ffff88805585f008 by task syz.0.15/5797

CPU: 0 UID: 0 PID: 5797 Comm: syz.0.15 Not tainted 6.12.0-rc7-syzkaller-g2d5404caa8c7-dirty #0

Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:94 [inline]
dump_stack_lvl+0x241/0x360 lib/dump_stack.c:120
print_address_description mm/kasan/report.c:377 [inline]
print_report+0x169/0x550 mm/kasan/report.c:488
kasan_report+0x143/0x180 mm/kasan/report.c:601

nilfs_find_entry+0x2ad/0x670 fs/nilfs2/dir.c:321

nilfs_inode_by_name+0xad/0x240 fs/nilfs2/dir.c:394
nilfs_lookup+0xed/0x210 fs/nilfs2/namei.c:63
lookup_open fs/namei.c:3573 [inline]
open_last_lookups fs/namei.c:3694 [inline]
path_openat+0x11a7/0x3590 fs/namei.c:3930
do_filp_open+0x235/0x490 fs/namei.c:3960
do_sys_openat2+0x13e/0x1d0 fs/open.c:1415
do_sys_open fs/open.c:1430 [inline]
__do_sys_openat fs/open.c:1446 [inline]
__se_sys_openat fs/open.c:1441 [inline]
__x64_sys_openat+0x247/0x2a0 fs/open.c:1441
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f

RIP: 0033:0x7fb5e537e719
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 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 a8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fb5e609a038 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
RAX: ffffffffffffffda RBX: 00007fb5e5535f80 RCX: 00007fb5e537e719
RDX: 000000000000275a RSI: 0000000020000080 RDI: ffffffffffffff9c
RBP: 00007fb5e53f139e R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 0000000000000000 R14: 00007fb5e5535f80 R15: 00007fff7a41f408

</TASK>

The buggy address belongs to the physical page:

page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x55be003c9 pfn:0x5585f
flags: 0x4fff00000000000(node=1|zone=1|lastcpupid=0x7ff)
raw: 04fff00000000000 ffffea0001224c08 ffffea000110c308 0000000000000000
raw: 000000055be003c9 0000000000000000 00000000ffffffff 0000000000000000

page dumped because: kasan: bad access detected
page_owner tracks the page as freed

page last allocated via order 0, migratetype Movable, gfp_mask 0x140cca(GFP_HIGHUSER_MOVABLE|__GFP_COMP), pid 5800, tgid 5800 (dhcpcd-run-hook), ts 123081593697, free_ts 123090641935
set_page_owner include/linux/page_owner.h:32 [inline]
post_alloc_hook+0x1f3/0x230 mm/page_alloc.c:1541
prep_new_page mm/page_alloc.c:1549 [inline]
get_page_from_freelist+0x3649/0x3790 mm/page_alloc.c:3459
__alloc_pages_noprof+0x292/0x710 mm/page_alloc.c:4735

alloc_pages_mpol_noprof+0x3e8/0x680 mm/mempolicy.c:2265
folio_alloc_mpol_noprof mm/mempolicy.c:2283 [inline]
vma_alloc_folio_noprof+0x12e/0x230 mm/mempolicy.c:2314
folio_prealloc+0x31/0x170

wp_page_copy mm/memory.c:3353 [inline]
do_wp_page+0x11c4/0x52d0 mm/memory.c:3745
handle_pte_fault+0x10e3/0x6820 mm/memory.c:5782

__handle_mm_fault mm/memory.c:5909 [inline]
handle_mm_fault+0x1106/0x1bb0 mm/memory.c:6077
do_user_addr_fault arch/x86/mm/fault.c:1338 [inline]
handle_page_fault arch/x86/mm/fault.c:1481 [inline]
exc_page_fault+0x459/0x8c0 arch/x86/mm/fault.c:1539
asm_exc_page_fault+0x26/0x30 arch/x86/include/asm/idtentry.h:623

page last free pid 5800 tgid 5800 stack trace:
reset_page_owner include/linux/page_owner.h:25 [inline]
free_pages_prepare mm/page_alloc.c:1112 [inline]
free_unref_folios+0xdb3/0x1750 mm/page_alloc.c:2689

folios_put_refs+0x76c/0x860 mm/swap.c:1007
free_pages_and_swap_cache+0x2ea/0x690 mm/swap_state.c:332
__tlb_batch_free_encoded_pages mm/mmu_gather.c:136 [inline]
tlb_batch_pages_flush mm/mmu_gather.c:149 [inline]
tlb_flush_mmu_free mm/mmu_gather.c:366 [inline]
tlb_flush_mmu+0x3a3/0x680 mm/mmu_gather.c:373
tlb_finish_mmu+0xd4/0x200 mm/mmu_gather.c:465

exit_mmap+0x496/0xc40 mm/mmap.c:1936
__mmput+0x115/0x390 kernel/fork.c:1348
exit_mm+0x220/0x310 kernel/exit.c:571
do_exit+0x9b2/0x28e0 kernel/exit.c:926
do_group_exit+0x207/0x2c0 kernel/exit.c:1088
__do_sys_exit_group kernel/exit.c:1099 [inline]
__se_sys_exit_group kernel/exit.c:1097 [inline]
__x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1097
x64_sys_call+0x2634/0x2640 arch/x86/include/generated/asm/syscalls_64.h:232

do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f

Memory state around the buggy address:

ffff88805585ef00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff88805585ef80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff88805585f000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
^
ffff88805585f080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88805585f100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
==================================================================


Tested on:

commit: 2d5404ca Linux 6.12-rc7
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11ea78c0580000
kernel config: https://syzkaller.appspot.com/x/.config?x=d2aeec8c0b2e420c

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

patch: https://syzkaller.appspot.com/x/patch.diff?x=1271335f980000

[Edward Adam Davis]

calc last byte dec reclen overflow ?

#syz test

diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c

index a8602729586a..f014b7fed5ce 100644
--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -317,7 +317,10 @@ struct nilfs_dir_entry *nilfs_find_entry(struct inode *dir,

de = (struct nilfs_dir_entry *)kaddr;
kaddr += nilfs_last_byte(dir, n) - reclen;
- while ((char *)de <= kaddr) {

+ printk("isize: %u, n: %lu, last byte: %u, reclen: %u, %s\n", dir->i_size, n, nilfs_last_byte(dir, n), reclen, __func__);
+ if (nilfs_last_byte(dir, n) < reclen)
+ break;

[syzbot]

Hello,

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

Reported-by: syzbot+96d5d1...@syzkaller.appspotmail.com
Tested-by: syzbot+96d5d1...@syzkaller.appspotmail.com

Tested on:

commit: 2d5404ca Linux 6.12-rc7
git tree: upstream

console output: https://syzkaller.appspot.com/x/log.txt?x=14ad335f980000

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

patch: https://syzkaller.appspot.com/x/patch.diff?x=129678c0580000

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

[Edward Adam Davis]

The i_size value of the directory "cgroup.controllers" opened by openat is 0,
which causes 0 to be returned when calculating the last valid byte in
nilfs_last_byte(), which ultimately causes kaddr to move forward by reclen
(its value is 32 in this case), which ultimately triggers the uaf when
accessing de->rec_len in nilfs_find_entry().

To avoid this issue, add a check for i_size in nilfs_lookup().

Reported-by: syzbot+96d5d1...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
Signed-off-by: Edward Adam Davis <ead...@qq.com>
---
fs/nilfs2/namei.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/fs/nilfs2/namei.c b/fs/nilfs2/namei.c
index 9b108052d9f7..0b57bcd9c2c5 100644
--- a/fs/nilfs2/namei.c
+++ b/fs/nilfs2/namei.c
@@ -60,6 +60,9 @@ nilfs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
if (dentry->d_name.len > NILFS_NAME_LEN)
return ERR_PTR(-ENAMETOOLONG);

+ if (!dir->i_size)
+ return ERR_PTR(-EINVAL);
+
res = nilfs_inode_by_name(dir, &dentry->d_name, &ino);
if (res) {
if (res != -ENOENT)
--
2.43.0

[Ryusuke Konishi]

Test fix for local variable type in nilfs_last_byte()

#syz test

diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c

index a8602729586a..6bc8f474a3e5 100644
--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -70,7 +70,7 @@ static inline unsigned int nilfs_chunk_size(struct inode *inode)
*/
static unsigned int nilfs_last_byte(struct inode *inode, unsigned long page_nr)
{
- unsigned int last_byte = inode->i_size;
+ loff_t last_byte = inode->i_size;

last_byte -= page_nr << PAGE_SHIFT;
if (last_byte > PAGE_SIZE)

[syzbot]

Hello,

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

Reported-by: syzbot+96d5d1...@syzkaller.appspotmail.com
Tested-by: syzbot+96d5d1...@syzkaller.appspotmail.com

Tested on:

commit: 2d5404ca Linux 6.12-rc7
git tree: upstream

console output: https://syzkaller.appspot.com/x/log.txt?x=13600130580000

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

patch: https://syzkaller.appspot.com/x/patch.diff?x=16d201a7980000

[Ryusuke Konishi]

On Tue, Nov 12, 2024 at 7:56 PM Edward Adam Davis wrote:
>
> The i_size value of the directory "cgroup.controllers" opened by openat is 0,
> which causes 0 to be returned when calculating the last valid byte in
> nilfs_last_byte(), which ultimately causes kaddr to move forward by reclen
> (its value is 32 in this case), which ultimately triggers the uaf when
> accessing de->rec_len in nilfs_find_entry().
>
> To avoid this issue, add a check for i_size in nilfs_lookup().
>
> Reported-by: syzbot+96d5d1...@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> Signed-off-by: Edward Adam Davis <ead...@qq.com>
> ---
> fs/nilfs2/namei.c | 3 +++
> 1 file changed, 3 insertions(+)

Hi Edward, thanks for the debugging help and patch suggestion.

But this fix is incorrect.

Reproducers are not creating the situation where i_size == 0.
In my debug message output inserted in the while loop of
nilfs_find_entry(), i_size was a corrupted large value like this:

NILFS (loop0): nilfs_find_entry: isize=422212465065984,
npages=103079215104, n=0, last_byte=0, reclen=32

This is different from your debug result, because the type of i_size
in the debug patch you sent to syzbot is "%u".
The type of inode->i_size is "loff_t", which is "long long".
Therefore, the output format specification for i_size in the debug
output should be "%lld".

If you look at the beginning of nilfs_find_entry(), you can see that
your check is double-checked:

struct nilfs_dir_entry *nilfs_find_entry(struct inode *dir,

const struct qstr *qstr, struct folio **foliop)
{
...
unsigned long npages = dir_pages(dir);
..

if (npages == 0)
goto out;
...

Here, dir_pages() returns 0 if i_size is 0, so it jumps to "out" and
returns ERR_PTR(-ENOENT).

I'm still debugging, but one problem is that the implementation of
nilfs_last_byte() is incorrect.
In the following part, the local variable "last_byte" is not of type
"loff_t", so depending on the value, it may be truncated and return a
wrong value (0 in this case):

static unsigned int nilfs_last_byte(struct inode *inode, unsigned long page_nr)
{

unsigned int last_byte = inode->i_size;

...
}

If this is the only problem, the following fix will be effective. (To
complete this fix, I think we need to think more carefully about
whether it's okay for i_size to have any value, especially since
loff_t is a signed type):

diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
index a8602729586a..6bc8f474a3e5 100644
--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -70,7 +70,7 @@ static inline unsigned int nilfs_chunk_size(struct
inode *inode)
*/
static unsigned int nilfs_last_byte(struct inode *inode, unsigned long page_nr)
{
- unsigned int last_byte = inode->i_size;
+ loff_t last_byte = inode->i_size;

last_byte -= page_nr << PAGE_SHIFT;
if (last_byte > PAGE_SIZE)

Regards,
Ryusuke Konishi

[Edward Adam Davis]

Yes, you are right, I ignore the type of i_size.

>
> If you look at the beginning of nilfs_find_entry(), you can see that
> your check is double-checked:
>
> struct nilfs_dir_entry *nilfs_find_entry(struct inode *dir,
> const struct qstr *qstr, struct folio **foliop)
> {
> ...
> unsigned long npages = dir_pages(dir);

Yes, now I noticed dir_pages().

I have noticed nilfs_last_byte(), I have other concerns about it, such
as the chance of last_byte overflowing when i_size is too small and page_nr
is too large, or that it will be negative after being type-adjusted to loff_t.
So, maybe following fix is more rigorous.

diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
index a8602729586a..0dbcf91538fd 100644
--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -70,9 +70,10 @@ static inline unsigned int nilfs_chunk_size(struct inode *inode)

*/
static unsigned int nilfs_last_byte(struct inode *inode, unsigned long page_nr)
{
- unsigned int last_byte = inode->i_size;
+ loff_t last_byte = inode->i_size;

- last_byte -= page_nr << PAGE_SHIFT;
+ if (last_byte > page_nr << PAGE_SHIFT)
+ last_byte -= page_nr << PAGE_SHIFT;
if (last_byte > PAGE_SIZE)
last_byte = PAGE_SIZE;
return last_byte;
BR,
Edward

[Ryusuke Konishi]

Test a variation of the nilfs_last_byte() fix in which the type of
the local variable last_byte was changed from "loff_t" to "u64".

Since both PAGE_SIZE and the argument page_nr are unsigned, in
arithmetic and comparison, both are calculated as unsigned by implicit
type conversion, and the behavior is the same.

However, changing the type of last_byte from unsigned to signed
results in a new comparision between an unsigned integer and a signed
integer, which may introduce a new warning from the grammer checker
when using "make W=2", etc., so use the unsigned type in the
declaration.

#syz test

diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
index a8602729586a..f61c58fbf117 100644

--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -70,7 +70,7 @@ static inline unsigned int nilfs_chunk_size(struct inode *inode)
*/
static unsigned int nilfs_last_byte(struct inode *inode, unsigned long page_nr)
{
- unsigned int last_byte = inode->i_size;

+ u64 last_byte = inode->i_size;

[syzbot]

Hello,

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

Reported-by: syzbot+96d5d1...@syzkaller.appspotmail.com
Tested-by: syzbot+96d5d1...@syzkaller.appspotmail.com

Tested on:

commit: f1b785f4 Merge tag 'for_linus' of git://git.kernel.org..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=121ba4c0580000

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

patch: https://syzkaller.appspot.com/x/patch.diff?x=11464ce8580000

[Ryusuke Konishi]

nilfs_last_byte itself does not return an error and is a function that
assumes that i_size is larger than the offset calculated from page_nr,
so let's limit the modification of this function to correcting bit
loss in assignment.

If any caller is missing the necessary range check, add that check to
the caller. I will check again for omissions, but please let me know
if there are any callers that seem to have problems (I hope there
aren't any).

To extend the bits of last_byte, declare last_byte as "u64" instead of "loff_t".
In assignments, the bit pattern is maintained regardless of whether it
is signed or not, and declaring it as u64 also avoids the problem of
negative i_size here.

Comparisons between unsigned and signed integers may introduce
warnings in syntax checks at build time such as "make W=2" depending
on the environment, and may be reported by bots at a later date, so I
would like to maintain comparisons between unsigned integers.
(PAGE_SIZE is an unsigned constant)

If the problem of negative i_size is actually a problem, I think we
should add a sanity check for i_size_read(inode) < 0 to the function
that reads inodes from block devices (such as
nilfs_read_inode_common). So, I would like to deal with that
separately.

I have already tested a change that modifies only the last_byte type
to "u64" with syzbot, but if you could proceed with creating a patch
that includes the commit log in this direction, I would like to adopt
it.

Thanks,
Ryusuke Konishi

[Edward Adam Davis]

Yes, I agree.

>
> To extend the bits of last_byte, declare last_byte as "u64" instead of "loff_t".
> In assignments, the bit pattern is maintained regardless of whether it
> is signed or not, and declaring it as u64 also avoids the problem of
> negative i_size here.
>
> Comparisons between unsigned and signed integers may introduce
> warnings in syntax checks at build time such as "make W=2" depending
> on the environment, and may be reported by bots at a later date, so I
> would like to maintain comparisons between unsigned integers.
> (PAGE_SIZE is an unsigned constant)
>
> If the problem of negative i_size is actually a problem, I think we
> should add a sanity check for i_size_read(inode) < 0 to the function
> that reads inodes from block devices (such as
> nilfs_read_inode_common). So, I would like to deal with that
> separately.
>
> I have already tested a change that modifies only the last_byte type
> to "u64" with syzbot, but if you could proceed with creating a patch
> that includes the commit log in this direction, I would like to adopt
> it.

You are such a nice person.
If I did that, I personally feel that you would suffer a loss.
There will be another chance in the future. I look forward to the next time.

BR,
Edward

[Ryusuke Konishi]

Okay, I'll handle this bug fix.
I don't mind either way, but maybe it was a superfluous suggestion. Never mind.
Well then, maybe another time.

Thanks,
Ryusuke Konishi