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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx ).
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?