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@xxxxxxxxx>님이 작성:
>
> 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@xxxxxxxxx>
> ---
> 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