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