Re: [PATCH V3 6/9] dax/fsdev: fail probe on invalid pgmap offset

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Late Validation / Out-of-Bounds Memory Access
--

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

dax/fsdev: fail probe on invalid pgmap offset

This commit converts the WARN_ON when pgmap_phys > phys into a fatal error,
failing the probe with -EINVAL. This prevents the probe from continuing with
a misaligned virt_addr when the remapped region starts after the data region.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index a2d2eb20fb4d0..aac0130ab2833 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -310,8 +310,13 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  		u64 phys = dev_dax->ranges[0].range.start;
>  		u64 pgmap_phys = dev_dax->pgmap[0].range.start;
>  
> -		if (!WARN_ON(pgmap_phys > phys))
> -			data_offset = phys - pgmap_phys;
> +		if (pgmap_phys > phys) {
> +			dev_err(dev, "pgmap start %#llx exceeds data start %#llx\n",
> +				pgmap_phys, phys);
> +			rc = -EINVAL;
> +			goto err_pgmap;
> +		}

[Severity: High]
This is a pre-existing issue, but does adding this explicit validation check
here occur too late to prevent a kernel panic?

Looking at fsdev_dax_probe(), devm_memremap_pages() populates the vmemmap
starting at pgmap_phys. Just before this block, the code unconditionally
calls fsdev_clear_folio_state(), which iterates over dev_dax->ranges
starting at phys.

If pgmap_phys > phys (the invalid state checked here), phys falls before
the region where the vmemmap is populated. Accessing unpopulated memory
during the folio state cleanup would panic before we even reach this new
validation check.

Additionally, the devres cleanup action is registered immediately before
this check. If this new check fails and jumps to err_pgmap, the devres
unrolling will execute on the error path:

fsdev_dax_probe()
   fsdev_clear_folio_state(dev_dax);
   devm_add_action_or_reset(..., fsdev_clear_folio_state_action, ...);

   if (pgmap_phys > phys) {
       ...
       goto err_pgmap;
   }

Would this trigger the unsafe memory access again during cleanup?

Should the validation of pgmap_phys vs phys be moved earlier in the probe
sequence, before clearing the folio state?

> +		data_offset = phys - pgmap_phys;
>  
>  		pr_debug("%s: offset detected phys=%llx pgmap_phys=%llx offset=%llx\n",
>  		       __func__, phys, pgmap_phys, data_offset);

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