[PATCH v4 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. 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@xxxxxxxxx>
---
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