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

[Hyunchul Lee]

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

Looks good to me.

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

> ---
> 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