Re: [PATCH v2] dax: fix dax_range_compare_iter() to use correct size and address offset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] fs/dax: memcmp on IOMAP_UNWRITTEN exposes uninitialized PMEM data via dedupe
- [High] fsdax: CPU soft lockup due to unbounded memcmp in dedupe
- [High] fs/dax: dax_dedupe_file_range_compare() drops error and returns false dedup success
--

commit 035a3cbae242fa7d9d78dcd078b21ab3eeaecc67
Author: Jianyun Gao <jianyungao89@gmail.com>

dax: fix dax_range_compare_iter() to use correct size and address offset

This patch fixes two bugs in dax_range_compare_iter() when calling
dax_iomap_direct_access(). It corrects the size argument to use a length
rather than an absolute file offset, and it adds the page-in-page offset to
the source and destination addresses before comparing them with memcmp().

> diff --git a/fs/dax.c b/fs/dax.c
> index 08adeaed2baf1..7bee4314f905b 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -2190,6 +2190,7 @@ static int dax_range_compare_iter(struct iomap_iter *it_src,
>  	const struct iomap *dmap = &it_dest->iomap;
>  	loff_t pos1 = it_src->pos, pos2 = it_dest->pos;
>  	void *saddr, *daddr;
> +	size_t size1, size2;
>  	int id, ret;
>  
>  	len = min(len, min(smap->length, dmap->length));

[ ... ]

> @@ -2205,17 +2206,18 @@ static int dax_range_compare_iter(struct iomap_iter *it_src,
>  	}
>  
>  	id = dax_read_lock();
> -	ret = dax_iomap_direct_access(smap, pos1, ALIGN(pos1 + len, PAGE_SIZE),
> -				      &saddr, NULL);
> +	size1 = ALIGN(len + offset_in_page(pos1), PAGE_SIZE);
> +	ret = dax_iomap_direct_access(smap, pos1, size1, &saddr, NULL);
>  	if (ret < 0)
>  		goto out_unlock;

[Severity: High]
If dax_iomap_direct_access() fails here (e.g., from an I/O error or a size
issue) and returns a negative error code, does the upstream caller
dax_dedupe_file_range_compare() incorrectly mask the failure and report false
success?

When this function returns a negative error code,
dax_dedupe_file_range_compare() handles it like this:

fs/dax.c:dax_dedupe_file_range_compare() {
    ...
		status = dax_range_compare_iter(&src_iter, &dst_iter,
				min(src_iter.len, dst_iter.len), same);
		if (status < 0)
			return ret;
    ...
}

Because ret holds a positive bytes count from the loop's earlier iomap_iter()
call, a positive value is propagated back to vfs_dedupe_file_range(). 

The VFS intercepts the positive return as a success flag, bypasses block
remapping, and reports a successful deduplication result to userspace even
though the remapping aborted early.

>  
> -	ret = dax_iomap_direct_access(dmap, pos2, ALIGN(pos2 + len, PAGE_SIZE),
> -				      &daddr, NULL);
> +	size2 = ALIGN(len + offset_in_page(pos2), PAGE_SIZE);
> +	ret = dax_iomap_direct_access(dmap, pos2, size2, &daddr, NULL);
>  	if (ret < 0)
>  		goto out_unlock;
>  
> -	*same = !memcmp(saddr, daddr, len);
> +	*same = !memcmp(saddr + offset_in_page(pos1),
> +			daddr + offset_in_page(pos2), len);

[Severity: High]
Does this code expose uninitialized PMEM data if the extent is
IOMAP_UNWRITTEN?

While dax_range_compare_iter() explicitly checks for and skips IOMAP_HOLE
earlier in the function, it fails to check for IOMAP_UNWRITTEN.

As a result, the physical PMEM blocks for unwritten extents (e.g., from
fallocate) are mapped by dax_iomap_direct_access() without zero-filling
them.

This memcmp() then directly compares this uninitialized data against the
target extent.

Since early-exit from memcmp() is measurable, could this be used as a
timing side-channel to extract previous persistent memory contents, as well
as causing functional deduplication failures when comparing logically-zero
unwritten extents?

[Severity: High]
Can this memcmp() cause a CPU soft lockup and RCU stall?

When called via the FIDEDUPERANGE ioctl, vfs_dedupe_file_range() can cap the
deduplication length to 1GB.

Because DAX/PMEM mappings can be contiguous for gigabytes, this length could
be passed directly to a single synchronous memcmp() call here without any
cond_resched() yielding points.

>  	if (!*same)
>  		len = 0;
>  	dax_read_unlock(id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011064840.1296-1-jianyungao89@gmail.com?part=1