[PATCH] spi: spi-qpic-snand: publish the ECC context to snandc->qspi

[PATCH] spi: spi-qpic-snand: publish the ECC context to snandc->qspi

[Johan Alvarado]

qcom_spi_ooblayout_ecc() and qcom_spi_ooblayout_free() read the ECC
configuration through snandc->qspi->ecc. qcom_spi_probe() points it at a
zeroed scratch struct and only qcom_spi_ecc_prepare_io_req_pipelined(),
which runs on page I/O, ever updates it. qcom_spi_ecc_init_ctx_pipelined()
installs the ooblayout but does not publish the context it just
allocated, and qcom_spi_ecc_cleanup_ctx_pipelined() frees that context
without clearing the pointer.

spinand_init() calls mtd_ooblayout_count_freebytes() right after the ECC
context is created and before any page I/O, so the ooblayout always runs
against a pointer that does not describe the current context:

  - On a first probe it reads the zeroed struct from qcom_spi_probe(),
    so steps, bytes and bbm_size are 0. The count then returns 0 rather
    than an error, so the probe continues with mtd->oobavail set to 0.

  - On a probe retry it reads the ecc_cfg the previous attempt freed.

A retry is easy to hit. On IPQ5018 with the qcom,smem-part parser the
partition parse returns -EPROBE_DEFER until SMEM has probed, so the
first spi-nand probe defers. It defers inside
mtd_device_parse_register(), after mtd_otp_nvmem_add() has already read
the factory OTP - that read goes through prepare_io_req and leaves
snandc->qspi->ecc pointing at the context that spinand_cleanup() then
frees. The second probe allocates a new context, never publishes it, and
computes the OOB layout from the freed one. Once the slab has been
reused, qecc->steps holds garbage and

  oobregion->length = qecc->steps * 4;

goes negative. qcom_spi_ooblayout_free() only reports -ERANGE for
section 1 and later, so mtd_ooblayout_count_bytes() sums the regions and
returns that negative length as the byte count. The -512 below is
steps * 4 with steps == -128. It is a byte count that happens to
collide with -ERESTARTSYS, not an error the driver returned.
spinand_init() takes it as an error, and because it is not
-EPROBE_DEFER the driver core never retries and the NAND never appears:

  spi-nand spi0.0: ESMT SPI NAND was found.
  spi-nand spi0.0: probe with driver spi-nand failed with error -512
  UBI error: cannot open mtd rootfs, error -2
  Waiting for root device /dev/ubiblock0_1...

On a Mercusys MR80X (IPQ5018, ESMT F50D1G41LB) about half of the boots
failed to mount the rootfs, the outcome depending on whether the freed
memory had been overwritten yet.

Publish the context when it is created and clear the pointer when it is
destroyed. Clearing leaves snandc->qspi->ecc NULL after cleanup, which
is safe: the mtd is unregistered before cleanup_ctx runs, so no
ooblayout callback can follow.

Fixes: 7304d1909080 ("spi: spi-qpic: add driver for QCOM SPI NAND flash Interface")
Cc: stable@vger.kernel.org
Signed-off-by: Johan Alvarado <contact@c127.dev>
---
Tested on a Mercusys MR80X (IPQ5018, ESMT F50D1G41LB) running 6.18.44
with the equivalent change; mainline build-tested with W=1, no warnings.

 drivers/spi/spi-qpic-snand.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index 61b1f2eb19ce..0d0ae93ad4bf 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -411,6 +411,8 @@ static int qcom_spi_ecc_init_ctx_pipelined(struct nand_device *nand)
 	dev_dbg(snandc->dev, "ECC strength: %u bits per %u bytes\n",
 		ecc_cfg->strength, ecc_cfg->step_size);
 
+	snandc->qspi->ecc = ecc_cfg;
+
 	return 0;
 
 err_free_ecc_cfg:
@@ -427,6 +429,7 @@ static void qcom_spi_ecc_cleanup_ctx_pipelined(struct nand_device *nand)
 
 	kfree(snandc->qspi->oob_buf);
 	snandc->qspi->oob_buf = NULL;
+	snandc->qspi->ecc = NULL;
 	kfree(ecc_cfg);
 }
 
-- 
2.55.0

Re: [PATCH] spi: spi-qpic-snand: publish the ECC context to snandc->qspi

[Miquel Raynal]

On Mon, 24 Aug 2026 20:38:48 -0500, Johan Alvarado wrote:
> qcom_spi_ooblayout_ecc() and qcom_spi_ooblayout_free() read the ECC
> configuration through snandc->qspi->ecc. qcom_spi_probe() points it at a
> zeroed scratch struct and only qcom_spi_ecc_prepare_io_req_pipelined(),
> which runs on page I/O, ever updates it. qcom_spi_ecc_init_ctx_pipelined()
> installs the ooblayout but does not publish the context it just
> allocated, and qcom_spi_ecc_cleanup_ctx_pipelined() frees that context
> without clearing the pointer.
> 
> [...]

Applied to mtd/fixes, thanks!

[1/1] spi: spi-qpic-snand: publish the ECC context to snandc->qspi
      commit: 93bc7c4d2f4198586a6dbd7797c3bbf1ca845761

Patche(s) should be available on mtd/linux.git and will be
part of the next PR (provided that no robot complains by then).

Kind regards,
Miquèl

Re: [PATCH] spi: spi-qpic-snand: publish the ECC context to snandc->qspi

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 404 bytes --]

On Fri, Sep 04, 2026 at 07:38:14PM +0200, Miquel Raynal wrote:
> On Mon, 24 Aug 2026 20:38:48 -0500, Johan Alvarado wrote:
> > qcom_spi_ooblayout_ecc() and qcom_spi_ooblayout_free() read the ECC
> > configuration through snandc->qspi->ecc. qcom_spi_probe() points it at a

> [1/1] spi: spi-qpic-snand: publish the ECC context to snandc->qspi
>       commit: 93bc7c4d2f4198586a6dbd7797c3bbf1ca845761

Oh?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: spi-qpic-snand: publish the ECC context to snandc->qspi

[Miquel Raynal]

On 04/09/2026 at 19:14:33 +01, Mark Brown <broonie@kernel.org> wrote:

> On Fri, Sep 04, 2026 at 07:38:14PM +0200, Miquel Raynal wrote:
>> On Mon, 24 Aug 2026 20:38:48 -0500, Johan Alvarado wrote:
>> > qcom_spi_ooblayout_ecc() and qcom_spi_ooblayout_free() read the ECC
>> > configuration through snandc->qspi->ecc. qcom_spi_probe() points it at a
>
>> [1/1] spi: spi-qpic-snand: publish the ECC context to snandc->qspi
>>       commit: 93bc7c4d2f4198586a6dbd7797c3bbf1ca845761
>
> Oh?

-ETOOMANYKEYWORDS ._.

Dropped, my bad :)

Re: [PATCH] spi: spi-qpic-snand: publish the ECC context to snandc->qspi

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

On Fri, Sep 04, 2026 at 08:31:35PM +0200, Miquel Raynal wrote:
> On 04/09/2026 at 19:14:33 +01, Mark Brown <broonie@kernel.org> wrote:

> >> [1/1] spi: spi-qpic-snand: publish the ECC context to snandc->qspi
> >>       commit: 93bc7c4d2f4198586a6dbd7797c3bbf1ca845761

> > Oh?

> -ETOOMANYKEYWORDS ._.

> Dropped, my bad :)

Ah, OK!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: spi-qpic-snand: publish the ECC context to snandc->qspi

[Gabor Juhos]

Hi Johan,

2026. 08. 25. 3:38 keltezéssel, Johan Alvarado írta:
> qcom_spi_ooblayout_ecc() and qcom_spi_ooblayout_free() read the ECC
> configuration through snandc->qspi->ecc. qcom_spi_probe() points it at a
> zeroed scratch struct and only qcom_spi_ecc_prepare_io_req_pipelined(),
> which runs on page I/O, ever updates it. qcom_spi_ecc_init_ctx_pipelined()
> installs the ooblayout but does not publish the context it just
> allocated, and qcom_spi_ecc_cleanup_ctx_pipelined() frees that context
> without clearing the pointer.
> 
> spinand_init() calls mtd_ooblayout_count_freebytes() right after the ECC
> context is created and before any page I/O, so the ooblayout always runs
> against a pointer that does not describe the current context:
> 
>   - On a first probe it reads the zeroed struct from qcom_spi_probe(),
>     so steps, bytes and bbm_size are 0. The count then returns 0 rather
>     than an error, so the probe continues with mtd->oobavail set to 0.
> 
>   - On a probe retry it reads the ecc_cfg the previous attempt freed.
> 
> A retry is easy to hit. On IPQ5018 with the qcom,smem-part parser the
> partition parse returns -EPROBE_DEFER until SMEM has probed, so the
> first spi-nand probe defers. It defers inside
> mtd_device_parse_register(), after mtd_otp_nvmem_add() has already read
> the factory OTP - that read goes through prepare_io_req and leaves
> snandc->qspi->ecc pointing at the context that spinand_cleanup() then
> frees. The second probe allocates a new context, never publishes it, and
> computes the OOB layout from the freed one. Once the slab has been
> reused, qecc->steps holds garbage and
> 
>   oobregion->length = qecc->steps * 4;
> 
> goes negative. qcom_spi_ooblayout_free() only reports -ERANGE for
> section 1 and later, so mtd_ooblayout_count_bytes() sums the regions and
> returns that negative length as the byte count. The -512 below is
> steps * 4 with steps == -128. It is a byte count that happens to
> collide with -ERESTARTSYS, not an error the driver returned.
> spinand_init() takes it as an error, and because it is not
> -EPROBE_DEFER the driver core never retries and the NAND never appears:
> 
>   spi-nand spi0.0: ESMT SPI NAND was found.
>   spi-nand spi0.0: probe with driver spi-nand failed with error -512
>   UBI error: cannot open mtd rootfs, error -2
>   Waiting for root device /dev/ubiblock0_1...
> 
> On a Mercusys MR80X (IPQ5018, ESMT F50D1G41LB) about half of the boots
> failed to mount the rootfs, the outcome depending on whether the freed
> memory had been overwritten yet.
> 
> Publish the context when it is created and clear the pointer when it is
> destroyed. Clearing leaves snandc->qspi->ecc NULL after cleanup, which
> is safe: the mtd is unregistered before cleanup_ctx runs, so no
> ooblayout callback can follow.
> 
> Fixes: 7304d1909080 ("spi: spi-qpic: add driver for QCOM SPI NAND flash Interface")
> Cc: stable@vger.kernel.org
> Signed-off-by: Johan Alvarado <contact@c127.dev>
> ---
> Tested on a Mercusys MR80X (IPQ5018, ESMT F50D1G41LB) running 6.18.44
> with the equivalent change; mainline build-tested with W=1, no warnings.

Tested-by: Gabor Juhos <j4g8y7@gmail.com>

Tested on top of v7.3-rc2, running on the Tp-Link Archer AX55 v1. It works as
expected, yet I have some comments, see below.

> 
>  drivers/spi/spi-qpic-snand.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
> index 61b1f2eb19ce..0d0ae93ad4bf 100644
> --- a/drivers/spi/spi-qpic-snand.c
> +++ b/drivers/spi/spi-qpic-snand.c
> @@ -411,6 +411,8 @@ static int qcom_spi_ecc_init_ctx_pipelined(struct nand_device *nand)
>  	dev_dbg(snandc->dev, "ECC strength: %u bits per %u bytes\n",
>  		ecc_cfg->strength, ecc_cfg->step_size);
>  
> +	snandc->qspi->ecc = ecc_cfg;

For the sake of completeness we should remove the identical assignment from the
qcom_spi_ecc_prepare_io_req_pipelined() function as it gets redundant after the
change.

Additionally, the zeroed ecc_cfg instance allocated in qcom_spi_probe() become
unused so the allocation can be dropped. However, since this is not strictly
required for fixing the use-after-free issue, it could be done in a separate patch.

Regards,
Gabor