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: [email protected]
Signed-off-by: Edward Adam Davis <[email protected]>
---
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: [email protected]
> Signed-off-by: Edward Adam Davis <[email protected]>
> ---
> 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!
--
With Best Regards,
Baokun Li
.
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!
--
With Best Regards,
Baokun Li
.