On Sat, Oct 1, 2022 at 9:41 PM Tetsuo Handa wrote: > > On 2022/10/01 20:24, Tetsuo Handa wrote: > > syzbot is reporting lockdep warning followed by NULL pointer dereference > > at nilfs_bmap_lookup_at_level() [1], for a crafted filesystem which > > contains raw_inode->i_mode == 0 is poisoning checkpoint inode at > > nilfs_read_inode_common() from nilfs_ifile_read() from > > nilfs_attach_checkpoint() from nilfs_fill_super() from nilfs_mount(). > > Check that filetype/uid/gid are valid as well as i_nlink is valid. > > > > Link: https://syzkaller.appspot.com/bug?extid=2b32eb36c1a825b7a74c [1] > > Reported-by: syzot <syzbot+2b32eb36c1a825b7a74c@xxxxxxxxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> > > --- > > This patch solves crash but I don't know whether this patch is correct. > > Hmm, since bmap->b_sem and bmap->b_ops are initialized by nilfs_bmap_read(), > and nilfs_bmap_read() is called from nilfs_read_inode_common() only if > S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode) > is true, I guess that the caller needs to verify that the returned inode is > a regular file? But where to add that check? Exactly, this looks like the root cause of the issue. For metadata files, which are internal persistent files of nilfs2 and used to store metadata on disk, must have a file type and must call nilfs_bmap_read(), but that sanity check is missing. As you pointed out, we need to add a sanity check for i_mode of metadata files. The simple way is to insert a conditional sanity check in nilfs_read_inode_common() using nilfs_is_metadata_file_inode() and S_ISREG(), for instance: if (nilfs_is_metadata_file_inode(inode) && !S_ISREG(inode->i_mode)) return -EIO; /* On-disk inode of the metadata file is corrupted. */ if (inode->i_nlink == 0) return -ESTALE; /* ... */ Or, we can add the i_mode sanity checks separately in nilfs_dat_read(), nilfs_ifile_read(), nilfs_sufile_read(), and nilfs_cpfile_read() before they call nilfs_read_inode_common(). I think the former is fine because we don't have the idea to add metadata file inodes other than file type. If you make a revised patch, I will approve or handle it. Or if you are busy, I will take over the fix. Thank you, Ryusuke Konishi
