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

[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+96d5d14c47d97015c624@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 syzkaller@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

[PATCH] nilfs2: fix a uaf in nilfs_find_entry

[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+96d5d14c47d97015c624@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
Signed-off-by: Edward Adam Davis <eadavis@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

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

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

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

[syzbot]

Hello,

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

Reported-by: syzbot+96d5d14c47d97015c624@syzkaller.appspotmail.com
Tested-by: syzbot+96d5d14c47d97015c624@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

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

Re: [PATCH] nilfs2: fix a uaf in nilfs_find_entry

[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+96d5d14c47d97015c624@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> Signed-off-by: Edward Adam Davis <eadavis@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


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

Re: [PATCH] nilfs2: fix a uaf in nilfs_find_entry

[Edward Adam Davis]

On Tue, 12 Nov 2024 23:38:11 +0900, Ryusuke Konishi wrote:
> 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+96d5d14c47d97015c624@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> > Signed-off-by: Edward Adam Davis <eadavis@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".
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().
>         ..
> 
>         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)
> 
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
> 
> Regards,
> Ryusuke Konishi
> 
> 
> >
> > 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
> >

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

[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;
 
 	last_byte -= page_nr << PAGE_SHIFT;
 	if (last_byte > PAGE_SIZE)

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

[syzbot]

Hello,

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

Reported-by: syzbot+96d5d14c47d97015c624@syzkaller.appspotmail.com
Tested-by: syzbot+96d5d14c47d97015c624@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

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

Re: [PATCH] nilfs2: fix a uaf in nilfs_find_entry

[Ryusuke Konishi]

On Wed, Nov 13, 2024 at 11:28 AM Edward Adam Davis wrote:
>
> On Tue, 12 Nov 2024 23:38:11 +0900, Ryusuke Konishi wrote:
> > 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+96d5d14c47d97015c624@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> > > Signed-off-by: Edward Adam Davis <eadavis@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".
> 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().
> >         ..
> >
> >         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)
> >
> 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

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

Re: [PATCH] nilfs2: fix a uaf in nilfs_find_entry

[Edward Adam Davis]

On Wed, 13 Nov 2024 23:54:39 +0900, Ryusuke Konishi wrote:
> On Wed, Nov 13, 2024 at 11:28 AM Edward Adam Davis wrote:
> >
> > On Tue, 12 Nov 2024 23:38:11 +0900, Ryusuke Konishi wrote:
> > > 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+96d5d14c47d97015c624@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> > > > Signed-off-by: Edward Adam Davis <eadavis@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".
> > 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().
> > >         ..
> > >
> > >         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)
> > >
> > 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
> 
> 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).
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

Re: [PATCH] nilfs2: fix a uaf in nilfs_find_entry

[Ryusuke Konishi]

On Thu, Nov 14, 2024 at 9:01 PM Edward Adam Davis wrote:
>
> On Wed, 13 Nov 2024 23:54:39 +0900, Ryusuke Konishi wrote:
> > On Wed, Nov 13, 2024 at 11:28 AM Edward Adam Davis wrote:
> > >
> > > On Tue, 12 Nov 2024 23:38:11 +0900, Ryusuke Konishi wrote:
> > > > 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+96d5d14c47d97015c624@syzkaller.appspotmail.com
> > > > > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> > > > > Signed-off-by: Edward Adam Davis <eadavis@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".
> > > 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().
> > > >         ..
> > > >
> > > >         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)
> > > >
> > > 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
> >
> > 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).
> 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

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

[PATCH] nilfs2: fix potential out-of-bounds memory access in nilfs_find_entry()

[Ryusuke Konishi]

Syzbot reported that when searching for records in a directory where
the inode's i_size is corrupted and has a large value, memory access
outside the folio/page range may occur, or a use-after-free bug may be
detected if KASAN is enabled.

This is because nilfs_last_byte(), which is called by
nilfs_find_entry() and others to calculate the number of valid bytes of
directory data in a page from i_size and the page index, loses the
upper 32 bits of the 64-bit size information due to an inappropriate
type of local variable to which the i_size value is assigned.

This caused a large byte offset value due to underflow in the end
address calculation in the calling nilfs_find_entry(), resulting in
memory access that exceeds the folio/page size.

Fix this issue by changing the type of the local variable causing the
bit loss from "unsigned int" to "u64".  The return value of
nilfs_last_byte() is also of type "unsigned int", but it is truncated
so as not to exceed PAGE_SIZE and no bit loss occurs, so no change is
required.

Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Reported-by: syzbot+96d5d14c47d97015c624@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
Tested-by: syzbot+96d5d14c47d97015c624@syzkaller.appspotmail.com
Fixes: 2ba466d74ed7 ("nilfs2: directory entry operations")
Cc: stable@vger.kernel.org
---
Andrew, please apply this as a bug fix.

This fixes a potential out-of-page memory access bug recently
reported by syzbot.

Thanks,
Ryusuke Konishi

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

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;
 
 	last_byte -= page_nr << PAGE_SHIFT;
 	if (last_byte > PAGE_SIZE)
-- 
2.43.0