[PATCH] bfs: Verify inode mode when loading from disk

[PATCH] bfs: Verify inode mode when loading from disk

[Tetsuo Handa]

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.3

Re: [PATCH] bfs: Verify inode mode when loading from disk

[Tigran Aivazian]

On 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

Re: [PATCH] bfs: Verify inode mode when loading from disk

[Tigran Aivazian]

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);

Re: [PATCH] bfs: Verify inode mode when loading from disk

[Tetsuo Handa]

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?

Re: [PATCH] bfs: Verify inode mode when loading from disk

[Tigran Aivazian]

Thank you for your detailed explanation.

On Sat, 11 Oct 2025 at 00:19, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:

> 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?

There is also the fourth way, see the patch below. It makes it as
explicit as possible that vtype value is the authoritative one and
sets the type bits from vtype by keeping (in i_mode) only the
permission/suid/sgid/sticky bits upfront. What do you think?

diff -ur 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-11 15:33:20.431967395 +0100
@@ -38,6 +38,8 @@
        struct inode *inode;
        struct buffer_head *bh;
        int block, off;
+       u32 dmode, dvtype;
+       umode_t expect_type = 0;

        inode = iget_locked(sb, ino);
        if (!inode)
@@ -61,23 +63,43 @@
        off = (ino - BFS_ROOT_INO) % BFS_INODES_PER_BLOCK;
        di = (struct bfs_inode *)bh->b_data + off;

-       inode->i_mode = 0x0000FFFF & le32_to_cpu(di->i_mode);
-       if (le32_to_cpu(di->i_vtype) == BFS_VDIR) {
+       dmode = le32_to_cpu(di->i_mode);
+       dvtype = le32_to_cpu(di->i_vtype);
+
+       /* Keep only permission/suid/sgid/sticky bits from on-disk mode. */
+       inode->i_mode = dmode & (S_IRWXU | S_IRWXG | S_IRWXO | S_ISUID
| S_ISGID | S_ISVTX);
+
+       if (dvtype == BFS_VDIR) {
+               expect_type = S_IFDIR;
                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) {
+       } else if (dvtype == BFS_VREG) {
+               expect_type = S_IFREG;
                inode->i_mode |= S_IFREG;
                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",
+                       dvtype, dmode, 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);
+       /* If the on-disk mode carried type bits, they must match vtype. */
+       if ((dmode & S_IFMT) && ((dmode & S_IFMT) != expect_type)) {
+               brelse(bh);
+               printf("Inconsistent type vtype=%u mode=0%07o
fmt=0%07o %s:%08lx\n",
+                       dvtype, dmode, (dmode & S_IFMT),
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);
        BFS_I(inode)->i_dsk_ino = le16_to_cpu(di->i_ino);
        i_uid_write(inode, le32_to_cpu(di->i_uid));
-       i_gid_write(inode,  le32_to_cpu(di->i_gid));
+       i_gid_write(inode, le32_to_cpu(di->i_gid));
        set_nlink(inode, le32_to_cpu(di->i_nlink));
        inode->i_size = BFS_FILESIZE(di);
        inode->i_blocks = BFS_FILEBLOCKS(di);

Re: [PATCH] bfs: Verify inode mode when loading from disk

[Tetsuo Handa]

On 2025/10/12 1:29, Tigran Aivazian wrote:
>> 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?
> 
> There is also the fourth way, see the patch below. It makes it as
> explicit as possible that vtype value is the authoritative one and
> sets the type bits from vtype by keeping (in i_mode) only the
> permission/suid/sgid/sticky bits upfront. What do you think?

Well, I feel that we should choose "replace the 0x0000FFFF mask with
0x00000FFF" approach, for situation might be worse than HFS+ case.

HFS+ explicitly explains that all bits are 0 when that field is not initialized.

  If the S_IFMT field (upper 4 bits) of the fileMode field is zero, then
  Mac OS X assumes that the permissions structure is uninitialized, and
  internally uses default values for all of the fields. 

But "BFS inodes" in https://martin.hinner.info/fs/bfs/bfs-structure.html did not
say that SCO UnixWare sets 0 to the unused 23 bits when writing to disk.

  32bit int 	mode 	File mode, rwxrwxrwx (only low 9 bits used) 

This means that the unused 23 bits might be random, and therefore we can't
trust S_IFMT bits. Please see the patch shown below.

 fs/bfs/inode.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 1d41ce477df5..984b365df046 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -61,7 +61,19 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
 	off = (ino - BFS_ROOT_INO) % BFS_INODES_PER_BLOCK;
 	di = (struct bfs_inode *)bh->b_data + off;
 
-	inode->i_mode = 0x0000FFFF & le32_to_cpu(di->i_mode);
+	/*
+	 * https://martin.hinner.info/fs/bfs/bfs-structure.html explains that
+	 * BFS in SCO UnixWare environment used only lower 9 bits of di->i_mode
+	 * value. This means that, although bfs_write_inode() saves whole
+	 * inode->i_mode bits (which include S_IFMT bits and S_IS{UID,GID,VTX}
+	 * bits), middle 7 bits of di->i_mode value can be garbage when these
+	 * bits were not saved by bfs_write_inode().
+	 * Since we can't tell whether middle 7 bits are garbage, use only
+	 * lower 12 bits (i.e. tolerate S_IS{UID,GID,VTX} bits possibly being
+	 * garbage) and reconstruct S_IFMT bits for Linux environment from
+	 * di->i_vtype value.
+	 */
+	inode->i_mode = 0x00000FFF & le32_to_cpu(di->i_mode);
 	if (le32_to_cpu(di->i_vtype) == BFS_VDIR) {
 		inode->i_mode |= S_IFDIR;
 		inode->i_op = &bfs_dir_inops;
@@ -71,6 +83,11 @@ 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 {
+		brelse(bh);
+		printf("Unknown vtype=%u %s:%08lx\n",
+		       le32_to_cpu(di->i_vtype), inode->i_sb->s_id, ino);
+		goto error;
 	}
 
 	BFS_I(inode)->i_sblock =  le32_to_cpu(di->i_sblock);

Re: [PATCH] bfs: Verify inode mode when loading from disk

[Tigran Aivazian]

On Sun, 12 Oct 2025 at 08:35, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Well, I feel that we should choose "replace the 0x0000FFFF mask with
> 0x00000FFF" approach, for situation might be worse than HFS+ case.
> ...
> -       inode->i_mode = 0x0000FFFF & le32_to_cpu(di->i_mode);
> +       /*
> +        * https://martin.hinner.info/fs/bfs/bfs-structure.html explains that
> +        * BFS in SCO UnixWare environment used only lower 9 bits of di->i_mode
> +        * value. This means that, although bfs_write_inode() saves whole
> +        * inode->i_mode bits (which include S_IFMT bits and S_IS{UID,GID,VTX}
> +        * bits), middle 7 bits of di->i_mode value can be garbage when these
> +        * bits were not saved by bfs_write_inode().
> +        * Since we can't tell whether middle 7 bits are garbage, use only
> +        * lower 12 bits (i.e. tolerate S_IS{UID,GID,VTX} bits possibly being
> +        * garbage) and reconstruct S_IFMT bits for Linux environment from
> +        * di->i_vtype value.
> +        */
> +       inode->i_mode = 0x00000FFF & le32_to_cpu(di->i_mode);
>         if (le32_to_cpu(di->i_vtype) == BFS_VDIR) {
>                 inode->i_mode |= S_IFDIR;
>                 inode->i_op = &bfs_dir_inops;
> @@ -71,6 +83,11 @@ 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 {
> +               brelse(bh);
> +               printf("Unknown vtype=%u %s:%08lx\n",
> +                      le32_to_cpu(di->i_vtype), inode->i_sb->s_id, ino);
> +               goto error;
>         }

Agreed -- given that historical BFS may leave those "middle 7 bits"
uninitialised, we shouldn't trust any S_IFMT coming off disk. Masking
to the lower 12 bits and reconstructing type from vtype is the right
thing to do.

Two optional tiny nits for readability:

  * use a symbolic mask for the 12 bits we keep:
        inode->i_mode = le32_to_cpu(di->i_mode) &
                (S_IRWXU | S_IRWXG | S_IRWXO | S_ISUID | S_ISGID | S_ISVTX);

  * cache the endianness conversions:
        u32 dmode = le32_to_cpu(di->i_mode);
        u32 dvtype = le32_to_cpu(di->i_vtype);
        inode->i_mode = dmode & (S_IRWXU | S_IRWXG | S_IRWXO |
                                 S_ISUID | S_ISGID | S_ISVTX);
        if (dvtype == BFS_VDIR) { ... } else if (dvtype == BFS_VREG) { ... }
        else {
                brelse(bh);
                printf("Unknown vtype=%u mode=0%07o %s:%08lx\n",
                       dvtype, dmode, inode->i_sb->s_id, ino);
                goto error;
        }

With or without those nits, your approach looks good to me.

Reviewed-by: Tigran Aivazian <aivazian.tigran@gmail.com>

[PATCH v2] bfs: Reconstruct file type when loading from disk

[Tetsuo Handa]

syzbot is reporting that S_IFMT bits of inode->i_mode can become bogus when
the S_IFMT bits of the 32bits "mode" field loaded from disk are corrupted
or when the 32bits "attributes" field loaded from disk are corrupted.

A documentation says that BFS uses only lower 9 bits of the "mode" field.
But I can't find an explicit explanation that the unused upper 23 bits
(especially, the S_IFMT bits) are initialized with 0.

Therefore, ignore the S_IFMT bits of the "mode" field loaded from disk.
Also, verify that the value of the "attributes" field loaded from disk is
either BFS_VREG or BFS_VDIR (because BFS supports only regular files and
the root directory).

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>
Reviewed-by: Tigran Aivazian <aivazian.tigran@gmail.com>
---
 fs/bfs/inode.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 1d41ce477df5..984b365df046 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -61,7 +61,19 @@ struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
 	off = (ino - BFS_ROOT_INO) % BFS_INODES_PER_BLOCK;
 	di = (struct bfs_inode *)bh->b_data + off;
 
-	inode->i_mode = 0x0000FFFF & le32_to_cpu(di->i_mode);
+	/*
+	 * https://martin.hinner.info/fs/bfs/bfs-structure.html explains that
+	 * BFS in SCO UnixWare environment used only lower 9 bits of di->i_mode
+	 * value. This means that, although bfs_write_inode() saves whole
+	 * inode->i_mode bits (which include S_IFMT bits and S_IS{UID,GID,VTX}
+	 * bits), middle 7 bits of di->i_mode value can be garbage when these
+	 * bits were not saved by bfs_write_inode().
+	 * Since we can't tell whether middle 7 bits are garbage, use only
+	 * lower 12 bits (i.e. tolerate S_IS{UID,GID,VTX} bits possibly being
+	 * garbage) and reconstruct S_IFMT bits for Linux environment from
+	 * di->i_vtype value.
+	 */
+	inode->i_mode = 0x00000FFF & le32_to_cpu(di->i_mode);
 	if (le32_to_cpu(di->i_vtype) == BFS_VDIR) {
 		inode->i_mode |= S_IFDIR;
 		inode->i_op = &bfs_dir_inops;
@@ -71,6 +83,11 @@ 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 {
+		brelse(bh);
+		printf("Unknown vtype=%u %s:%08lx\n",
+		       le32_to_cpu(di->i_vtype), inode->i_sb->s_id, ino);
+		goto error;
 	}
 
 	BFS_I(inode)->i_sblock =  le32_to_cpu(di->i_sblock);
-- 
2.47.3