Re: [PATCH v8 4/8] mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: spi-nor: UAF of SFDP cached data via NVMEM cell consumer after unbind
--

commit 27ebe83329cdf6f2e062cc50041bbf3a877fcaba
Author: Manikandan Muralidharan <manikandan.m@microchip.com>

mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device

This commit exposes the SFDP tables read from the SPI NOR flash as a
read-only NVMEM device. The device is conditionally registered when a
jedec,sfdp child node is present in the device tree, allowing consumers
to reference any SFDP data.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c

[ ... ]

> +static int spi_nor_sfdp_reg_read(void *priv, unsigned int offset,
> +				 void *val, size_t bytes)
> +{
> +	struct spi_nor *nor = priv;
> +	struct sfdp *sfdp = nor->sfdp;
> +	size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
> +
> +	if (offset >= sfdp_size || bytes > sfdp_size - offset)
> +		return -EINVAL;
> +
> +	/* The cached SFDP is kept in on-flash (little-endian) byte order. */
> +	memcpy(val, (u8 *)sfdp->dwords + offset, bytes);

[Severity: High]
Does this code introduce a potential use-after-free if the SPI NOR device is
unbound while an NVMEM cell consumer still holds a reference?

When an in-kernel consumer driver obtains an NVMEM cell reference to the
SFDP device, it increments the nvmem_device reference count. Later, if the
SPI NOR device is unbound via sysfs, the devres framework runs
devm_nvmem_unregister.

Looking at nvmem_unregister in drivers/nvmem/core.c, it only drops the
provider's reference count. It does not explicitly block future reads, clear
the operations pointer, or disconnect active consumers. Because the consumer
still holds a cell reference, the nvmem_device object and its reg_read ops
remain active.

Devres then proceeds to free the nor struct and nor->sfdp->dwords buffers
which were allocated earlier in the probe sequence.

If the consumer subsequently calls nvmem_cell_read, the read is forwarded
to spi_nor_sfdp_reg_read, which dereferences the already freed nor and sfdp
memory.

> +
> +	return 0;
> +}

[ ... ]

> +static int spi_nor_register_sfdp_nvmem(struct spi_nor *nor)
> +{
> +	struct device *dev = nor->dev;
> +	struct nvmem_config config = { };
> +	struct nvmem_device *nvmem;
> +	struct device_node *np;
> +
> +	if (!nor->sfdp)
> +		return 0;
> +
> +	np = of_get_compatible_child(dev_of_node(dev), "jedec,sfdp");
> +	if (!np)
> +		return 0;
> +
> +	config.dev = dev;
> +	config.of_node = np;
> +	config.name = "sfdp";
> +	config.id = NVMEM_DEVID_AUTO;
> +	config.owner = THIS_MODULE;
> +	config.read_only = true;
> +	config.word_size = 1;
> +	config.stride = 1;
> +	config.size = (int)(nor->sfdp->num_dwords * sizeof(*nor->sfdp->dwords));
> +	config.reg_read = spi_nor_sfdp_reg_read;
> +	config.priv = nor;
> +
> +	nvmem = devm_nvmem_register(dev, &config);

[Severity: High]
Is there a risk to binding the NVMEM registration to the devres lifecycle
here?

Since devres tears down in reverse order, devm_nvmem_unregister will run
before the SFDP memory is freed. But as noted above, this does not safely
destroy the NVMEM device if a consumer is holding a cell reference, allowing
the operations pointer to outlive the memory it accesses.

> +	of_node_put(np);
> +	if (IS_ERR(nvmem)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790847519.git.manikandan.m@microchip.com?part=4

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