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] [thread-next>] [day] [month] [year] [list]
Message-ID: <5f0769cd-2cbb-4349-8be4-dfdc74c2c5f8@I-love.SAKURA.ne.jp>
Date: Fri, 1 Aug 2025 06:12:31 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>,
        "leocstone@...il.com" <leocstone@...il.com>,
        "jack@...e.cz" <jack@...e.cz>,
        "willy@...radead.org" <willy@...radead.org>,
        "brauner@...nel.org" <brauner@...nel.org>
Cc: "glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>,
        "frank.li@...o.com" <frank.li@...o.com>,
        "slava@...eyko.com" <slava@...eyko.com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH v4] hfs: update sanity check of the root record

On 2025/08/01 3:03, Viacheslav Dubeyko wrote:
> On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote:
>> On 2025/07/31 4:24, Viacheslav Dubeyko wrote:
>>> If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
>>> could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
>>> HFS_POR_CNID could be a problem in hfs_write_inode()?
>>
>> Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG()
>> in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode()
>> shall have to also reject 1, 5 and 15 (and as a result only accept 2).
> 
> The fix should be in hfs_read_inode(). Currently, suggested solution hides the
> issue but not fix the problem.

Not fixing this problem might be hiding other issues, by hitting BUG() before
other issues shows up.

> Because b-tree nodes could contain multiple
> corrupted records. Now, this patch checks only record for root folder. Let's
> imagine that root folder record will be OK but another record(s) will be
> corrupted in such way.

Can the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) be something other than HFS_ROOT_CNID ?

If the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) must be HFS_ROOT_CNID, this patch itself will be
a complete fix for this problem.

> Finally, we will have successful mount but operation with
> corrupted record(s) will trigger this issue. So, I cannot consider this patch as
> a complete fix of the problem.

Did you try what you think as a fix of this problem (I guess something like
shown below will be needed for avoid hitting BUG()) using
https://lkml.kernel.org/r/a8f8da77-f099-499b-98e0-39ed159b6a2d@I-love.SAKURA.ne.jp ?

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index a81ce7a740b9..d60395111ed5 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
 		tree = HFS_SB(sb)->cat_tree;
 		break;
 	default:
-		BUG();
+		pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
+		       inode->i_ino);
 		return false;
 	}
 
@@ -305,11 +306,31 @@ static int hfs_test_inode(struct inode *inode, void *data)
 	case HFS_CDR_FIL:
 		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
 	default:
-		BUG();
+		pr_err("detected unknown type %u, running fsck.hfs is recommended.\n", rec->type);
 		return 1;
 	}
 }
 
+static bool is_bad_id(unsigned long ino)
+{
+	switch (ino) {
+	case 0:
+	case 3:
+	case 4:
+	case 6:
+	case 7:
+	case 8:
+	case 9:
+	case 10:
+	case 11:
+	case 12:
+	case 13:
+	case 14:
+		return true;
+	}
+	return false;
+}
+
 /*
  * hfs_read_inode
  */
@@ -348,6 +369,10 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		}
 
 		inode->i_ino = be32_to_cpu(rec->file.FlNum);
+		if (is_bad_id(inode->i_ino)) {
+			make_bad_inode(inode);
+			break;
+		}
 		inode->i_mode = S_IRUGO | S_IXUGO;
 		if (!(rec->file.Flags & HFS_FIL_LOCK))
 			inode->i_mode |= S_IWUGO;
@@ -358,9 +383,15 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		inode->i_op = &hfs_file_inode_operations;
 		inode->i_fop = &hfs_file_operations;
 		inode->i_mapping->a_ops = &hfs_aops;
+		if (inode->i_ino < 16)
+			pr_info("HFS_CDR_FIL i_ino=%ld\n", inode->i_ino);
 		break;
 	case HFS_CDR_DIR:
 		inode->i_ino = be32_to_cpu(rec->dir.DirID);
+		if (is_bad_id(inode->i_ino)) {
+			make_bad_inode(inode);
+			break;
+		}
 		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
 		HFS_I(inode)->fs_blocks = 0;
 		inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask);
@@ -368,6 +399,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
 				      inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
 		inode->i_op = &hfs_dir_inode_operations;
 		inode->i_fop = &hfs_dir_operations;
+		if (inode->i_ino < 16)
+			pr_info("HFS_CDR_DIR i_ino=%ld\n", inode->i_ino);
 		break;
 	default:
 		make_bad_inode(inode);
@@ -441,7 +474,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
 			return 0;
 		default:
-			BUG();
+			pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
+			       inode->i_ino);
 			return -EIO;
 		}
 	}


# for i in $(seq 0 15); do timeout 1 unshare -m ./hfs $i; done
# dmesg | grep fsck
[   52.563547] [    T479] hfs: detected unknown inode 1, running fsck.hfs is recommended.
[   56.606238] [    T255] hfs: detected unknown inode 5, running fsck.hfs is recommended.
[   66.694795] [    T500] hfs: detected unknown inode 15, running fsck.hfs is recommended.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ