fs/bfs/inode.c | 6 ++++++ 1 file changed, 6 insertions(+)
The inode mode loaded from corrupted disk can be invalid.
Since Boot File System supports only root directory and regular files [1],
reject inodes which are neither directory nor regular file.
Link: https://martin.hinner.info/fs/bfs/ [1]
Reported-by: syzbot+895c23f6917da440ed0d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=895c23f6917da440ed0d
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Since reproducer for BFS is not yet found, I can't test this patch.
But I assume this is the only location which can store bogus file mode.
fs/bfs/inode.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 1d41ce477df5..6cc552f77d51 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -72,6 +72,12 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
inode->i_fop = &bfs_file_operations;
inode->i_mapping->a_ops = &bfs_aops;
}
+ if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode)) {
+ brelse(bh);
+ printf("Bad file type (0%04o) %s:%08lx.\n",
+ inode->i_mode, inode->i_sb->s_id, ino);
+ goto error;
+ }
BFS_I(inode)->i_sblock = le32_to_cpu(di->i_sblock);
BFS_I(inode)->i_eblock = le32_to_cpu(di->i_eblock);
--
2.47.3On Fri, 10 Oct 2025 at 15:24, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> The inode mode loaded from corrupted disk can be invalid.
> Since Boot File System supports only root directory and regular files [1],
> reject inodes which are neither directory nor regular file.
>
> ...
> + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode)) {
> + brelse(bh);
> + printf("Bad file type (0%04o) %s:%08lx.\n",
> + inode->i_mode, inode->i_sb->s_id, ino);
> + goto error;
> + }
Thank you, but logically this code should simply be inside the "else"
clause of the previous checks, which already check for BFS_VDIR and
BFS_VREG, I think.
--
Tigran
On Fri, 10 Oct 2025 at 16:44, Tigran Aivazian <aivazian.tigran@gmail.com> wrote:
> Thank you, but logically this code should simply be inside the "else"
> clause of the previous checks, which already check for BFS_VDIR and
> BFS_VREG, I think.
Actually, I would first of all print the value of vtype, because that
is where the
corruption comes from, and print i_mode as %07o, not %04o. So, I would
suggest a patch like this.
Reviewed-by: Tigran Aivazian <aivazian.tigran@gmail.com>
diff -urN a/fs/bfs/inode.c b/fs/bfs/inode.c
--- a/fs/bfs/inode.c 2025-10-10 16:52:40.968468556 +0100
+++ b/fs/bfs/inode.c 2025-10-10 16:54:09.904052351 +0100
@@ -71,6 +71,11 @@
inode->i_op = &bfs_file_inops;
inode->i_fop = &bfs_file_operations;
inode->i_mapping->a_ops = &bfs_aops;
+ } else {
+ brelse(bh);
+ printf("Bad file type vtype=%u mode=0%07o %s:%08lx\n",
+ le32_to_cpu(di->i_vtype), inode->i_mode,
inode->i_sb->s_id, ino);
+ goto error;
}
BFS_I(inode)->i_sblock = le32_to_cpu(di->i_sblock);
Thank you for responding quickly.
On 2025/10/11 1:06, Tigran Aivazian wrote:
> On Fri, 10 Oct 2025 at 16:44, Tigran Aivazian <aivazian.tigran@gmail.com> wrote:
>> Thank you, but logically this code should simply be inside the "else"
>> clause of the previous checks, which already check for BFS_VDIR and
>> BFS_VREG, I think.
>
> Actually, I would first of all print the value of vtype, because that
> is where the
> corruption comes from, and print i_mode as %07o, not %04o. So, I would
> suggest a patch like this.
Changing what to print is fine. But
> Thank you, but logically this code should simply be inside the "else"
> clause of the previous checks, which already check for BFS_VDIR and
> BFS_VREG, I think.
the reason I didn't choose the "else" clause is that since inode->i_mode is
initially masked by 0x0000FFFF (which can make inode->i_mode & S_FMT != 0),
inode->i_mode = 0x0000FFFF & le32_to_cpu(di->i_mode);
setting the S_FMT bits like inode->i_mode |= S_IFDIR or
inode->i_mode |= S_IFREG can make bogus S_FMT values.
if (le32_to_cpu(di->i_vtype) == BFS_VDIR) {
inode->i_mode |= S_IFDIR;
inode->i_op = &bfs_dir_inops;
inode->i_fop = &bfs_dir_operations;
} else if (le32_to_cpu(di->i_vtype) == BFS_VREG) {
inode->i_mode |= S_IFREG;
inode->i_op = &bfs_file_inops;
inode->i_fop = &bfs_file_operations;
inode->i_mapping->a_ops = &bfs_aops;
}
If we do
- inode->i_mode = 0x0000FFFF & le32_to_cpu(di->i_mode);
+ inode->i_mode = 0x00000FFF & le32_to_cpu(di->i_mode);
then we can choose the "else" clause but we can't catch invalid
file types when (0x0000F000 & le32_to_cpu(di->i_mode)) != 0.
If we do
- if (le32_to_cpu(di->i_vtype) == BFS_VDIR) {
- inode->i_mode |= S_IFDIR;
+ if (le32_to_cpu(di->i_vtype) == BFS_VDIR && S_ISDIR(inode->i_mode)) {
- } else if (le32_to_cpu(di->i_vtype) == BFS_VREG) {
- inode->i_mode |= S_IFREG;
+ } else if (le32_to_cpu(di->i_vtype) == BFS_VREG && S_ISREG(inode->i_mode)) {
then we can choose the "else" clause but we can't accept
le32_to_cpu(di->i_vtype) == BFS_VDIR && (inode->i_mode & S_FMT) == 0 case and
le32_to_cpu(di->i_vtype) == BFS_VREG && (inode->i_mode & S_FMT) == 0 case.
I don't know whether (inode->i_mode & S_FMT) == 0 was historically valid in BFS
like HFS+ did ( https://lkml.kernel.org/r/d089dcbd-0db2-48a1-86b0-0df3589de9cc@I-love.SAKURA.ne.jp ).
If (inode->i_mode & S_FMT) == 0 was always invalid in BFS, we can choose the
- if (le32_to_cpu(di->i_vtype) == BFS_VDIR) {
- inode->i_mode |= S_IFDIR;
+ if (le32_to_cpu(di->i_vtype) == BFS_VDIR && S_ISDIR(inode->i_mode)) {
- } else if (le32_to_cpu(di->i_vtype) == BFS_VREG) {
- inode->i_mode |= S_IFREG;
+ } else if (le32_to_cpu(di->i_vtype) == BFS_VREG && S_ISREG(inode->i_mode)) {
+ } else {
+ brelse(bh);
+ printf("Bad file type vtype=%u mode=0%07o %s:%08lx\n",
+ le32_to_cpu(di->i_vtype), inode->i_mode, inode->i_sb->s_id, ino);
+ goto error;
approach.
Which pattern (just adding a new "if", or adding "else" with "if" and "else if" updated,
or replace the 0x0000FFFF mask with 0x00000FFF) do you prefer?
© 2016 - 2025 Red Hat, Inc.