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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Releasing the lock before unregistering the MTD device allows queued MTD operations to execute on a flash that has been restored to 3-byte mode, corrupting writes and erases.
- [Critical] Unregistering the MTD device does not wait for existing users to close their file descriptors, leading to a Use-After-Free of `struct spi_nor` on subsequent I/O.
- [High] Missing `spi_nor_unprep()` in the error path of `spi_nor_prep_and_lock()` causes permanent hardware resource leaks and deadlocks.
--

commit cca0f80dbf08e34d8606b95f2df252dd1ddf153c
Author: Itai Handler <itai.handler@gmail.com>

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 commit fixes
the issue by taking nor->lock for the restore to prevent unbinds
from running the restore while another thread is in the middle of
an operation.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 96dd6ae6d6191..51128c94d1ce3 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 spi_nor_prep_and_lock() leak resources
if interrupted by a signal?

If a user issues an operation using parallel locking (SNOR_F_RWW),
spi_nor_prep_and_lock() acquires resources via spi_nor_prep() and then enters
wait_event_killable():

drivers/mtd/spi-nor/core.c:spi_nor_prep_and_lock() {
    ...
    ret = spi_nor_prep(nor);
    if (ret)
        return ret;

    if (!spi_nor_use_parallel_locking(nor))
        ...
    else
        ret = wait_event_killable(nor->rww.wait,
                                  spi_nor_rww_start_exclusive(nor));

    return ret;
}

If a signal interrupts the wait, wait_event_killable() returns -ERESTARTSYS.
The function returns this error immediately without calling spi_nor_unprep(),
permanently leaking the acquired controller resources (such as clocks or
power management runtime) and deadlocking the mutex.

> +	if (!ret) {
> +		spi_nor_restore(nor);
> +		spi_nor_unlock_and_unprep(nor);
> +	}
>  
>  	/* Clean up MTD stuff. */
>  	return mtd_device_unregister(&nor->mtd);

[Severity: Critical]
This is a pre-existing issue, but does dropping the lock before unregistering
the MTD device leave a race window where pending operations can wake up and
execute?

A userspace process can queue operations (e.g., via a blocked character
device) while the SPI NOR driver is concurrently unbound. Since the lock is
released here, and mtd_device_unregister() doesn't wait for pending
operations to finish, the queued operations can acquire the lock and execute
before the device is fully torn down.

If this happens, the operations will execute while the driver software expects
4-byte addressing but the hardware has been restored to 3-byte mode, leading
to corrupted reads and writes.

[Severity: Critical]
This is a pre-existing issue, but does unregistering the MTD device without
waiting for existing users to close their file descriptors lead to a
Use-After-Free of struct spi_nor?

A userspace process can open the MTD device, incrementing its kref. When the
driver is unbound, spi_nor_remove() calls mtd_device_unregister().

As noted in the commit message, del_mtd_device() merely drops a reference
without waiting for active users to close the device.

When spi_nor_remove() returns, devres will immediately free the struct spi_nor
memory (which was allocated via devm_kzalloc), but the userspace process may
still issue operations or close the file descriptor, dereferencing the freed
memory.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910184452.895485-1-itai.handler@gmail.com?part=2