[PATCH 0/2] mtd: spi-nor: fix the unlocked restore on shutdown and remove

[PATCH 0/2] mtd: spi-nor: fix the unlocked restore on shutdown and remove

[Itai Handler]

Two threads can talk to the flash at once during reboot/kexec and during
an unbind, because spi_nor_restore() is called without nor->lock while
MTD users are still attached. A busy flash silently ignores the restore
and is left in 4-byte addressing, which is exactly the failure the
restore was added to prevent; a restore landing inside a read corrupts
the rest of that read instead.

I reproduced this under QEMU, using a flash model extended to implement
erase busy time. With an erase outstanding, spi_nor_shutdown() issues
WREN, EX4B and WRDI; the chip refuses all three because it is busy, and
each one still reports success to the caller:

  the flash was mid-erase when spi_nor_shutdown() tried to put it back
  into 3-byte addressing, and refused 3 of its command(s):
  0x06 (four_byte=1), 0xe9 (four_byte=1), 0x04 (four_byte=1)

four_byte=1 is the mode the chip was left in, and so the mode the next
kernel inherits. With the patch, shutdown waits for the erase and the
restore reaches an idle chip. The reproduction is deterministic: the
workload keeps the flash busy continuously, so no timing window is
involved.

Two caveats on that. It runs on a 5.10 vendor tree rather than mainline,
though the path is unchanged - mainline's spi_nor_shutdown() has the
same unlocked spi_nor_restore() call. And what started the investigation
was intermittent hangs after kexec on a Zynq UltraScale+ board, which I
have not tied to this race.

Patch 1 fixes ->shutdown, which every reboot and kexec goes through,
and is marked for stable. Patch 2 fixes the identical problem in ->remove;
I have deliberately not marked it for stable, since nobody has reported
hitting it and it changes how long an unbind can block.

Note what patch 1 does not do: the restore still runs with MTD users
attached, so an operation starting after it completes still addresses a
3-byte chip with nor->addr_nbytes == 4. Serialising against operations
already in flight is what stops the restore being issued into a busy
chip; fully closing the window would mean stopping MTD from accepting
operations before ->shutdown, which seemed too big a change to fold in
here. I am happy to look at that separately if you would prefer it.

The patches are independent; patch 2 can be dropped without affecting
patch 1.

Itai Handler (2):
  mtd: spi-nor: take the flash lock in spi_nor_shutdown()
  mtd: spi-nor: take the flash lock in spi_nor_remove()

 drivers/mtd/spi-nor/core.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

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

[Itai Handler]

spi_nor_shutdown() calls spi_nor_restore() to put the flash back into
3-byte addressing before the system reboots or kexecs. It does so
without taking nor->lock, which every other path that talks to the chip
acquires through spi_nor_prep_and_lock().

device_shutdown() does not freeze userspace and does not stop kernel
threads; it walks the device list calling ->shutdown with all CPUs
online. Another thread can therefore be in the middle of an operation,
with the restore running concurrently with it. A write and a read are
both damaged, in different ways.

A program or erase leaves the flash busy, and a busy flash accepts only
status register reads and ignores everything else, including the EX4B
that spi_nor_restore() sends. Neither spi_nor_write_enable() nor
spi_nor_set_4byte_addr_mode() reads anything back, so the restore
reports success while the flash is left in 4-byte addressing. The next
boot stage then addresses it with 3 bytes and reads the wrong data,
which is the failure commit 59b356ffd0b0 ("mtd: m25p80: restore the
status of SPI flash when exiting") introduced this restore to prevent.

A read, by contrast, does not ignore the restore - it is corrupted by
it. spi_nor_read() holds the lock across a loop that issues one
spi_nor_read_data() per chunk, each using nor->addr_nbytes.
spi_nor_set_4byte_addr_mode() updates nor->params->addr_nbytes and not
nor->addr_nbytes, so a restore landing between two chunks switches the
chip to 3-byte addressing while the driver carries on sending 4 address
bytes. The rest of the transfer is addressed wrongly and returns wrong
data, and nothing reports an error. A restore may also soft reset the
chip in the middle of that same read.

Take nor->lock for the restore, so it runs between operations instead of
during one: a program or erase has finished waiting on the chip, and a
read has issued its last chunk. This is a locking fix rather than a
missing wait - each operation already waits for completion at the site
that started it.

This narrows the race without closing it. The restore still runs while
MTD users are attached, so an operation that starts after it has
completed will address a chip that is now in 3-byte mode while
nor->addr_nbytes is still 4. Closing that as well would mean having MTD
stop accepting operations before ->shutdown runs, which is a larger
change; serialising against the operations already in flight is what
keeps the restore itself from being issued into a busy chip.

Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
Cc: stable@vger.kernel.org
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
 drivers/mtd/spi-nor/core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index ccf4396cdcd0..96dd6ae6d619 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3862,8 +3862,21 @@ static int spi_nor_remove(struct spi_mem *spimem)
 static void spi_nor_shutdown(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
+	int ret;
+
+	/*
+	 * Wait for an operation started by another thread to finish.
+	 * device_shutdown() runs with MTD users still active: a busy flash
+	 * ignores the commands spi_nor_restore() issues, leaving it in
+	 * 4-byte address mode, and a restore landing mid-read changes the
+	 * chip's address width under the transfer.
+	 */
+	ret = spi_nor_prep_and_lock(nor);
+	if (ret)
+		return;
 
 	spi_nor_restore(nor);
+	spi_nor_unlock_and_unprep(nor);
 }
 
 /*

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

[Itai Handler]

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: the MTD device is still
registered at that point, so an unbind can run the restore while another
thread is in the middle of an operation. A busy flash silently ignores
the restore, and a restore that lands between two chunks of a read
switches the chip to 3-byte addressing while the driver keeps sending
4 address bytes.

Moving the restore after mtd_device_unregister() would not fix this.
Since commit 19bfa9ebebb5 ("mtd: use refcount to prevent corruption")
del_mtd_device() drops a reference instead of refusing with -EBUSY when
the device is in use, so unregistering returns right away and does not
wait for an operation that is already running.

Take nor->lock for the restore, as spi_nor_shutdown() now does. The
unregister stays unconditional, so a flash whose restore had to be
skipped is still torn down.

Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
 drivers/mtd/spi-nor/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 96dd6ae6d619..51128c94d1ce 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);
+	if (!ret) {
+		spi_nor_restore(nor);
+		spi_nor_unlock_and_unprep(nor);
+	}
 
 	/* Clean up MTD stuff. */
 	return mtd_device_unregister(&nor->mtd);

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

[sashiko-bot]

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

New issues:
- [High] Unsafe Killable Wait in Shutdown/Remove skips hardware restoration

Pre-existing issues:
- [Critical] Race condition leading to data corruption after shutdown restore
- [High] Resource leak in locking helpers on fatal signal interruption
--

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

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

This commit modifies spi_nor_shutdown() to acquire the device lock before
restoring the flash addressing mode. This serializes the restore operation
against operations already in flight during system shutdown.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index ccf4396cdcd04..96dd6ae6d6191 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3862,8 +3862,21 @@ static int spi_nor_remove(struct spi_mem *spimem)
>  static void spi_nor_shutdown(struct spi_mem *spimem)
>  {
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +	int ret;
> +
> +	/*
> +	 * Wait for an operation started by another thread to finish.
> +	 * device_shutdown() runs with MTD users still active: a busy flash
> +	 * ignores the commands spi_nor_restore() issues, leaving it in
> +	 * 4-byte address mode, and a restore landing mid-read changes the
> +	 * chip's address width under the transfer.
> +	 */
> +	ret = spi_nor_prep_and_lock(nor);

[Severity: High]
This is a pre-existing issue, but does spi_nor_prep_and_lock() leak device
preparation resources if interrupted by a fatal signal?

If a process is killed while waiting on wait_event_killable() inside
spi_nor_prep_and_lock(), it returns -ERESTARTSYS but fails to invoke
spi_nor_unprep() to roll back the earlier spi_nor_prep() call. Does this
also apply to the other locking helpers like spi_nor_prep_and_lock_pe() and
spi_nor_prep_and_lock_rd()?

> +	if (ret)
> +		return;

[Severity: High]
If a fatal signal is pending during the reboot process, such as from the OOM
killer or a manual SIGKILL, wait_event_killable() in spi_nor_prep_and_lock()
will immediately return -ERESTARTSYS.

Will this early return skip the spi_nor_restore() hardware state reset? Since
this skips restoring the 3-byte addressing mode, could the bootloader read
garbage on the next boot, leaving the device unbootable until a power cycle?

>  
>  	spi_nor_restore(nor);
> +	spi_nor_unlock_and_unprep(nor);

[Severity: Critical]
This is a pre-existing issue, but could releasing the lock while the MTD
device remains active expose the flash's modified 3-byte addressing state
to concurrent operations?

If a concurrent thread acquires the lock immediately after spi_nor_shutdown()
finishes and issues a write command, the driver will assume 4-byte mode
and send 4 address bytes. Could the hardware misinterpret the 4th address
byte as data, resulting in permanent data corruption by shifting the
destination address?

>  }

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

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

[sashiko-bot]

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