[PATCH v2 1/3] ntfs: validate resident attribute lists and harden the validator

[PATCH v2 1/3] ntfs: validate resident attribute lists and harden the validator

[Bryam Vargas]

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(), 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_is_valid(), which
requires each entry's fixed header (offsetof(struct attr_list_entry, name))
to be in range before any field is dereferenced and that the entries tile
the buffer exactly. Use it both 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).

Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2: dropped the redundant Reported-by (it duplicated Signed-off-by); reproducer
    omitted on the public list -- available to the maintainers on request.
    Posting publicly per security@kernel.org guidance that crafted-image
    filesystem bugs fall outside the kernel security process's threat model.

Root cause behind the two out-of-bounds reads in patches 2/3 and 3/3: those
harden the individual read sites; this validates the list centrally so the
other walks (ntfs_inode_attach_all_extents, ntfs_attrlist_need) are covered
too. Verified under KASAN: the crafted attribute list that previously oopsed
ntfs_external_attr_find() is now rejected at load with
"ntfs_read_locked_inode(): Corrupt attribute list." and no out-of-bounds
access; a benign mkntfs image still mounts. checkpatch clean.

 fs/ntfs/attrib.c |   56 ++++++++++++++++++++++++++++++++++++++++--------------
 fs/ntfs/attrib.h |    2 ++
 fs/ntfs/inode.c  |    6 ++++++
 3 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 421c6cdcbb53..36c1df25a622 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -843,11 +843,49 @@ char *ntfs_attr_name_get(const struct ntfs_volume *vol, const __le16 *uname,
 	return NULL;
 }
 
+/*
+ * 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 well-formed sequence of
+ * attr_list_entry records.  Each record's fixed header must lie within the
+ * buffer before any of its fields are dereferenced, its length must cover
+ * the fixed header plus the name, and the records must 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;
+		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);
+		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;
+		al += ale_len;
+	}
+	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;
@@ -869,19 +907,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 f7acc7986b09..4b11d4b0be14 100644
--- a/fs/ntfs/attrib.h
+++ b/fs/ntfs/attrib.h
@@ -71,6 +71,8 @@ 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_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 360bebd1ee3f..a5f7400fd19d 100644
--- 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:
-- 
2.43.0

[PATCH v2 2/3] ntfs: bound the look-ahead attribute-list entry in ntfs_external_attr_find()

[Bryam Vargas]

When resolving an attribute lookup with a non-zero @lowest_vcn,
ntfs_external_attr_find() peeks at the next $ATTRIBUTE_LIST entry to
decide whether to keep searching, but bounds that not-yet-validated
entry only with "(u8 *)next_al_entry + 6 < al_end" (which proves just
bytes 0..6 are in range) and "(u8 *)next_al_entry + length <= al_end"
with an attacker-controlled, non-8-aligned length. It then reads
next_al_entry->lowest_vcn (an __le64 at offset 8) and the name at
next_al_entry->name_offset, both of which can lie past al_end -- the
exact end of the kvmalloc'd attribute-list buffer (allocated at the
on-disk attr_list_size, no rounding). A crafted on-disk $ATTRIBUTE_LIST
whose last entry sits a few bytes before al_end therefore yields a slab
out-of-bounds read when the inode is read.

Apply to the look-ahead entry the same bounds the main loop already
applies to the current entry: require the fixed header up to
offsetof(struct attr_list_entry, name) to be in range, and the name to
lie within the buffer, before dereferencing lowest_vcn and the name.

Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2: dropped the redundant Reported-by; reproducer and KASAN splat omitted on
    the public list -- available to the maintainers on request. Posting per
    security@kernel.org guidance that crafted-image filesystem bugs are out of
    the kernel security process's threat model.

Verified by mounting a crafted NTFS image under KASAN (fs/ntfs built as a
module against v7.1-rc5): a slab out-of-bounds read of next_al_entry->lowest_vcn
during inode read; a benign mkntfs image is KASAN-clean on the same kernel
(control). Arch-independent: the on-disk struct is __packed (lowest_vcn at
offset 8, fixed header up to offsetof(struct attr_list_entry, name) == 26) and
the over-read is an le64; identical geometry built -m32/-m64, so 32- and 64-bit
kernels are both affected.

Fixes note: this is in the fs/ntfs driver added during the v7.1 merge window.
The look-ahead expression is carried verbatim from the legacy fs/ntfs driver
(removed in v6.9; present unchanged since v2.6.12), where __ntfs_malloc()
rounded the list allocation up to a full kmalloc(PAGE_SIZE) and masked it; the
new driver's exact-size kvmalloc(attr_list_size) exposes it. Please add the
appropriate Fixes: tag for the commit that introduced ntfs_external_attr_find()
in the new driver.

 fs/ntfs/attrib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 421c6cdcbb53..aed68b8e69ea 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -1137,9 +1137,14 @@ static int ntfs_external_attr_find(const __le32 type,
 		 * we have reached the right one or the search has failed.
 		 */
 		if (lowest_vcn && (u8 *)next_al_entry >= al_start &&
-				(u8 *)next_al_entry + 6 < al_end &&
+				(u8 *)next_al_entry +
+						offsetof(struct attr_list_entry, name) <= al_end &&
 				(u8 *)next_al_entry + le16_to_cpu(
 					next_al_entry->length) <= al_end &&
+				(!next_al_entry->name_length ||
+				 (u8 *)next_al_entry + next_al_entry->name_offset +
+				 next_al_entry->name_length * sizeof(__le16) <=
+						al_end) &&
 				le64_to_cpu(next_al_entry->lowest_vcn) <=
 					lowest_vcn &&
 				next_al_entry->type == al_entry->type &&
-- 
2.43.0

[PATCH v2 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount()

[Bryam Vargas]

The $MFT attribute-list walk in ntfs_read_inode_mount() validates each
entry only with "(u8 *)al_entry + 6 > al_end" and
"(u8 *)al_entry + le16_to_cpu(al_entry->length) > al_end", but then reads
al_entry->lowest_vcn (an __le64 at offset 8) and al_entry->mft_reference
(offset 16) -- fields beyond the 6 bytes proven in range. al_entry->length
is attacker-controlled and only required non-zero, so a short entry (e.g.
length 8) placed at the tail passes both checks while the lowest_vcn /
mft_reference reads fall past al_end.

al_end is ni->attr_list + attr_list_size (the on-disk size); the buffer is
kvzalloc(round_up(attr_list_size, SECTOR_SIZE)), so the sector rounding
usually absorbs the over-read -- but when attr_list_size is a multiple of
SECTOR_SIZE there is no slack and a crafted $MFT attribute list produces an
out-of-bounds read at mount time.

Require the fixed attribute-list-entry header to be in range before
dereferencing it, matching the bound ntfs_external_attr_find() already
applies to the entries in its own attribute-list walk.

Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2: dropped the redundant Reported-by; reproducer omitted on the public list
    (available to the maintainers on request). Posting per security@kernel.org
    guidance that crafted-image filesystem bugs are out of the security threat
    model.

Sibling of the ntfs_external_attr_find() look-ahead OOB (patch 2/3): the same
struct attr_list_entry fixed fields (lowest_vcn at 8, mft_reference at 16) are
read here with a weaker bound. The expression is carried from the legacy
fs/ntfs driver (removed v6.9; present since v2.6.12), where ntfs_malloc_nofs()
rounded every allocation up to kmalloc(PAGE_SIZE) and masked it. Please add the
appropriate Fixes: tag for the commit that introduced ntfs_read_inode_mount()
in the new driver.

 fs/ntfs/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 360bebd1ee3f..70fb216a1bd0 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -1996,7 +1996,8 @@ int ntfs_read_inode_mount(struct inode *vi)
 				goto em_put_err_out;
 			if (!al_entry->length)
 				goto em_put_err_out;
-			if ((u8 *)al_entry + 6 > al_end ||
+			if ((u8 *)al_entry + offsetof(struct attr_list_entry, name) >
+			    al_end ||
 			    (u8 *)al_entry + le16_to_cpu(al_entry->length) > al_end)
 				goto em_put_err_out;
 			next_al_entry = (struct attr_list_entry *)((u8 *)al_entry +
-- 
2.43.0

[PATCH v3 1/3] ntfs: validate resident attribute lists and harden the validator

[Bryam Vargas]

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.

Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v3 (Hyunchul Lee review):
 - Split the per-entry check into ntfs_attr_list_entry_is_valid() and call it
   from the whole series (this patch's list walk, the look-ahead in 2/3 and
   the $MFT walk in 3/3).
 - Require ale->length to be a multiple of 8 (on-disk attribute-list entries
   are 8-byte aligned).
v2: dropped the redundant Reported-by; reproducer omitted on the public list
   (available to the maintainers on request).

The out-of-bounds reads were confirmed under KASAN on the earlier revision
(crafted NTFS image, fs/ntfs built as a module): the crafted attribute list
that oopsed ntfs_external_attr_find() is now rejected at load with
"ntfs_read_locked_inode(): Corrupt attribute list." and a benign mkntfs
image still mounts. The validator is arch-independent -- struct
attr_list_entry is __packed, so sizeof() == offsetof(.., name) == 26 and
every field offset (length 4, lowest_vcn 8, mft_reference 16) is identical
built -m32 and -m64; a clean-room model of this v3 validator decides every
benign/crafted vector the same way on both ABIs, including the new
8-byte-alignment rejection. checkpatch clean.

 fs/ntfs/attrib.c | 78 ++++++++++++++++++++++++++++++++++++++----------
 fs/ntfs/attrib.h |  4 +++
 fs/ntfs/inode.c  |  6 ++++
 3 files changed, 73 insertions(+), 15 deletions(-)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 421c6cdcbb53..abc0add6f0c4 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -843,11 +843,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;
@@ -869,19 +929,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 f7acc7986b09..e2224fbfaabe 100644
--- 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 360bebd1ee3f..a5f7400fd19d 100644
--- 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:
-- 
2.43.0

[PATCH v3 2/3] ntfs: bound the look-ahead attribute-list entry in ntfs_external_attr_find()

[Bryam Vargas]

When resolving an attribute lookup with a non-zero @lowest_vcn,
ntfs_external_attr_find() peeks at the next $ATTRIBUTE_LIST entry to
decide whether to keep searching, but bounds that not-yet-validated
entry only with "(u8 *)next_al_entry + 6 < al_end" (which proves just
bytes 0..6 are in range) and "(u8 *)next_al_entry + length <= al_end"
with an attacker-controlled, non-8-aligned length. It then reads
next_al_entry->lowest_vcn (an __le64 at offset 8) and the name at
next_al_entry->name_offset, both of which can lie past al_end -- the
exact end of the kvmalloc'd attribute-list buffer (allocated at the
on-disk attr_list_size, no rounding). A crafted on-disk $ATTRIBUTE_LIST
whose last entry sits a few bytes before al_end therefore yields a slab
out-of-bounds read when the inode is read.

Validate the look-ahead entry with ntfs_attr_list_entry_is_valid() (added
in patch 1/3) before dereferencing lowest_vcn and the name, so the same
fixed-header, length and name bounds the main attribute-list walk uses now
guard this read too.

Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v3 (Hyunchul Lee review): use the extracted ntfs_attr_list_entry_is_valid()
   helper (added in 1/3) instead of an open-coded bound, so the look-ahead
   gets the same fixed-header, 8-byte-aligned-length and name checks as the
   main walk.
v2: dropped the redundant Reported-by; reproducer/KASAN splat omitted on the
   public list (available to the maintainers on request).

Confirmed under KASAN on the earlier revision: a slab out-of-bounds read of
next_al_entry->lowest_vcn during inode read of a crafted image; a benign
mkntfs image is KASAN-clean (control). Arch-independent (lowest_vcn at
offset 8, fixed header offsetof(.., name) == 26; identical geometry built
-m32/-m64). Please add the appropriate Fixes: tag for the commit that
introduced ntfs_external_attr_find() in the new fs/ntfs driver (v7.1 merge
window).

 fs/ntfs/attrib.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index abc0add6f0c4..e425c8d074c5 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -1185,9 +1185,8 @@ static int ntfs_external_attr_find(const __le32 type,
 		 * we have reached the right one or the search has failed.
 		 */
 		if (lowest_vcn && (u8 *)next_al_entry >= al_start &&
-				(u8 *)next_al_entry + 6 < al_end &&
-				(u8 *)next_al_entry + le16_to_cpu(
-					next_al_entry->length) <= al_end &&
+				ntfs_attr_list_entry_is_valid(next_al_entry,
+							      al_end) &&
 				le64_to_cpu(next_al_entry->lowest_vcn) <=
 					lowest_vcn &&
 				next_al_entry->type == al_entry->type &&
-- 
2.43.0

[PATCH v3 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount()

[Bryam Vargas]

The $MFT attribute-list walk in ntfs_read_inode_mount() validates each
entry only with "(u8 *)al_entry + 6 > al_end" and
"(u8 *)al_entry + le16_to_cpu(al_entry->length) > al_end", but then reads
al_entry->lowest_vcn (an __le64 at offset 8) and al_entry->mft_reference
(offset 16) -- fields beyond the 6 bytes proven in range. al_entry->length
is attacker-controlled and only required non-zero, so a short entry (e.g.
length 8) placed at the tail passes both checks while the lowest_vcn /
mft_reference reads fall past al_end.

al_end is ni->attr_list + attr_list_size (the on-disk size); the buffer is
kvzalloc(round_up(attr_list_size, SECTOR_SIZE)), so the sector rounding
usually absorbs the over-read -- but when attr_list_size is a multiple of
SECTOR_SIZE there is no slack and a crafted $MFT attribute list produces an
out-of-bounds read at mount time.

Validate the entry with ntfs_attr_list_entry_is_valid() (added in patch
1/3) before dereferencing it, matching the bound the other attribute-list
walks now use.

Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v3 (Hyunchul Lee review): validate with the extracted
   ntfs_attr_list_entry_is_valid() helper (added in 1/3) rather than an
   open-coded bound, matching the other attribute-list walks.
v2: dropped the redundant Reported-by; reproducer omitted on the public list
   (available to the maintainers on request).

Sibling of the ntfs_external_attr_find() look-ahead OOB (2/3): the same
struct attr_list_entry fixed fields (lowest_vcn at 8, mft_reference at 16)
are read here with a weaker bound. Geometry is arch-independent (identical
-m32/-m64). Please add the appropriate Fixes: tag for the commit that
introduced ntfs_read_inode_mount() in the new fs/ntfs driver (v7.1 merge
window).

 fs/ntfs/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index a5f7400fd19d..77d7ea4a855b 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -2002,8 +2002,7 @@ int ntfs_read_inode_mount(struct inode *vi)
 				goto em_put_err_out;
 			if (!al_entry->length)
 				goto em_put_err_out;
-			if ((u8 *)al_entry + 6 > al_end ||
-			    (u8 *)al_entry + le16_to_cpu(al_entry->length) > al_end)
+			if (!ntfs_attr_list_entry_is_valid(al_entry, al_end))
 				goto em_put_err_out;
 			next_al_entry = (struct attr_list_entry *)((u8 *)al_entry +
 					le16_to_cpu(al_entry->length));
-- 
2.43.0

Re: [PATCH v3 1/3] ntfs: validate resident attribute lists and harden the validator

[Hyunchul Lee]

2026년 6월 4일 (목) 오후 1:29, Bryam Vargas <hexlabsecurity@proton.me>님이 작성:
>
> 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.
>
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Looks good to me.

Reviewed-by: Hyunchul Lee <hyc.lee@gmail.com>

> ---
> v3 (Hyunchul Lee review):
>  - Split the per-entry check into ntfs_attr_list_entry_is_valid() and call it
>    from the whole series (this patch's list walk, the look-ahead in 2/3 and
>    the $MFT walk in 3/3).
>  - Require ale->length to be a multiple of 8 (on-disk attribute-list entries
>    are 8-byte aligned).
> v2: dropped the redundant Reported-by; reproducer omitted on the public list
>    (available to the maintainers on request).
>
> The out-of-bounds reads were confirmed under KASAN on the earlier revision
> (crafted NTFS image, fs/ntfs built as a module): the crafted attribute list
> that oopsed ntfs_external_attr_find() is now rejected at load with
> "ntfs_read_locked_inode(): Corrupt attribute list." and a benign mkntfs
> image still mounts. The validator is arch-independent -- struct
> attr_list_entry is __packed, so sizeof() == offsetof(.., name) == 26 and
> every field offset (length 4, lowest_vcn 8, mft_reference 16) is identical
> built -m32 and -m64; a clean-room model of this v3 validator decides every
> benign/crafted vector the same way on both ABIs, including the new
> 8-byte-alignment rejection. checkpatch clean.
>
>  fs/ntfs/attrib.c | 78 ++++++++++++++++++++++++++++++++++++++----------
>  fs/ntfs/attrib.h |  4 +++
>  fs/ntfs/inode.c  |  6 ++++
>  3 files changed, 73 insertions(+), 15 deletions(-)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 421c6cdcbb53..abc0add6f0c4 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -843,11 +843,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;
> @@ -869,19 +929,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 f7acc7986b09..e2224fbfaabe 100644
> --- 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 360bebd1ee3f..a5f7400fd19d 100644
> --- 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:
> --
> 2.43.0
>
>


-- 
Thanks,
Hyunchul

Re: [PATCH v3 2/3] ntfs: bound the look-ahead attribute-list entry in ntfs_external_attr_find()

[Hyunchul Lee]

2026년 6월 4일 (목) 오후 1:29, Bryam Vargas <hexlabsecurity@proton.me>님이 작성:
>
> When resolving an attribute lookup with a non-zero @lowest_vcn,
> ntfs_external_attr_find() peeks at the next $ATTRIBUTE_LIST entry to
> decide whether to keep searching, but bounds that not-yet-validated
> entry only with "(u8 *)next_al_entry + 6 < al_end" (which proves just
> bytes 0..6 are in range) and "(u8 *)next_al_entry + length <= al_end"
> with an attacker-controlled, non-8-aligned length. It then reads
> next_al_entry->lowest_vcn (an __le64 at offset 8) and the name at
> next_al_entry->name_offset, both of which can lie past al_end -- the
> exact end of the kvmalloc'd attribute-list buffer (allocated at the
> on-disk attr_list_size, no rounding). A crafted on-disk $ATTRIBUTE_LIST
> whose last entry sits a few bytes before al_end therefore yields a slab
> out-of-bounds read when the inode is read.
>
> Validate the look-ahead entry with ntfs_attr_list_entry_is_valid() (added
> in patch 1/3) before dereferencing lowest_vcn and the name, so the same
> fixed-header, length and name bounds the main attribute-list walk uses now
> guard this read too.
>
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Looks good to me.

Reviewed-by: Hyunchul Lee <hyc.lee@gmail.com>

> ---
> v3 (Hyunchul Lee review): use the extracted ntfs_attr_list_entry_is_valid()
>    helper (added in 1/3) instead of an open-coded bound, so the look-ahead
>    gets the same fixed-header, 8-byte-aligned-length and name checks as the
>    main walk.
> v2: dropped the redundant Reported-by; reproducer/KASAN splat omitted on the
>    public list (available to the maintainers on request).
>
> Confirmed under KASAN on the earlier revision: a slab out-of-bounds read of
> next_al_entry->lowest_vcn during inode read of a crafted image; a benign
> mkntfs image is KASAN-clean (control). Arch-independent (lowest_vcn at
> offset 8, fixed header offsetof(.., name) == 26; identical geometry built
> -m32/-m64). Please add the appropriate Fixes: tag for the commit that
> introduced ntfs_external_attr_find() in the new fs/ntfs driver (v7.1 merge
> window).
>
>  fs/ntfs/attrib.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index abc0add6f0c4..e425c8d074c5 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -1185,9 +1185,8 @@ static int ntfs_external_attr_find(const __le32 type,
>                  * we have reached the right one or the search has failed.
>                  */
>                 if (lowest_vcn && (u8 *)next_al_entry >= al_start &&
> -                               (u8 *)next_al_entry + 6 < al_end &&
> -                               (u8 *)next_al_entry + le16_to_cpu(
> -                                       next_al_entry->length) <= al_end &&
> +                               ntfs_attr_list_entry_is_valid(next_al_entry,
> +                                                             al_end) &&
>                                 le64_to_cpu(next_al_entry->lowest_vcn) <=
>                                         lowest_vcn &&
>                                 next_al_entry->type == al_entry->type &&
> --
> 2.43.0
>
>


-- 
Thanks,
Hyunchul

Re: [PATCH v3 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount()

[Hyunchul Lee]

2026년 6월 4일 (목) 오후 1:29, Bryam Vargas <hexlabsecurity@proton.me>님이 작성:
>
> The $MFT attribute-list walk in ntfs_read_inode_mount() validates each
> entry only with "(u8 *)al_entry + 6 > al_end" and
> "(u8 *)al_entry + le16_to_cpu(al_entry->length) > al_end", but then reads
> al_entry->lowest_vcn (an __le64 at offset 8) and al_entry->mft_reference
> (offset 16) -- fields beyond the 6 bytes proven in range. al_entry->length
> is attacker-controlled and only required non-zero, so a short entry (e.g.
> length 8) placed at the tail passes both checks while the lowest_vcn /
> mft_reference reads fall past al_end.
>
> al_end is ni->attr_list + attr_list_size (the on-disk size); the buffer is
> kvzalloc(round_up(attr_list_size, SECTOR_SIZE)), so the sector rounding
> usually absorbs the over-read -- but when attr_list_size is a multiple of
> SECTOR_SIZE there is no slack and a crafted $MFT attribute list produces an
> out-of-bounds read at mount time.
>
> Validate the entry with ntfs_attr_list_entry_is_valid() (added in patch
> 1/3) before dereferencing it, matching the bound the other attribute-list
> walks now use.
>
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> ---
> v3 (Hyunchul Lee review): validate with the extracted
>    ntfs_attr_list_entry_is_valid() helper (added in 1/3) rather than an
>    open-coded bound, matching the other attribute-list walks.
> v2: dropped the redundant Reported-by; reproducer omitted on the public list
>    (available to the maintainers on request).
>
> Sibling of the ntfs_external_attr_find() look-ahead OOB (2/3): the same
> struct attr_list_entry fixed fields (lowest_vcn at 8, mft_reference at 16)
> are read here with a weaker bound. Geometry is arch-independent (identical
> -m32/-m64). Please add the appropriate Fixes: tag for the commit that
> introduced ntfs_read_inode_mount() in the new fs/ntfs driver (v7.1 merge
> window).
>
>  fs/ntfs/inode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index a5f7400fd19d..77d7ea4a855b 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -2002,8 +2002,7 @@ int ntfs_read_inode_mount(struct inode *vi)
>                                 goto em_put_err_out;
>                         if (!al_entry->length)
>                                 goto em_put_err_out;

It seems that some of the above conditions can be removed.

> -                       if ((u8 *)al_entry + 6 > al_end ||
> -                           (u8 *)al_entry + le16_to_cpu(al_entry->length) > al_end)
> +                       if (!ntfs_attr_list_entry_is_valid(al_entry, al_end))
>                                 goto em_put_err_out;
>                         next_al_entry = (struct attr_list_entry *)((u8 *)al_entry +
>                                         le16_to_cpu(al_entry->length));
> --
> 2.43.0
>
>


-- 
Thanks,
Hyunchul

Re: [PATCH v3 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount()

[Bryam Vargas]

On Thu, 4 Jun 2026 17:44:45 +0900 Hyunchul Lee <hyc.lee@gmail.com> wrote:
> It seems that some of the above conditions can be removed.

Right -- with ntfs_attr_list_entry_is_valid() in place the
"if (!al_entry->length)" check just above the call is redundant. The
validator forces name_offset == sizeof(struct attr_list_entry) and
requires name_offset + name_length * sizeof(__le16) <= length, so
length == 0 is already rejected there (and length must be a multiple of
8, so a valid entry is at least 32 bytes and next_al_entry still
advances -- no zero-length loop).

I'll drop that check in v4. v4 also rebases onto the attribute
value-validation series that was just applied: the
ntfs_external_attr_find() rework there overlaps the change 2/3 makes, so
the series needs to go on top of it.

Bryam