ntfs: validate resident attribute lists and harden the validator

A base inode's $ATTRIBUTE_LIST is sanity-checked by load_attribute_list()
only on the non-resident path; ntfs_read_locked_inode() copies a *resident*
attribute list into ni->attr_list with a plain memcpy() and no validation
at all. Every subsequent walk of ni->attr_list --
ntfs_external_attr_find(), ntfs_inode_attach_all_extents() and
ntfs_attrlist_need() -- then trusts the entries are well-formed and reads
attr_list_entry fixed-header fields
(lowest_vcn at offset 8, mft_reference at offset 16, and the name) with
bounds that assume validation already happened. A crafted resident
attribute list therefore reaches those walks unvalidated and can drive
out-of-bounds reads of the attribute-list buffer.

load_attribute_list() itself reads ale->name_offset (offset 7),
ale->mft_reference (offset 16) and the name length under only an
"al < al_start + size" bound, so its own validation loop can over-read the
fixed header of a truncated trailing entry by a few bytes.

Factor the per-entry validation into ntfs_attr_list_entry_is_valid(),
which requires each entry's fixed header (offsetof(struct
attr_list_entry, name)) to be in range before any field is dereferenced,
that ale->length is a multiple of 8 covering the fixed header plus the
name, and that the entry is in use and carries a live MFT reference.
ntfs_attr_list_is_valid() walks the buffer with it and checks the entries
tile it exactly. Use the list validator in load_attribute_list()
(replacing the open-coded loop, closing its own over-read) and on the
resident path in ntfs_read_locked_inode() (which previously skipped
validation entirely); patches 2/3 reuse the per-entry helper at the other
two attribute-list walks.

Fixes: 1e9ea7e04472 ("Revert "fs: Remove NTFS classic"")
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Reviewed-by: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 5e1ad1cd01186525dc546501b05569ba73fdcdd4..2d675bf99ca762deaf5671bfd056a08e924827fc 100644 (file)
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -921,11 +921,71 @@ char *ntfs_attr_name_get(const struct ntfs_volume *vol, const __le16 *uname,
        return NULL;
 }
 
+/*
+ * ntfs_attr_list_entry_is_valid - sanity check one $ATTRIBUTE_LIST entry
+ * @ale:       the attribute-list entry to check
+ * @al_end:    end of the attribute-list buffer @ale lives in
+ *
+ * Verify that @ale is a well-formed attr_list_entry wholly contained in
+ * [.., @al_end): its fixed header must lie in range before any field is
+ * dereferenced, its length must be a multiple of 8 that covers the fixed
+ * header plus the name, the name must lie within the buffer, the entry must
+ * be in use and carry a live MFT reference.  Return true if valid.
+ */
+bool ntfs_attr_list_entry_is_valid(const struct attr_list_entry *ale,
+                                  const u8 *al_end)
+{
+       const u8 *al = (const u8 *)ale;
+       u16 ale_len;
+
+       /* The fixed header must be in bounds before it is parsed. */
+       if (al + offsetof(struct attr_list_entry, name) > al_end)
+               return false;
+       ale_len = le16_to_cpu(ale->length);
+       /* On-disk entries are 8-byte aligned (see struct attr_list_entry). */
+       if (ale_len & 7)
+               return false;
+       if (ale->name_offset != sizeof(struct attr_list_entry))
+               return false;
+       if ((u32)ale->name_offset +
+           (u32)ale->name_length * sizeof(__le16) > ale_len ||
+           al + ale_len > al_end)
+               return false;
+       if (ale->type == AT_UNUSED)
+               return false;
+       if (MSEQNO_LE(ale->mft_reference) == 0)
+               return false;
+       return true;
+}
+
+/*
+ * ntfs_attr_list_is_valid - sanity check an in-memory $ATTRIBUTE_LIST
+ * @al_start:  start of the attribute list buffer
+ * @size:      length of the attribute list in bytes
+ *
+ * Verify that [@al_start, @al_start + @size) is a sequence of valid
+ * attr_list_entry records (see ntfs_attr_list_entry_is_valid()) that tile the
+ * buffer exactly.  Return true if valid, false otherwise.
+ */
+bool ntfs_attr_list_is_valid(const u8 *al_start, s64 size)
+{
+       const u8 *al = al_start;
+       const u8 *al_end = al_start + size;
+
+       while (al < al_end) {
+               const struct attr_list_entry *ale =
+                               (const struct attr_list_entry *)al;
+
+               if (!ntfs_attr_list_entry_is_valid(ale, al_end))
+                       return false;
+               al += le16_to_cpu(ale->length);
+       }
+       return al == al_end;
+}
+
 int load_attribute_list(struct ntfs_inode *base_ni, u8 *al_start, const s64 size)
 {
        struct inode *attr_vi = NULL;
-       u8 *al;
-       struct attr_list_entry *ale;
 
        if (!al_start || size <= 0)
                return -EINVAL;
@@ -947,19 +1007,7 @@ int load_attribute_list(struct ntfs_inode *base_ni, u8 *al_start, const s64 size
        }
        iput(attr_vi);
 
-       for (al = al_start; al < al_start + size; al += le16_to_cpu(ale->length)) {
-               ale = (struct attr_list_entry *)al;
-               if (ale->name_offset != sizeof(struct attr_list_entry))
-                       break;
-               if (le16_to_cpu(ale->length) <= ale->name_offset + ale->name_length ||
-                   al + le16_to_cpu(ale->length) > al_start + size)
-                       break;
-               if (ale->type == AT_UNUSED)
-                       break;
-               if (MSEQNO_LE(ale->mft_reference) == 0)
-                       break;
-       }
-       if (al != al_start + size) {
+       if (!ntfs_attr_list_is_valid(al_start, size)) {
                ntfs_error(base_ni->vol->sb, "Corrupt attribute list, mft = %llu",
                           base_ni->mft_no);
                return -EIO;

diff --git a/fs/ntfs/attrib.h b/fs/ntfs/attrib.h
index f7acc7986b090d58bb6aec84bf6c751bf378e6a1..e2224fbfaabe95ec4fffa092111c8adaafe4b4b1 100644 (file)
--- a/fs/ntfs/attrib.h
+++ b/fs/ntfs/attrib.h
@@ -71,6 +71,10 @@ int ntfs_attr_lookup(const __le32 type, const __le16 *name,
                const u32 name_len, const u32 ic,
                const s64 lowest_vcn, const u8 *val, const u32 val_len,
                struct ntfs_attr_search_ctx *ctx);
+bool ntfs_attr_list_entry_is_valid(const struct attr_list_entry *ale,
+                                  const u8 *al_end);
+bool ntfs_attr_list_is_valid(const u8 *al_start, s64 size);
+
 int load_attribute_list(struct ntfs_inode *base_ni,
                               u8 *al_start, const s64 size);
 

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 9717fb5b47097e925e53f7f87df49742badc4c4e..8a7798d7f5fc0ce730b4c317d15992d6c988ce9b 100644 (file)
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -848,6 +848,12 @@ static int ntfs_read_locked_inode(struct inode *vi)
                                        a->data.resident.value_offset),
                                        le32_to_cpu(
                                        a->data.resident.value_length));
+                       /* A resident list is not validated on load; check it now. */
+                       if (!ntfs_attr_list_is_valid(ni->attr_list,
+                                                    ni->attr_list_size)) {
+                               ntfs_error(vi->i_sb, "Corrupt attribute list.");
+                               goto unm_err_out;
+                       }
                }
        }
 skip_attr_list_load: