Re: [PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Dangling pointer vulnerability (Use-After-Free) in pgmap->ops on driver unbind
- [Critical] Invalid Virtual Address Calculation for disjoint DAX ranges causing data corruption
- [High] Probe failure leaves dangling dev_dax->pgmap preventing device rebinding
--

commit fd314d6aab8b188d638ba9782dcab88646401f10
Author: John Groves <John@Groves.net>

dax/fsdev: clear vmemmap_shift when binding static pgmap

This commit clears pgmap->vmemmap_shift for static DAX devices before
calling devm_memremap_pages(). This ensures the vmemmap is built for order-0
folios as required by fsdev, explicitly zeroing any stale state left over
from a previous device_dax binding.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index f315533b299e9..dbd722ed7ab05 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
[ ... ]
> @@ -237,6 +237,7 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  		}
>  
>  		pgmap = dev_dax->pgmap;
> +		pgmap->vmemmap_shift = 0;
>  	} else {
>  		size_t pgmap_size;
> 
> 		if (dev_dax->pgmap) {
> 			dev_warn(dev, "dynamic-dax with pre-populated page map\n");
> 			return -EINVAL;
> 		}
> 
> 		pgmap_size = struct_size(pgmap, ranges, dev_dax->nr_range - 1);
> 		pgmap = devm_kzalloc(dev, pgmap_size, GFP_KERNEL);
> 		if (!pgmap)
> 			return -ENOMEM;
> 
> 		pgmap->nr_range = dev_dax->nr_range;
> 		dev_dax->pgmap = pgmap;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
dangling pointer if the probe fails later.

In fsdev_dax_probe(), dev_dax->pgmap is assigned the devm_kzalloc'd pgmap.
If a subsequent probe step fails:

fsdev_dax_probe()
    dev_dax->pgmap = pgmap;
    ...
    addr = devm_memremap_pages(dev, pgmap);
    if (IS_ERR(addr))
        return PTR_ERR(addr);

The devm framework will free the pgmap memory upon returning the error, but
dev_dax->pgmap is never reset to NULL.

Subsequent attempts to re-bind the driver will see dev_dax->pgmap != NULL
and fail permanently with -EINVAL, erroneously claiming a pre-populated
page map.

Does this need a cleanup action to clear dev_dax->pgmap on failure?

Note that this was fixed later in the series by "dax/fsdev: clear
dev_dax->pgmap on probe failure".

[ ... ]
> 	pgmap->type = MEMORY_DEVICE_FS_DAX;
> 	pgmap->ops = &fsdev_pagemap_ops;
> 	pgmap->owner = dev_dax;
> 
> 	addr = devm_memremap_pages(dev, pgmap);
> 	if (IS_ERR(addr))
> 		return PTR_ERR(addr);

[Severity: Critical]
This is a pre-existing issue, but I am concerned about a dangling pointer
vulnerability on driver unbind.

When binding static DAX devices, fsdev_dax_probe() assigns the module-local
&fsdev_pagemap_ops to pgmap->ops and dev_dax to pgmap->owner.

However, there appears to be no corresponding cleanup action in fsdev_kill()
or fsdev_clear_ops() to clear these fields when the driver is unbound.

If the static device is subsequently bound to device_dax (which reuses the
same pgmap but does not overwrite pgmap->ops), and the dax_fsdev module is
unloaded, pgmap->ops becomes a dangling pointer to freed module memory.

Could a hardware memory failure in the mapped DAX range then trigger
memory_failure_dev_pagemap(), dereferencing the stale
pgmap->ops->memory_failure pointer and causing a kernel panic?

[ ... ]
> 	if (kaddr)
> 		*kaddr = dev_dax->virt_addr + offset;

[Severity: Critical]
This is a pre-existing issue, but the virtual address computation here
seems to assume a contiguous virtual memory layout.

For dynamic DAX devices spanning multiple physically disjoint ranges,
__fsdev_dax_direct_access() calculates the kernel virtual address using
linear arithmetic:

    void *virt_addr = dev_dax->virt_addr + offset;

Because virt_addr maps directly over physical memory via __va(), wouldn't
a physical gap between ranges cause the linearly calculated virt_addr to
point into the unmapped gap rather than the correctly mapped subsequent
physical memory?

Does this return an invalid pointer for offsets beyond the first range,
leading to data corruption when callers read or write memory?

Note that this was fixed later in the series by "dax/fsdev: use __va(phys)
for kaddr in direct_access".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019e79caead2-5795328c-af48-4a93-b147-c11df7446e1a-000000@email.amazonses.com?part=3