Re: [PATCH v2 3/3] mtd: spi-nor: take the flash lock in spi_nor_remove()

[sashiko-bot]

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

New issues:
- [Critical] Using a killable wait in the device removal path allows signals to abort critical hardware teardown, potentially bricking the device on the next boot.

Pre-existing issues:
- [Critical] devm-allocated driver data (struct spi_nor and its DMA bouncebuf) is freed while userspace can still access the MTD device.
- [High] Releasing the device lock before unregistering the MTD device exposes restored hardware to concurrent I/O and block-layer flushes, causing data corruption.
- [High] If `wait_event_killable()` is interrupted by a signal, `spi_nor_prep_and_lock()` returns an error without unwinding `spi_nor_prep()`, permanently leaking the SPI controller's prepared state.
--

commit f3b4a47f56b538ca231ed893cf829db3979e2e80
Author: Itai Handler <itai.handler@xxxxxxxxx>

mtd: spi-nor: take the flash lock in spi_nor_remove()

spi_nor_remove() restores the addressing mode with the same unlocked
call to spi_nor_restore() that spi_nor_shutdown() used before the
previous patch, and it is exposed the same way. This patch moves the
restore inside spi_nor_prep_and_lock() to ensure the lock is held.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 647bf8dce7193..8d0302565445b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3852,8 +3852,14 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  static int spi_nor_remove(struct spi_mem *spimem)
>  {
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +	int ret;
>  
> -	spi_nor_restore(nor);
> +	/* As in spi_nor_shutdown(), do not restore under an operation. */
> +	ret = spi_nor_prep_and_lock(nor);

[Severity: High]
This is a pre-existing issue, but does this function leak the SPI
controller's prepared state if interrupted by a signal?

Looking at drivers/mtd/spi-nor/core.c:spi_nor_prep_and_lock(), it
successfully calls spi_nor_prep(), but then waits via wait_event_killable():

	ret = wait_event_killable(nor->rww.wait,
				  spi_nor_rww_start_exclusive(nor));
	return ret;

If this wait is interrupted by a fatal signal, it returns -ERESTARTSYS
directly without calling spi_nor_unprep().

Can this permanently leak SPI controller resources like clocks and PM
runtime references?


[Severity: Critical]
Does using a killable wait here allow signals to abort the essential hardware
teardown?

If the process triggering the unbind receives a fatal signal, the lock
acquisition aborts and returns an error. This causes spi_nor_remove() to
skip the essential spi_nor_restore() hardware reset step, leaving the
physical flash in a non-default active state (e.g. 4-byte addressing mode).

Can this cause the system to become unbootable on the next boot if the
bootloader fails to read the flash in the unexpected 4-byte addressing mode?

> +	if (!ret) {
> +		spi_nor_restore(nor);
> +		spi_nor_unlock_and_unprep(nor);
> +	}

[Severity: High]
This is a pre-existing issue, but does dropping the lock here re-open the
race window before the MTD device is unregistered?

Concurrent I/O operations can acquire the lock after
spi_nor_unlock_and_unprep() but before or during mtd_device_unregister()
(which can trigger block layer flushes via mtdblock_release()).

These operations will execute using the driver's cached software state
(4-byte addressing), but the physical chip has already been restored to
3-byte addressing.

Can this result in silent data corruption?

>  
>  	/* Clean up MTD stuff. */
>  	return mtd_device_unregister(&nor->mtd);
>  }

[Severity: Critical]
This is a pre-existing issue, but is it possible for devm-allocated driver
data to be freed while userspace can still access the MTD device?

In drivers/mtd/spi-nor/core.c:spi_nor_probe(), struct spi_nor is allocated
using devm_kzalloc().

During spi_nor_remove(), mtd_device_unregister() drops a reference to the
MTD device, but does not block if userspace still holds an open file
descriptor:

	return mtd_device_unregister(&nor->mtd);

Since spi_nor_remove() returns immediately, the driver core will execute
devres cleanup, freeing struct spi_nor.

Could subsequent MTD operations from the open file descriptor dereference
the freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914081149.1916589-1-itai.handler@xxxxxxxxx?part=3