Re: [PATCH] ntfs: validate error codes in check_windows_hibernation_status()

[Namjae Jeon]

On Fri, Jul 3, 2026 at 5:25 PM Hongling Zeng <zhongling0719@xxxxxxx> wrote:
>
> Looking at ntfs_lookup_inode_by_name() more carefully:
>
> All error return paths inside the function use hardcoded kernel errnos
> (MREF_ERR(-ENOENT), MREF_ERR(-EIO), MREF_ERR(-ENOMEM)) - these are
> already valid by construction.
>
> The actual risk occurs when the function returns a "successful" MFT
> reference from disk (ie->data.dir.indexed_file) that happens to have
> bit 47 set - making IS_ERR_MREF() true at the caller. In this case,
> MREF_ERR() extracts garbage from untrusted disk data.
>
> This cannot be fixed inside ntfs_lookup_inode_by_name() without
> changing its return value semantics, because from the function's
> perspective it found a matching index entry and returned it. Only
> the caller, after IS_ERR_MREF() triggers, is in a position to
> validate that the extracted error code is a legitimate errno.
>
> Restructuring the function to distinguish "real errors I generated"
> from "disk data that looks like an error" would require a more
> invasive API change (e.g., returning int + out-parameter), which
> seems inappropriate for a legacy filesystem in maintenance mode.

I think it would be good to fix it like this. Let me know your opinion.

diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
index 4b6bd5f30c65..6fa9ae3377cb 100644
--- a/fs/ntfs/dir.c
+++ b/fs/ntfs/dir.c
@@ -23,6 +23,13 @@
 __le16 I30[5] = { cpu_to_le16('$'), cpu_to_le16('I'),
                cpu_to_le16('3'),       cpu_to_le16('0'), 0 };

+static inline u64 ntfs_check_mref(u64 mref)
+{
+       if (IS_ERR_MREF(mref))
+               return ERR_MREF(-EIO);
+       return mref;
+}
+
 /*
  * ntfs_lookup_inode_by_name - find an inode in a directory given its name
  * @dir_ni:    ntfs inode of the directory in which to search for the name
@@ -178,7 +185,7 @@ u64 ntfs_lookup_inode_by_name(struct ntfs_inode
*dir_ni, const __le16 *uname,
                        mref = le64_to_cpu(ie->data.dir.indexed_file);
                        ntfs_attr_put_search_ctx(ctx);
                        unmap_mft_record(dir_ni);
-                       return mref;
+                       return ntfs_check_mref(mref);
                }
                /*
                 * For a case insensitive mount, we also perform a case
@@ -273,7 +280,7 @@ u64 ntfs_lookup_inode_by_name(struct ntfs_inode
*dir_ni, const __le16 *uname,
                if (name) {
                        ntfs_attr_put_search_ctx(ctx);
                        unmap_mft_record(dir_ni);
-                       return name->mref;
+                       return ntfs_check_mref(name->mref);
                }
                ntfs_debug("Entry not found.");
                err = -ENOENT;
@@ -413,7 +420,7 @@ u64 ntfs_lookup_inode_by_name(struct ntfs_inode
*dir_ni, const __le16 *uname,
                        mref = le64_to_cpu(ie->data.dir.indexed_file);
                        kfree(kaddr);
                        iput(ia_vi);
-                       return mref;
+                       return ntfs_check_mref(mref);
                }
                /*
                 * For a case insensitive mount, we also perform a case
@@ -538,7 +545,7 @@ u64 ntfs_lookup_inode_by_name(struct ntfs_inode
*dir_ni, const __le16 *uname,
        if (name) {
                kfree(kaddr);
                iput(ia_vi);
-               return name->mref;
+               return ntfs_check_mref(name->mref);
        }
        ntfs_debug("Entry not found.");
        err = -ENOENT;