linux-kernel - Re: [PATCH] spi-nor: Verify written data in paranoid mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D981O3AA6NK9.2EEVUPM62EV6S@kernel.org>
Date: Wed, 16 Apr 2025 13:59:24 +0200
From: "Michael Walle" <mwalle@...nel.org>
To: Bence Csókás <csokas.bence@...lan.hu>,
 <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Cc: Szentendrei, Tamás <szentendrei.tamas@...lan.hu>,
 "Tudor Ambarus" <tudor.ambarus@...aro.org>, "Pratyush Yadav"
 <pratyush@...nel.org>, "Miquel Raynal" <miquel.raynal@...tlin.com>,
 "Richard Weinberger" <richard@....at>, "Vignesh Raghavendra"
 <vigneshr@...com>
Subject: Re: [PATCH] spi-nor: Verify written data in paranoid mode

Hi,

> Add MTD_SPI_NOR_PARANOID config option for verifying all written data to
> prevent silent bit errors to be undetected, at the cost of halving SPI
> bandwidth.

What is the use case for this? Why is it specific to SPI-NOR
flashes? Or should it rather be an MTD "feature". I'm not sure
whether this is the right way to do it, thus I'd love to hear more
about the background story to this.

> Co-developed-by: Szentendrei, Tamás <szentendrei.tamas@...lan.hu>
> Signed-off-by: Szentendrei, Tamás <szentendrei.tamas@...lan.hu>
> Signed-off-by: Csókás, Bence <csokas.bence@...lan.hu>
> ---
>  drivers/mtd/spi-nor/Kconfig | 10 ++++++++++
>  drivers/mtd/spi-nor/core.c  | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 24cd25de2b8b..425ea9a22424 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -68,6 +68,16 @@ config MTD_SPI_NOR_SWP_KEEP
>  
>  endchoice
>  
> +config MTD_SPI_NOR_PARANOID
> +	bool "Read back written data (paranoid mode)"

No kernel configs please. This doesn't scale. What if you have two
flashes and one should have this and one does not?

-michael

> +	help
> +	  This option makes the SPI NOR core read back all data on a write
> +	  and report an error if it doesn't match the written data. This can
> +	  safeguard against silent bit errors resulting from a faulty Flash,
> +	  controller oddities, bus noise etc.
> +
> +	  If you are unsure, select 'n'.
> +
>  source "drivers/mtd/spi-nor/controllers/Kconfig"
>  
>  endif # MTD_SPI_NOR
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index ac4b960101cc..ca05a6ec8afe 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2063,6 +2063,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  	size_t *retlen, const u_char *buf)
>  {
>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	u_char *verify_buf = NULL;
>  	size_t i;
>  	ssize_t ret;
>  	u32 page_size = nor->params->page_size;
> @@ -2073,6 +2074,14 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  	if (ret)
>  		return ret;
>  
> +#if IS_ENABLED(CONFIG_MTD_SPI_NOR_PARANOID)
> +	verify_buf = devm_kmalloc(nor->dev, page_size, GFP_KERNEL);
> +	if (!verify_buf) {
> +		ret = -ENOMEM;
> +		goto write_err;
> +	}
> +#endif
> +
>  	for (i = 0; i < len; ) {
>  		ssize_t written;
>  		loff_t addr = to + i;
> @@ -2099,11 +2108,35 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  		ret = spi_nor_wait_till_ready(nor);
>  		if (ret)
>  			goto write_err;
> +
> +#if IS_ENABLED(CONFIG_MTD_SPI_NOR_PARANOID)
> +		/* read back to make sure it's correct */
> +		ret = spi_nor_read_data(nor, addr, written, verify_buf);
> +		if (ret < 0)
> +			goto write_err;
> +		if (ret != written) {
> +			/* We shouldn't see short reads */
> +			dev_err(nor->dev, "Verify failed, written %zd but only read %zd",
> +				written, ret);
> +			ret = -EIO;
> +			goto write_err;
> +		}
> +
> +		if (memcmp(verify_buf, buf + i, written)) {
> +			dev_err(nor->dev, "Verify failed, compare mismatch!");
> +			ret = -EIO;
> +			goto write_err;
> +		}
> +#endif
> +
> +		ret = 0;
> +
>  		*retlen += written;
>  		i += written;
>  	}
>  
>  write_err:
> +	devm_kfree(nor->dev, verify_buf);
>  	spi_nor_unlock_and_unprep_pe(nor, to, len);
>  
>  	return ret;
>
> base-commit: 834a4a689699090a406d1662b03affa8b155d025


Download attachment "signature.asc" of type "application/pgp-signature" (298 bytes)