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 <[email protected]> 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/[email protected]?part=1
