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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAK+_RLmbaxE9Q-ORiOUV8emrB+M6e7YgUNZEb48VwD28EuqwhQ@mail.gmail.com>
Date: Sat, 11 Oct 2025 17:29:04 +0100
From: Tigran Aivazian <aivazian.tigran@...il.com>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
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

Thank you for your detailed explanation.

On Sat, 11 Oct 2025 at 00:19, Tetsuo Handa
<penguin-kernel@...ove.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);