Re: [PATCH v3 2/2] mtd: spi-nor: fix use-after-free on unbind with open handles

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] mtd: spi-nor: use-after-free in spi_nor_suspend() and spi_nor_resume()
- [High] mtd: spi-nor: use-after-free of nor->dev in dev_dbg() after unregistration
- [High] mtd: spi-nor: use-after-free of devres-backed nor->mtd.name after unbind
- [High] mtd: spi-nor: missing nor->removed check in spi_nor_get_device() after unbind
--

commit de3094832a517827e88d8d0203973cdc91471e95
Author: Tze Yee Ng <tze.yee.ng@altera.com>

mtd: spi-nor: fix use-after-free on unbind with open handles

This commit moves spi_nor and its associated buffers off devres on the
spi-mem probe path and refcounts struct spi_nor across registration and open
MTD handles. During remove(), it unregisters debugfs and the MTD device, marks
nor->removed under the lock to fence subsequent operations with -ENODEV, and
drops the registration reference.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 8bc117b46e028..76928bd8413ed 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c

[ ... ]

> @@ -3420,20 +3449,48 @@ static void spi_nor_resume(struct mtd_info *mtd)
>  		dev_err(dev, "resume() failed\n");
>  }

[Severity: Critical]
What happens if the system suspends and resumes after spi_nor_remove() while
an open file descriptor still holds a reference on the MTD device?

When a partition (or a master device with CONFIG_MTD_PARTITIONED_MASTER
enabled) is open during spi_nor_remove(), del_mtd_device() drops mtd->refcnt
from 2 to 1 without running mtd_device_release() -> device_unregister(), so
mtd->dev remains registered on mtd_class with mtd_cls_pm_ops active:

  mtd_cls_suspend() -> mtd_suspend() -> spi_nor_suspend()
  mtd_cls_resume()  -> mtd_resume()  -> spi_nor_resume()

Meanwhile, once spi_nor_remove() returns, devres_release_all(&spi->dev) frees
nor->spimem, nor->dirmap, nor->params->vreg_offset, and nor->params->priv, and
disables the vcc regulator:

  drivers/mtd/spi-nor/core.c:spi_nor_suspend() {
      struct spi_nor *nor = mtd_to_spi_nor(mtd);
      int ret;

      /* Disable octal DTR mode if we enabled it. */
      ret = spi_nor_set_octal_dtr(nor, false);
      ...
  }

  drivers/mtd/spi-nor/core.c:spi_nor_resume() {
      struct spi_nor *nor = mtd_to_spi_nor(mtd);
      struct device *dev = nor->dev;
      int ret;

      /* re-initialize the nor chip */
      ret = spi_nor_init(nor);
      ...
  }

Since neither spi_nor_suspend() nor spi_nor_resume() acquires nor->lock or
checks nor->removed, won't a suspend/resume cycle execute SPI operations
through the freed nor->spimem and freed params fields?

> -static int spi_nor_get_device(struct mtd_info *mtd)
> +/* Free spi_nor and its buffers; safe on partial init. */
> +static void spi_nor_free(struct spi_nor *nor)
> +{
> +	spi_nor_debugfs_unregister(nor);
> +	kfree(nor->mtd.eraseregions);
> +	kfree(nor->bouncebuf);
> +	kfree(nor->params);
> +	kfree(nor);
> +}
> +
> +static void spi_nor_release(struct kref *kref)
> +{
> +	spi_nor_free(container_of(kref, struct spi_nor, refcnt));
> +}
> +
> +static struct module *spi_nor_controller_owner(struct spi_nor *nor)
>  {
> -	struct mtd_info *master = mtd_get_master(mtd);
> -	struct spi_nor *nor = mtd_to_spi_nor(master);
>  	struct device *dev;
>  
> +	/* The controller may be gone on the last put; use the cached owner. */
> +	if (nor->refcounted)
> +		return nor->controller_module;
> +
>  	if (nor->spimem)
>  		dev = nor->spimem->spi->controller->dev.parent;
>  	else
>  		dev = nor->dev;
>  
> -	if (!try_module_get(dev->driver->owner))
> +	return dev->driver->owner;
> +}
> +
> +static int spi_nor_get_device(struct mtd_info *mtd)
> +{
> +	struct mtd_info *master = mtd_get_master(mtd);
> +	struct spi_nor *nor = mtd_to_spi_nor(master);
> +
> +	if (!try_module_get(spi_nor_controller_owner(nor)))
>  		return -ENODEV;
>  
> +	if (nor->refcounted)
> +		kref_get(&nor->refcnt);
> +
>  	return 0;
>  }

[Severity: High]
Should spi_nor_get_device() fail with -ENODEV once nor->removed is set?

When spi_nor_remove() runs while a handle is open on a partition (or master
with CONFIG_MTD_PARTITIONED_MASTER), mtd_device_unregister() only decrements
mtd->refcnt without calling mtd_device_release(), so the MTD device stays in
mtd_idr until the open handle is closed and mtd_release() runs:

  mtdchar_open()
    -> get_mtd_device()
      -> __get_mtd_device()
        -> spi_nor_get_device()

Without checking nor->removed (or using kref_get_unless_zero()), new opens of
/dev/mtdX after unbind will still succeed and take new references on
nor->refcnt, allowing userspace to keep struct spi_nor and the controller
module pinned indefinitely after the original pre-unbind handle is closed.

In addition, in __put_mtd_device(), master->_put_device(master) ->
spi_nor_put_device() frees struct spi_nor immediately when the last handle
closes, whereas idr_remove(&mtd_idr, mtd->index) only runs in mtd_release()
when all device references on mtd->dev drop. If anything holds a temporary
reference on mtd->dev across the final close, could a concurrent
get_mtd_device() still find the mtd in mtd_idr and call spi_nor_get_device()
on a freed struct spi_nor?

[ ... ]

> @@ -3788,12 +3870,15 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  	if (ret)
>  		return ret;
>  
> -	nor = devm_kzalloc(dev, sizeof(*nor), GFP_KERNEL);
> +	nor = kzalloc_obj(*nor, GFP_KERNEL);
>  	if (!nor)
>  		return -ENOMEM;
>  
>  	nor->spimem = spimem;
>  	nor->dev = dev;
> +	nor->controller_module = spi->controller->dev.parent->driver->owner;
> +	nor->refcounted = true;
> +	kref_init(&nor->refcnt);

[Severity: High]
Does nor->dev need a get_device() reference tied to nor->refcnt, or need to be
avoided before nor->removed is checked?

With CONFIG_MTD_PARTITIONED_MASTER disabled, opening an unpartitioned master
/dev/mtd0 calls __get_mtd_device(), which increments nor->refcnt via
spi_nor_get_device() but does not increment master->refcnt:

  drivers/mtd/mtdcore.c:__get_mtd_device() {
      ...
      while (mtd) {
          if (mtd != master)
              kref_get(&mtd->refcnt);
          mtd = mtd->parent;
      }

      if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
          kref_get(&master->refcnt);
      ...
  }

If the SPI controller is unbound or the SPI device is unregistered while
/dev/mtd0 is open, spi_nor_remove() -> mtd_device_unregister() drops
nor->mtd.refcnt to 0 and unregisters nor->mtd.dev immediately, releasing its
parent reference on &spi->dev. Once spi_unregister_device() finishes, struct
spi_device (nor->dev) is freed while struct spi_nor remains alive for the open
descriptor.

A subsequent read, write, or erase on that open descriptor calls dev_dbg() on
nor->dev before spi_nor_prep_and_lock*() checks nor->removed:

  drivers/mtd/spi-nor/core.c:spi_nor_read() {
      ...
      dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len);

      ret = spi_nor_prep_and_lock_rd(nor, from_lock, len_lock);
      ...
  }

The same pattern appears in spi_nor_write(), spi_nor_erase(), and
sst_nor_write(). Could this dereference a freed nor->dev when debug logging is
enabled?

>  	spi_nor_set_flash_node(nor, dev->of_node);
>  
>  	spi_mem_set_drvdata(spimem, nor);

[Severity: High]
Right below this in spi_nor_probe(), nor->mtd.name is assigned from
spi_mem_get_name(spimem):

  drivers/mtd/spi-nor/core.c:spi_nor_probe() {
      ...
      if (data && data->name)
          nor->mtd.name = data->name;

      if (!nor->mtd.name)
          nor->mtd.name = spi_mem_get_name(spimem);
      ...
  }

In spi_mem_probe(), spimem->name is either dev_name(&spi->dev) or a string
allocated via ctlr->mem_ops->get_name(mem), which drivers such as
spi-nxp-fspi, spi-fsl-qspi, spi-nxp-xspi, spi-aspeed-smc, and
spi-cadence-quadspi allocate with devm_kasprintf().

When spi-nor or the SPI controller is unbound while an open handle keeps
nor->mtd alive in mtd_idr and sysfs, devres_release_all() frees that name
buffer. Won't subsequent reads of /proc/mtd (mtd_proc_show()),
/sys/class/mtd/mtdX/name (mtd_name_show()), or lookups via
get_mtd_device_nm() read freed memory from nor->mtd.name unless the string is
duplicated and freed in spi_nor_free()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022035352.52129-1-tze.yee.ng@altera.com?part=2

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/