linux-kernel - Re: [PATCH V2] nilfs2: prevent use of deleted inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKFNMo=ck+c2NJHVOFszzT02ksF1a0KG9vA5zU+Woa7noLeFrA@mail.gmail.com>
Date: Sun, 8 Dec 2024 13:39:16 +0900
From: Ryusuke Konishi <konishi.ryusuke@...il.com>
To: Edward Adam Davis <eadavis@...com>
Cc: linux-kernel@...r.kernel.org, linux-nilfs@...r.kernel.org, 
	syzbot+9260555647a5132edd48@...kaller.appspotmail.com, 
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH V2] nilfs2: prevent use of deleted inode

On Sun, Dec 8, 2024 at 12:24 PM Edward Adam Davis wrote:
>
> syzbot reported a WARNING in nilfs_rmdir. [1]
>
> Because the inode bitmap is corrupted, an inode with an inode number
> that should exist as a ".nilfs" file was reassigned by nilfs_mkdir for
> "file0", causing an inode duplication during execution.
> And this causes an underflow of i_nlink in rmdir operations.
>
> Avoid to this issue, check i_nlink in nilfs_iget(), if it is 0, it means
> that this inode has been deleted, and iput is executed to reclaim it.
>
> [1]
> WARNING: CPU: 1 PID: 5824 at fs/inode.c:407 drop_nlink+0xc4/0x110 fs/inode.c:407
> Modules linked in:
> CPU: 1 UID: 0 PID: 5824 Comm: syz-executor223 Not tainted 6.12.0-syzkaller-12113-gbcc8eda6d349 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/13/2024
> RIP: 0010:drop_nlink+0xc4/0x110 fs/inode.c:407
> Code: bb 70 07 00 00 be 08 00 00 00 e8 57 0b e6 ff f0 48 ff 83 70 07 00 00 5b 41 5c 41 5e 41 5f 5d c3 cc cc cc cc e8 9d 4c 7e ff 90 <0f> 0b 90 eb 83 44 89 e1 80 e1 07 80 c1 03 38 c1 0f 8c 5c ff ff ff
> RSP: 0018:ffffc900037f7c70 EFLAGS: 00010293
> RAX: ffffffff822124a3 RBX: 1ffff1100e7ae034 RCX: ffff88807cf53c00
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> RBP: 0000000000000000 R08: ffffffff82212423 R09: 1ffff1100f8ba8ee
> R10: dffffc0000000000 R11: ffffed100f8ba8ef R12: ffff888073d701a0
> R13: 1ffff1100e79f5c4 R14: ffff888073d70158 R15: dffffc0000000000
> FS:  0000555558d1e480(0000) GS:ffff8880b8700000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000555558d37878 CR3: 000000007d920000 CR4: 00000000003526f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>  nilfs_rmdir+0x1b0/0x250 fs/nilfs2/namei.c:342
>  vfs_rmdir+0x3a3/0x510 fs/namei.c:4394
>  do_rmdir+0x3b5/0x580 fs/namei.c:4453
>  __do_sys_rmdir fs/namei.c:4472 [inline]
>  __se_sys_rmdir fs/namei.c:4470 [inline]
>  __x64_sys_rmdir+0x47/0x50 fs/namei.c:4470
>  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
>

I'll make a few comments.

> Reported-and-tested-by: syzbot+9260555647a5132edd48@...kaller.appspotmail.com

First please separate "Reported-and-tested-by" into two tags.
Although it is still seen occasionally, it causes warnings for the
checkpatch script. (It has already been explicitly deprecated in some
subtrees, because it just complicates automatic tag extraction.)

> Closes: https://syzkaller.appspot.com/bug?extid=9260555647a5132edd48
> Signed-off-by: Edward Adam Davis <eadavis@...com>
> ---
> V1 -> V2: Adjust the patch as suggested by Ryusuke Konishi
>
>  fs/nilfs2/inode.c | 8 +++++++-
>  fs/nilfs2/namei.c | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index cf9ba481ae37..b7d4105f37bf 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -544,8 +544,14 @@ struct inode *nilfs_iget(struct super_block *sb, struct nilfs_root *root,
>         inode = nilfs_iget_locked(sb, root, ino);
>         if (unlikely(!inode))
>                 return ERR_PTR(-ENOMEM);
> -       if (!(inode->i_state & I_NEW))
> +
> +       if (!(inode->i_state & I_NEW)) {
> +               if (!inode->i_nlink) {
> +                       iput(inode);
> +                       return ERR_PTR(-ESTALE);
> +               }
>                 return inode;
> +       }
>
>         err = __nilfs_read_inode(sb, root, ino, inode);
>         if (unlikely(err)) {
> diff --git a/fs/nilfs2/namei.c b/fs/nilfs2/namei.c
> index 9b108052d9f7..7037f47c454f 100644
> --- a/fs/nilfs2/namei.c
> +++ b/fs/nilfs2/namei.c
> @@ -67,6 +67,12 @@ nilfs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
>                 inode = NULL;
>         } else {
>                 inode = nilfs_iget(dir->i_sb, NILFS_I(dir)->i_root, ino);
> +               if (inode == ERR_PTR(-ESTALE)) {

> +                       nilfs_error(dir->i_sb, __func__,
> +                                       "deleted inode referenced: %lu",
> +                                       (unsigned long) ino);

Unlink ext2_error(), nilfs_error() does not require __func__, to be
passed as an argument.
nilfs_error() is a wrapper macro for the actual error output function
__nilfs_error(), which hides __func__ there.
(I should have mentioned the difference, sorry.)

Another comment:  "ino" is of type "ino_t", which is of type "unsigned
long", so the typecast to "unsigned long" for the argument "ino" is
not necessary.
I don't know why the ext2 implementation does it, but even if this
patch is backported to stable trees, this typecast is not necessary.

Thanks,
Ryusuke Konishi

> +                       return ERR_PTR(-EIO);
> +               }
>         }
>
>         return d_splice_alias(inode, dentry);
> --
> 2.47.0
>