Re: [PATCH] fs/ntfs3: validate rec->used in journal-replay file record check

On 4/9/26 16:37, Greg Kroah-Hartman wrote:

> check_file_record() validates rec->total against the record size but
> never validates rec->used.  The do_action() journal-replay handlers read
> rec->used from disk and use it to compute memmove lengths:
>
>    DeleteAttribute:    memmove(attr, ..., used - asize - roff)
>    CreateAttribute:    memmove(..., attr, used - roff)
>    change_attr_size:   memmove(..., used - PtrOffset(rec, next))
>
> When rec->used is smaller than the offset of a validated attribute, or
> larger than the record size, these subtractions can underflow allowing
> us to copy huge amounts of memory in to a 4kb buffer, generally
> considered a bad idea overall.
>
> This requires a corrupted filesystem, which isn't a threat model the
> kernel really needs to worry about, but checking for such an obvious
> out-of-bounds value is good to keep things robust, especially on journal
> replay
>
> Fix this up by bounding rec->used correctly.
>
> This is much like commit b2bc7c44ed17 ("fs/ntfs3: Fix slab-out-of-bounds
> read in DeleteIndexEntryRoot") which checked different values in this
> same switch statement.
>
> Cc: Konstantin Komarov <[email protected]>
> Fixes: b46acd6a6a62 ("fs/ntfs3: Add NTFS journal")
> Cc: stable <[email protected]>
> Assisted-by: gregkh_clanker_t1000
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
>   fs/ntfs3/fslog.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ntfs3/fslog.c b/fs/ntfs3/fslog.c
> index 272e45276143..037df47fa9f3 100644
> --- a/fs/ntfs3/fslog.c
> +++ b/fs/ntfs3/fslog.c
> @@ -2791,13 +2791,14 @@ static inline bool check_file_record(const struct MFT_REC *rec,
>   	u16 fn = le16_to_cpu(rec->rhdr.fix_num);
>   	u16 ao = le16_to_cpu(rec->attr_off);
>   	u32 rs = sbi->record_size;
> +	u32 used = le32_to_cpu(rec->used);
>   
>   	/* Check the file record header for consistency. */
>   	if (rec->rhdr.sign != NTFS_FILE_SIGNATURE ||
>   	    fo > (SECTOR_SIZE - ((rs >> SECTOR_SHIFT) + 1) * sizeof(short)) ||
>   	    (fn - 1) * SECTOR_SIZE != rs || ao < MFTRECORD_FIXUP_OFFSET_1 ||
>   	    ao > sbi->record_size - SIZEOF_RESIDENT || !is_rec_inuse(rec) ||
> -	    le32_to_cpu(rec->total) != rs) {
> +	    le32_to_cpu(rec->total) != rs || used > rs || used < ao) {
>   		return false;
>   	}
>   
> @@ -2809,6 +2810,15 @@ static inline bool check_file_record(const struct MFT_REC *rec,
>   		return false;
>   	}
>   
> +	/*
> +	 * The do_action() handlers compute memmove lengths as
> +	 * "rec->used - <offset of validated attr>", which underflows when
> +	 * rec->used is smaller than the attribute walk reached.  At this
> +	 * point attr is the ATTR_END marker; rec->used must cover it.
> +	 */
> +	if (used < PtrOffset(rec, attr) + sizeof(attr->type))
> +		return false;
> +
>   	return true;
>   }
>   

Hello,

Thanks for the patch. Applied.

Regards,
Konstantin