Re: [PATCH V5 2/9] dax/fsdev: fix multi-range offset in memory_failure handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Incorrect memory failure offset calculation for static DAX devices, causing the wrong filesystem blocks to be marked corrupted.

Pre-existing issues:
- [High] Use-After-Free in dax_pgoff_to_phys() due to concurrent reallocation of dev_dax->ranges array via sysfs.
--

commit 410f6bffae27c2c1b51ab4214947b135248b8594
Author: John Groves <John@Groves.net>

dax/fsdev: fix multi-range offset in memory_failure handler

This commit updates the offset calculation in the memory_failure handler for
multi-range DAX devices. It changes the logic to iterate over the device
pagemap's immutable ranges array instead of the potentially dynamically
reallocated dev_dax ranges array.

>     Walk the pagemap's own range array (pgmap->ranges[]) rather than
>     dev_dax->ranges[]. The pgmap copy is the immutable snapshot populated at
>     probe and is never mutated afterwards, whereas dev_dax->ranges[] can be
>     krealloc()'d by a concurrent sysfs mapping_store() (under dax_region_rwsem,
>     which this ->memory_failure callback does not hold). For dynamic devices the
>     two arrays are identical, so the reported offset is unchanged for the
>     multi-range case this targets.

[Severity: High]
This isn't a bug introduced by this patch, but since the commit message
mentions that dev_dax->ranges can be krealloc()'d by a concurrent sysfs
mapping_store(), does this expose a use-after-free risk in
dax_pgoff_to_phys()?

Unlike size_store() which checks if the device is bound to a driver,
mapping_store() appears to lack this check and can invoke
alloc_dev_dax_range() to reallocate the array while the device is
actively in use:

drivers/dax/bus.c:mapping_store() {
    ...
    rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc);
    ...
}

Meanwhile, filesystem I/O operations can locklessly iterate over the ranges
array:

drivers/dax/bus.c:dax_pgoff_to_phys() {
    ...
    for (i = 0; i < dev_dax->nr_range; i++) {
        struct dev_dax_range *dax_range = &dev_dax->ranges[i];
    ...
}

Could a concurrent sysfs write to the mapping attribute cause a
use-after-free if dax_pgoff_to_phys() is iterating the old ranges
array that was freed by krealloc()?

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 188b2526bee45..2c5de3d80a618 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c

[ ... ]

> @@ -135,11 +135,26 @@ static void fsdev_clear_ops(void *data)
>   * The core mm code in free_zone_device_folio() handles the wake_up_var()
>   * directly for this memory type.
>   */
> +static u64 fsdev_pfn_to_offset(struct dev_pagemap *pgmap, unsigned long pfn)
> +{
> +	phys_addr_t phys = PFN_PHYS(pfn);
> +	u64 offset = 0;
> +
> +	for (int i = 0; i < pgmap->nr_range; i++) {
> +		struct range *range = &pgmap->ranges[i];
> +
> +		if (phys >= range->start && phys <= range->end)
> +			return offset + (phys - range->start);

[Severity: Critical]
Does this calculation incorrectly include the data_offset padding for static
DAX devices?

For static DAX devices, the device's physical range starts with a metadata
padding area (data_offset). The old code calculated the offset based on
dev_dax->ranges[0].range.start, which represents the start of the actual
user data.

By using pgmap->ranges[0].start instead, which includes this metadata
padding, the logical offset might be inflated by data_offset. Could this
cause the filesystem to receive an incorrect offset in
dax_holder_notify_failure(), potentially resulting in poisoning the wrong
filesystem blocks while leaving the actual corrupted blocks unmarked?

> +		offset += range_len(range);
> +	}
> +	return -1ULL;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019eb7bcda4b-3f8edae9-d7a4-4bfa-aaea-fcef77fdbbc3-000000@email.amazonses.com?part=2