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] [day] [month] [year] [list]

Message-ID: <CAK+_RLmwT5EHC6aajJxG0_ccPe7YhnWkd_wOPhhCz3mGo8Ub_g@mail.gmail.com>
Date: Sun, 12 Oct 2025 18:02:30 +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

On Sun, 12 Oct 2025 at 08:35, Tetsuo Handa
<penguin-kernel@...ove.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@...il.com>