| Message ID | tencent_DABB2333139E8D1BCF4B5D1B2725FABA9108@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ext4: fix WARNING in lock_two_nondirectories | expand |
On Sun, Dec 24, 2023 at 07:53:00PM +0800, Edward Adam Davis wrote: > If inode is the ext4 boot loader inode, then when it is a directory, the inode > should also be set to bad inode. ... what? Are you saying that syzbot has randomly created a filesystem where the boot loader inode is a directory? If so, I'd suggest just rejecting the filesystem with EFSCORRUPT. > Reported-and-tested-by: syzbot+2c4a3b922a860084cc7f@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > fs/ext4/inode.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 61277f7f8722..b311f610f008 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -4944,8 +4944,12 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, > inode->i_fop = &ext4_file_operations; > ext4_set_aops(inode); > } else if (S_ISDIR(inode->i_mode)) { > - inode->i_op = &ext4_dir_inode_operations; > - inode->i_fop = &ext4_dir_operations; > + if (ino == EXT4_BOOT_LOADER_INO) > + make_bad_inode(inode); > + else { > + inode->i_op = &ext4_dir_inode_operations; > + inode->i_fop = &ext4_dir_operations; > + } > } else if (S_ISLNK(inode->i_mode)) { > /* VFS does not allow setting these so must be corruption */ > if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) { > -- > 2.43.0 > >
On 2023/12/24 19:53, Edward Adam Davis wrote: > If inode is the ext4 boot loader inode, then when it is a directory, the inode > should also be set to bad inode. > > Reported-and-tested-by: syzbot+2c4a3b922a860084cc7f@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > fs/ext4/inode.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 61277f7f8722..b311f610f008 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -4944,8 +4944,12 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, > inode->i_fop = &ext4_file_operations; > ext4_set_aops(inode); > } else if (S_ISDIR(inode->i_mode)) { > - inode->i_op = &ext4_dir_inode_operations; > - inode->i_fop = &ext4_dir_operations; > + if (ino == EXT4_BOOT_LOADER_INO) > + make_bad_inode(inode); Marking the boot loader inode as a bad inode here is useless, EXT4_IGET_BAD allows us to get a bad boot loader inode. In my opinion, it doesn't make sense to call lock_two_nondirectories() here to determine if the inode is a regular file or not, since the logic for dealing with non-regular files comes after the locking, so calling lock_two_inodes() directly here will suffice. Merry Christmas! Baokun > + else { > + inode->i_op = &ext4_dir_inode_operations; > + inode->i_fop = &ext4_dir_operations; > + } > } else if (S_ISLNK(inode->i_mode)) { > /* VFS does not allow setting these so must be corruption */ > if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote: > In my opinion, it doesn't make sense to call lock_two_nondirectories() > here to determine if the inode is a regular file or not, since the logic > for dealing with non-regular files comes after the locking, so calling > lock_two_inodes() directly here will suffice. No. First of all, lock_two_inodes() is a mistake that is going to be removed in the coming cycle. What's more, why the hell do you need to lock *anything* to check the inode type? Inode type never changes, period. Just take that check prior to lock_two_nondirectories() and be done with that.
On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote: > Marking the boot loader inode as a bad inode here is useless, > EXT4_IGET_BAD allows us to get a bad boot loader inode. > In my opinion, it doesn't make sense to call lock_two_nondirectories() > here to determine if the inode is a regular file or not, since the logic > for dealing with non-regular files comes after the locking, so calling > lock_two_inodes() directly here will suffice. This is all very silly, and why I consider this sort of thing pure syzkaller noise. It really doesn't protect against any real threat, and it encourages people to put all sorts of random crud in kernel code, all in the name of trying to shut up syzbot. If we *are* going to care about shutting up syzkaller, the right approach is to simply add a check in swap_inode_boot_loader() which causes it to call ext4_error() and declare the file system corrupted if the bootloader inode is not a regular file, and then return -EFSCORRUPTED. We don't need to add random hacks to ext4_iget(), or in other places... - Ted
On Sun, Dec 24, 2023 at 09:11:36PM -0500, Theodore Ts'o wrote: > On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote: > > Marking the boot loader inode as a bad inode here is useless, > > EXT4_IGET_BAD allows us to get a bad boot loader inode. > > In my opinion, it doesn't make sense to call lock_two_nondirectories() > > here to determine if the inode is a regular file or not, since the logic > > for dealing with non-regular files comes after the locking, so calling > > lock_two_inodes() directly here will suffice. > > This is all very silly, and why I consider this sort of thing pure > syzkaller noise. It really doesn't protect against any real threat, > and it encourages people to put all sorts of random crud in kernel > code, all in the name of trying to shut up syzbot. > > If we *are* going to care about shutting up syzkaller, the right > approach is to simply add a check in swap_inode_boot_loader() which > causes it to call ext4_error() and declare the file system corrupted > if the bootloader inode is not a regular file, and then return > -EFSCORRUPTED. > > We don't need to add random hacks to ext4_iget(), or in other places... Just check the inode type before anything else and be done with that - if an in-core inode of a regular file manages to become a directory right under us, we have a much worse problem. IOW, the bug is real, but suggested patch is just plain wrong.
On 2023/12/25 10:07, Al Viro wrote: > On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote: > >> In my opinion, it doesn't make sense to call lock_two_nondirectories() >> here to determine if the inode is a regular file or not, since the logic >> for dealing with non-regular files comes after the locking, so calling >> lock_two_inodes() directly here will suffice. > No. First of all, lock_two_inodes() is a mistake that is going to be > removed in the coming cycle. Okay, I didn't know about this. > What's more, why the hell do you need to lock *anything* to check the > inode type? Inode type never changes, period. > > Just take that check prior to lock_two_nondirectories() and be done with > that. Since in the current logic we update the boot loader file via swap_inode_boot_loader(), however the boot loader inode on disk may be uninitialized and may be garbage data, so we allow to get a bad boot loader inode and then initialize it and swap it with the boot loader file to be set. When reinitializing the bad boot loader inode, something like an inode type conversion may occur. Cheers, Baokun
On 2023/12/25 10:11, Theodore Ts'o wrote: > On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote: >> Marking the boot loader inode as a bad inode here is useless, >> EXT4_IGET_BAD allows us to get a bad boot loader inode. >> In my opinion, it doesn't make sense to call lock_two_nondirectories() >> here to determine if the inode is a regular file or not, since the logic >> for dealing with non-regular files comes after the locking, so calling >> lock_two_inodes() directly here will suffice. > This is all very silly, and why I consider this sort of thing pure > syzkaller noise. It really doesn't protect against any real threat, > and it encourages people to put all sorts of random crud in kernel > code, all in the name of trying to shut up syzbot. Indeed, the warning is meaningless, but it is undeniable that if the user can easily trigger the warning, something is wrong with the code. > If we *are* going to care about shutting up syzkaller, the right > approach is to simply add a check in swap_inode_boot_loader() which > causes it to call ext4_error() and declare the file system corrupted > if the bootloader inode is not a regular file, and then return > -EFSCORRUPTED. > > We don't need to add random hacks to ext4_iget(), or in other places... > > - Ted Without considering the case where the boot loader inode is uninitialized, I think this is fine and the logic to determine if the boot loader inode is initialized and to initialize it can be removed. Merry Christmas!
On Mon, Dec 25, 2023 at 10:33:20AM +0800, Baokun Li wrote: > Since in the current logic we update the boot loader file via > swap_inode_boot_loader(), however the boot loader inode on disk > may be uninitialized and may be garbage data, so we allow to get a > bad boot loader inode and then initialize it and swap it with the boot > loader file to be set. > When reinitializing the bad boot loader inode, something like an > inode type conversion may occur. Yes, but the boot laoder inode is *either* all zeros, or a regular file. If it's a directory, then it's a malicious syzbot trying to mess with our minds. Aside from the warning, it's pretty harmless, but it will very likely result in a corrupted file system --- but the file system was corrupted in the first place. So who cares? Just check to make sure that i_mode is either 0, or regular file, and return EFSCORRUPTEd, and we're done. - Ted
On 2023/12/25 10:49, Theodore Ts'o wrote: > On Mon, Dec 25, 2023 at 10:33:20AM +0800, Baokun Li wrote: >> Since in the current logic we update the boot loader file via >> swap_inode_boot_loader(), however the boot loader inode on disk >> may be uninitialized and may be garbage data, so we allow to get a >> bad boot loader inode and then initialize it and swap it with the boot >> loader file to be set. >> When reinitializing the bad boot loader inode, something like an >> inode type conversion may occur. > Yes, but the boot laoder inode is *either* all zeros, or a regular > file. If it's a directory, then it's a malicious syzbot trying to > mess with our minds. > > Aside from the warning, it's pretty harmless, but it will very likely > result in a corrupted file system --- but the file system was > corrupted in the first place. So who cares? > > Just check to make sure that i_mode is either 0, or regular file, and > return EFSCORRUPTEd, and we're done. > > - Ted Yes, this seems to work, but for that matter, when i_mode is 0, we still trigger the WARN_ON_ONCE in lock_two_nondirectories(). Merry Christmas!
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 61277f7f8722..b311f610f008 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -4944,8 +4944,12 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, inode->i_fop = &ext4_file_operations; ext4_set_aops(inode); } else if (S_ISDIR(inode->i_mode)) { - inode->i_op = &ext4_dir_inode_operations; - inode->i_fop = &ext4_dir_operations; + if (ino == EXT4_BOOT_LOADER_INO) + make_bad_inode(inode); + else { + inode->i_op = &ext4_dir_inode_operations; + inode->i_fop = &ext4_dir_operations; + } } else if (S_ISLNK(inode->i_mode)) { /* VFS does not allow setting these so must be corruption */ if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
If inode is the ext4 boot loader inode, then when it is a directory, the inode should also be set to bad inode. Reported-and-tested-by: syzbot+2c4a3b922a860084cc7f@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- fs/ext4/inode.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)