Re: [PATCH] fs/bfs: fix possible NULL pointer dereference caused by empty i_op/i_fop

[Andrew Kanner <andrew.kanner@gmail.com>]

On Mon, Sep 30, 2024 at 11:01:54AM +0200, Andrew Kanner wrote:
> Syzkaller reported and reproduced the following issue:
> 
> loop0: detected capacity change from 0 to 64
> overlayfs: fs on './file0' does not support file handles, \
>            falling back to index=off,nfs_export=off.
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> [...]
> Comm: syz-executor169 Not tainted 6.11.0-rc5-syzkaller-00176-g20371ba12063 #0
> [...]
> Call Trace:
>  <TASK>
>  __lookup_slow+0x28c/0x3f0 fs/namei.c:1718
>  lookup_slow fs/namei.c:1735 [inline]
>  lookup_one_unlocked+0x1a4/0x290 fs/namei.c:2898
>  ovl_lookup_positive_unlocked fs/overlayfs/namei.c:210 [inline]
>  ovl_lookup_single+0x200/0xbd0 fs/overlayfs/namei.c:240
>  ovl_lookup_layer+0x417/0x510 fs/overlayfs/namei.c:333
>  ovl_lookup+0xcf7/0x2a60 fs/overlayfs/namei.c:1124
>  lookup_one_qstr_excl+0x11f/0x260 fs/namei.c:1633
>  filename_create+0x297/0x540 fs/namei.c:3980
>  do_mknodat+0x18b/0x5b0 fs/namei.c:4125
>  __do_sys_mknod fs/namei.c:4171 [inline]
>  __se_sys_mknod fs/namei.c:4169 [inline]
>  __x64_sys_mknod+0x8c/0xa0 fs/namei.c:4169
>  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:0x7fc4b42b2839
> 
> However, the actual root cause is not related to overlayfs:
>   (gdb) p lower.dentry->d_inode->i_op
>   $6 = (const struct inode_operations *) 0xffffffff8242fcc0 <empty_iops>
>   (gdb) p lower.dentry->d_inode->i_op->lookup
>   $7 = (struct dentry *(*) \
>        (struct inode *, struct dentry *, unsigned int)) 0x0
> 
> The inode, which is passed to ovl_lookup(), has an empty i_op,
> so the following __lookup_slow() hit NULL doing it's job:
>   old = inode->i_op->lookup(inode, dentry, flags);
> 
> bfs_fill_super()->bfs_iget() are skipping i_op/i_fop initialization
> if vnode type is not BFS_VDIR or BFS_VREG (e.g. corrupted fs).
> Adding extra error handling fixes the issue and syzkaller repro
> doesn't trigger anything bad anymore.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+94891a5155abdf6821b7@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/0000000000003d5bc30617238b6d@google.com/T/
> Signed-off-by: Andrew Kanner <andrew.kanner@gmail.com>
> ---
>  fs/bfs/inode.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> index db81570c9637..e590b231ad20 100644
> --- a/fs/bfs/inode.c
> +++ b/fs/bfs/inode.c
> @@ -70,6 +70,10 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
>  		inode->i_op = &bfs_file_inops;
>  		inode->i_fop = &bfs_file_operations;
>  		inode->i_mapping->a_ops = &bfs_aops;
> +	} else {
> +		pr_err("Bad i_vtype for inode %s:%08lx\n", inode->i_sb->s_id, ino);
> +		brelse(bh);
> +		goto error;
>  	}
>  
>  	BFS_I(inode)->i_sblock =  le32_to_cpu(di->i_sblock);
> -- 
> 2.39.3
> 

I sent it when merge window was closing.
Any objections against it now?

It seems to be introduced long time ago, but affects
kernel builds with CONFIG_BFS_FS=y or =m only.

-- 
Andrew Kanner