Re: [PATCH v2] ntfs: fix name offset validation in ntfs_non_resident_attr_value_is_valid

Hi:

  Thank you for the review. You're absolutely right on both points:

   1. name_length is u8, not __le16 - my mistake.

   2. Moving the check outside the if (name_length) block is better as
      it covers both named and unnamed attributes.

   However, I have a concern about using ATTR_COMPRESSION_MASK here.

   In ntfs_attr_update_meta() line 3610, the actual conversion check is:

       if (sparse && !(a->flags & (ATTR_IS_SPARSE | ATTR_IS_COMPRESSED)))

   This uses ATTR_IS_COMPRESSED (0x0001), not ATTR_COMPRESSION_MASK (0x00ff).

   If a malicious MFT record sets flags = 0x0002:
   - Your check would skip (0x0002 & 0x00ff is true)
   - But ntfs_attr_update_meta() would still do the forward shift
     (0x0002 & 0x0001 is false)

   Should we use ATTR_IS_COMPRESSED instead to match the actual conversion
   predicate? Or is there another reason for the mask that I'm missing?

   Otherwise, your suggested approach of checking:
       attr_len - mp_offset < sizeof(a->data.non_resident.compressed_size)
   is much cleaner than checking name_end + 8.

鍦 2026骞08鏈06鏃 09:39, Hongling Zeng 鍐欓亾:
> ntfs_attr_update_meta() performs memmove operations on attribute names
> when converting between sparse and non-sparse attributes:
>
> - Converting to sparse shifts the name forward by 8 bytes
>    (name_offset + 8)
> - Converting from sparse shifts the name backward by 8 bytes
>    (name_offset - 8)
>
> However, the validator does not check that name_offset is within safe
> boundaries for these operations. A malicious MFT record could set
> name_offset such that:
>
> 1. The name is positioned at the very end of a non-sparse attribute.
>     Converting to sparse would shift the name forward by 8 bytes,
>     writing beyond the attribute boundary.
>
> 2. The name overlaps with the mapping pairs, causing corruption during
>     conversion.
>
> Add validation to ensure:
> - For named attributes, name_offset is within valid bounds
> - Name does not extend beyond the attribute or overlap with mapping pairs
> - For non-sparse attributes, name_end + 8 fits within attr_len to allow
>    room for the forward shift when becoming sparse
>
> Note: name_offset validation only applies when name_length != 0, as
> unnamed attributes use name_offset = 0 which is valid.
>
> Fixes: 7e2a1c554bc4 ("ntfs: Fix min_len for compressed/sparse attributes in ntfs_non_resident_attr_value_is_valid()")
> Cc: [email protected]
> Signed-off-by: Hongling Zeng <[email protected]>
> ---
> Changes in v2:
> - Read name_length directly as an 8-bit field instead of using
>    le16_to_cpu().
> - Restrict the name forward-shift bounds check to the same attribute
>    types that can actually be converted by ntfs_attr_update_meta().
> ---
>   fs/ntfs/attrib.c | 33 ++++++++++++++++++++++++++++++++-
>   1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index d354c3b0fae1..ffd08633970a 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -693,6 +693,9 @@ static bool ntfs_non_resident_attr_value_is_valid(const struct attr_record *a)
>   	u32 attr_len;
>   	u32 min_len;
>   	u16 mp_offset;
> +	u16 name_offset;
> +	u8 name_len;
> +	u32 name_end;
>   
>   	attr_len = le32_to_cpu(a->length);
>   	min_len = offsetof(struct attr_record, data.non_resident.initialized_size) +
> @@ -706,7 +709,35 @@ static bool ntfs_non_resident_attr_value_is_valid(const struct attr_record *a)
>   		return false;
>   
>   	mp_offset = le16_to_cpu(a->data.non_resident.mapping_pairs_offset);
> -	return mp_offset >= min_len && mp_offset <= attr_len;
> +	if (mp_offset < min_len || mp_offset > attr_len)
> +		return false;
> +
> +	/*
> +	 * Validate name_offset for named attributes.
> +	 * may be zero for unnamed attributes.
> +	 */
> +	name_len = a->name_length;
> +	if (name_len) {
> +		name_offset = le16_to_cpu(a->name_offset);
> +
> +		if (name_offset < min_len || name_offset >= attr_len)
> +			return false;
> +
> +		name_end = name_offset + name_len * sizeof(__le16);
> +		if (name_end > attr_len || name_end > mp_offset)
> +			return false;
> +
> +		/*
> +		 * A normal non-resident attribute moves its name forward when
> +		 * it is converted to a sparse attribute.
> +		 */
> +		if (!(a->flags & (ATTR_IS_SPARSE | ATTR_COMPRESSION_MASK)) &&
> +		     name_end + 8 > attr_len)
> +			return false;
> +	}
> +
> +	return true;
> +
>   }
>   
>   static bool ntfs_attr_value_is_valid(struct ntfs_volume *vol,