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