Re: [PATCH] dax: fix wrong size passed to dax_iomap_direct_access() in dax_range_compare_iter()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] fsdax: dax_range_compare_iter() memcmp ignores page-in-page offset
- [High] fsdax: out-of-bounds access due to bypassed length check in dax_iomap_direct_access()
--

commit 05244bba82891c8d959214ace5da854f182c9e20
Author: Jianyun Gao <jianyungao89@gmail.com>

dax: fix wrong size passed to dax_iomap_direct_access() in dax_range_compare_iter()

dax_range_compare_iter() passes ALIGN(pos + len, PAGE_SIZE) as the size
argument to dax_iomap_direct_access(), but this is an absolute file
offset rather than a length. This causes the underlying
dax_direct_access() to request a massively inflated nr_pages via
PHYS_PFN(size), leading to unnecessarily large access ranges and
potential spurious -EHWPOISON from driver badblock scans.

> diff --git a/fs/dax.c b/fs/dax.c
> index 08adeaed2baf1..fb12d0c0dcd49 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,13 +2206,13 @@ 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 + (pos1 & ~PAGE_MASK), PAGE_SIZE);
> +	ret = dax_iomap_direct_access(smap, pos1, size1, &saddr, NULL);

[Severity: High]
Does this code risk an out-of-bounds access in the subsequent memcmp() if the
underlying physical DAX memory mapped is shorter than requested?

Looking at dax_iomap_direct_access(), it bypasses the length validation when
the pfnp argument is NULL:

fs/dax.c:dax_iomap_direct_access() {
    ...
	if (!pfnp)
		goto out_check_addr;
	rc = -EINVAL;
	if (PFN_PHYS(length) < size)
		goto out;
    ...
}

If the iomap extent crosses a device mapper target boundary, such as on
dm-linear, dm_dax_direct_access() truncates the mapping length and returns
fewer pages than requested.

Because dax_iomap_direct_access() is called here with a NULL pfnp, it skips
the length check and returns success even for short mappings.

Could this regression cause the subsequent memcmp() to read beyond the
physically contiguous mapped memory?

>  	if (ret < 0)
>  		goto out_unlock;
>  
> -	ret = dax_iomap_direct_access(dmap, pos2, ALIGN(pos2 + len, PAGE_SIZE),
> -				      &daddr, NULL);
> +	size2 = ALIGN(len + (pos2 & ~PAGE_MASK), PAGE_SIZE);
> +	ret = dax_iomap_direct_access(dmap, pos2, size2, &daddr, NULL);
>  	if (ret < 0)
>  		goto out_unlock;
>  

[Severity: High]
Is it possible that the memcmp() that follows compares the wrong file data
because it ignores the page-in-page offset?

The dax_iomap_direct_access() function translates a file offset to a kernel
virtual address, but the returned pointers saddr and daddr are strictly
page-aligned, ignoring any page-in-page offset (pos & ~PAGE_MASK).

When filesystem block sizes are smaller than the architecture's page size,
deduplication requests can be block-aligned but not page-aligned.

If memcmp() is called directly on saddr and daddr without adding the
pos1 & ~PAGE_MASK and pos2 & ~PAGE_MASK offsets:

fs/dax.c:dax_range_compare_iter() {
    ...
	*same = !memcmp(saddr, daddr, len);
    ...
}

Will this regression compare memory starting from the page boundary instead of
the actual file offset? Could this lead to FIDEDUPERANGE incorrectly
deduplicating non-identical blocks and causing data corruption?

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