Re: [PATCH V5 4/9] dax/fsdev: don't leave a dangling dev_dax->pgmap on probe failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unpatched dangling pointer in device.c leading to permanent probe failure and potential UAF
- [High] Concurrent sysfs mapping_store() execution causes TOCTOU heap buffer overflow and Use-After-Free in probe pathways
--

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

dax/fsdev: don't leave a dangling dev_dax->pgmap on probe failure

This patch factors out pgmap acquisition into fsdev_acquire_pgmap() and
defers the dev_dax->pgmap assignment until the probe can no longer fail.
This ensures a failed probe does not leave a dangling pointer.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 52f46b3e245ea..cc097167ad2c7 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -219,47 +219,62 @@ static const struct file_operations fsdev_fops = {
>  	.release = fsdev_release,
>  };
>  
> -static int fsdev_dax_probe(struct dev_dax *dev_dax)
> +/*
> + * Acquire the dev_pagemap for probe: the static (pre-populated) one if
> + * present, or a devm-allocated one for the dynamic case. Note that
> + * dev_dax->pgmap is not set here; fsdev_dax_probe() sets it only once
> + * probe succeeds, so a failed probe never leaves a dangling pointer
> + * to a devres-freed pgmap.
> + */
> +static struct dev_pagemap *fsdev_acquire_pgmap(struct dev_dax *dev_dax)
>  {

[ ... ]

> +	if (dev_dax->pgmap) {
> +		dev_warn(dev, "dynamic-dax with pre-populated page map\n");
> +		return ERR_PTR(-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_size = struct_size(pgmap, ranges, dev_dax->nr_range - 1);
> +	pgmap = devm_kzalloc(dev, pgmap_size, GFP_KERNEL);
> +	if (!pgmap)
> +		return ERR_PTR(-ENOMEM);

[Severity: High]
This is a pre-existing issue, but does this allocation race with a concurrent
sysfs mapping_store() execution?

Since mapping_store() in drivers/dax/bus.c dynamically adds memory ranges
without checking if dev->driver is bound, it can run concurrently while the
driver is probing.

If dev_dax->nr_range increments after pgmap_size is calculated but before
the loop below executes, couldn't this cause a heap buffer overflow when
writing to pgmap->ranges[i]?

> -		pgmap->nr_range = dev_dax->nr_range;
> -		dev_dax->pgmap = pgmap;
> +	pgmap->nr_range = dev_dax->nr_range;
> +	for (int i = 0; i < dev_dax->nr_range; i++)
> +		pgmap->ranges[i] = dev_dax->ranges[i].range;
>  
> -		for (i = 0; i < dev_dax->nr_range; i++) {
> -			struct range *range = &dev_dax->ranges[i].range;
> +	return pgmap;
> +}
>  
> -			pgmap->ranges[i] = *range;
> -		}
> -	}
> +static int fsdev_dax_probe(struct dev_dax *dev_dax)
> +{
> +	struct dax_device *dax_dev = dev_dax->dax_dev;
> +	struct device *dev = &dev_dax->dev;
> +	struct dev_pagemap *pgmap;
> +	struct inode *inode;
> +	u64 data_offset = 0;
> +	struct cdev *cdev;
> +	void *addr;
> +	int rc, i;
> +
> +	pgmap = fsdev_acquire_pgmap(dev_dax);

[Severity: High]
This isn't a bug introduced by this patch, but since you are fixing the
dangling dev_dax->pgmap pointer vulnerability here in fsdev_dax_probe(),
should the exact same fix be applied to dev_dax_probe() in drivers/dax/device.c?

It appears device.c still assigns dev_dax->pgmap immediately after
devm_kzalloc(), which would leave it dangling if a subsequent operation like
devm_request_mem_region() fails, causing subsequent probe attempts to
permanently fail.

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