Re: [PATCH] ext2: reject inodes with zero i_nlink and valid mode in ext2_iget()

[Jan Kara]

On Sat 04-04-26 18:20:11, Vasiliy Kovalev wrote:
> ext2_iget() already rejects inodes with i_nlink == 0 when i_mode is
> zero or i_dtime is set, treating them as deleted. However, the case of
> i_nlink == 0 with a non-zero mode and zero dtime slips through. Since
> ext2 has no orphan list, such a combination can only result from
> filesystem corruption - a legitimate inode deletion always sets either
> i_dtime or clears i_mode before freeing the inode.
> 
> A crafted image can exploit this gap to present such an inode to the
> VFS, which then triggers WARN_ON inside drop_nlink() (fs/inode.c) via
> ext2_unlink(), ext2_rename() and ext2_rmdir():
> 
> WARNING: CPU: 3 PID: 609 at fs/inode.c:336 drop_nlink+0xad/0xd0 fs/inode.c:336
> CPU: 3 UID: 0 PID: 609 Comm: syz-executor Not tainted 6.12.77+ #1
> Call Trace:
>  <TASK>
>  inode_dec_link_count include/linux/fs.h:2518 [inline]
>  ext2_unlink+0x26c/0x300 fs/ext2/namei.c:295
>  vfs_unlink+0x2fc/0x9b0 fs/namei.c:4477
>  do_unlinkat+0x53e/0x730 fs/namei.c:4541
>  __x64_sys_unlink+0xc6/0x110 fs/namei.c:4587
>  do_syscall_64+0xf5/0x220 arch/x86/entry/common.c:78
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>  </TASK>
> 
> WARNING: CPU: 0 PID: 646 at fs/inode.c:336 drop_nlink+0xad/0xd0 fs/inode.c:336
> CPU: 0 UID: 0 PID: 646 Comm: syz.0.17 Not tainted 6.12.77+ #1
> Call Trace:
>  <TASK>
>  inode_dec_link_count include/linux/fs.h:2518 [inline]
>  ext2_rename+0x35e/0x850 fs/ext2/namei.c:374
>  vfs_rename+0xf2f/0x2060 fs/namei.c:5021
>  do_renameat2+0xbe2/0xd50 fs/namei.c:5178
>  __x64_sys_rename+0x7e/0xa0 fs/namei.c:5223
>  do_syscall_64+0xf5/0x220 arch/x86/entry/common.c:78
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>  </TASK>
> 
> WARNING: CPU: 0 PID: 634 at fs/inode.c:336 drop_nlink+0xad/0xd0 fs/inode.c:336
> CPU: 0 UID: 0 PID: 634 Comm: syz-executor Not tainted 6.12.77+ #1
> Call Trace:
>  <TASK>
>  inode_dec_link_count include/linux/fs.h:2518 [inline]
>  ext2_rmdir+0xca/0x110 fs/ext2/namei.c:311
>  vfs_rmdir+0x204/0x690 fs/namei.c:4348
>  do_rmdir+0x372/0x3e0 fs/namei.c:4407
>  __x64_sys_unlinkat+0xf0/0x130 fs/namei.c:4577
>  do_syscall_64+0xf5/0x220 arch/x86/entry/common.c:78
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>  </TASK>
> 
> Extend the existing i_nlink == 0 check to also catch this case,
> reporting the corruption via ext2_error() and returning -EFSCORRUPTED.
> This rejects the inode at load time and prevents it from reaching any
> of the namei.c paths.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Vasiliy Kovalev <kovalev@xxxxxxxxxxxx>

Thanks. I've added the patch to my tree.

								Honza

> ---
>  fs/ext2/inode.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index dbfe9098a124..39d972722f5f 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1430,9 +1430,17 @@ struct inode *ext2_iget (struct super_block *sb, unsigned long ino)
>  	 * the test is that same one that e2fsck uses
>  	 * NeilBrown 1999oct15
>  	 */
> -	if (inode->i_nlink == 0 && (inode->i_mode == 0 || ei->i_dtime)) {
> -		/* this inode is deleted */
> -		ret = -ESTALE;
> +	if (inode->i_nlink == 0) {
> +		if (inode->i_mode == 0 || ei->i_dtime) {
> +			/* this inode is deleted */
> +			ret = -ESTALE;
> +		} else {
> +			ext2_error(sb, __func__,
> +				   "inode %lu has zero i_nlink with mode 0%o and no dtime, "
> +				   "filesystem may be corrupt",
> +				   ino, inode->i_mode);
> +			ret = -EFSCORRUPTED;
> +		}
>  		goto bad_inode;
>  	}
>  	inode->i_blocks = le32_to_cpu(raw_inode->i_blocks);
> -- 
> 2.50.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR