Re: [PATCH 08/15] mtd: spi-nor: Fix SWP for octal DTR chips

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: spi-nor: uninitialized stack memory read causes spurious -EIO in SR2 check
--

commit 7f40280ba9ff306f160cda218a293f8e370be331
Author: Miquel Raynal <miquel.raynal@bootlin.com>

mtd: spi-nor: Fix SWP for octal DTR chips

During the recent rework of status registers handling, we mostly focused
on quad capable devices, partially skipping ODTR devices in our mental
model. It seems that many (if not all?) ODTR devices have no SR2 read
opcode and have other control mechanisms. Because of that, the current
support in swp.c is broken, because reading back the written value may
or may not lead to the expected result.

Let's extend the _careful() suffix to the helper reading back the values
and comparing them. Maybe this "carfulness" will have to be moved back
to the core, it is a bit early to state it. For now, let's just make
sure that we read back 0s without errors when virtually accessing SR2 on
those chips.

[ ... ]

> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
> index 7e667e4ca84d5..e9cf2ca232341 100644
> --- a/drivers/mtd/spi-nor/swp.c
> +++ b/drivers/mtd/spi-nor/swp.c
> @@ -240,6 +240,25 @@ static int spi_nor_read_sr1_and_sr2_careful(struct spi_nor *nor, u8 *sr)
>  	return spi_nor_read_sr2_careful(nor, &sr[1]);
>  }
>  
> +static int spi_nor_write_sr1_and_sr2_and_check_careful(struct spi_nor *nor, const u8 *sr)
> +{
> +	u8 tmp[2];

[Severity: High]
Does this allocate uninitialized memory on the stack?

> +	int ret;
> +
> +	ret = spi_nor_write_sr1_and_sr2(nor, sr);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_nor_read_sr1_and_sr2_careful(nor, tmp);
> +	if (ret)
> +		return ret;
> +
> +	if (sr[0] != tmp[0] || sr[1] != tmp[1])

[Severity: High]
Does this unconditionally compare tmp[1] against sr[1] even when it contains
uninitialized stack data?

In the call chain spi_nor_read_sr1_and_sr2_careful() ->
spi_nor_read_sr2_careful(), if the device lacks a read_sr2 opcode and is not
quad-enabled, the function falls back to returning 0 without populating the
sr2 value.

If tmp[1] remains uninitialized, this check might fail and return -EIO
sporadically for devices lacking a read_sr2 opcode, such as the ODTR devices
targeted by this patch. Could this lead to unpredictable failures during
lock and unlock operations?

Would zero-initializing tmp avoid this issue?

> +		return -EIO;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-winbond-master-spi-nor-w35t-v1-0-a22a2f95c26e@bootlin.com?part=8