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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ddd2cd94-683f-462b-a475-cc04462e9bdd@I-love.SAKURA.ne.jp>
Date: Sun, 12 Oct 2025 16:34:56 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Tigran Aivazian <aivazian.tigran@...il.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] bfs: Verify inode mode when loading from disk

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