linux-kernel - Re: [PATCH v3] mtd: 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: <87frhambri.fsf@bootlin.com>
Date: Mon, 12 May 2025 11:14:25 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Bence Csókás <csokas.bence@...lan.hu>
Cc: <linux-mtd@...ts.infradead.org>,  <linux-kernel@...r.kernel.org>,
  Richard Weinberger <richard@....at>,  Vignesh Raghavendra
 <vigneshr@...com>
Subject: Re: [PATCH v3] mtd: Verify written data in paranoid mode

On 12/05/2025 at 10:40:32 +02, Bence Csókás <csokas.bence@...lan.hu> wrote:

> Add MTD_PARANOID config option for verifying all written data to prevent
> silent bit errors being undetected, at the cost of some bandwidth overhead.
>
> Signed-off-by: Bence Csókás <csokas.bence@...lan.hu>
> ---
>  drivers/mtd/Kconfig   | 14 ++++++++++++
>  drivers/mtd/mtdcore.c | 51 +++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 796a2eccbef0..e75f4a57df6a 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -206,6 +206,20 @@ config MTD_PARTITIONED_MASTER
>  	  the parent of the partition device be the master device, rather than
>  	  what lies behind the master.
>  
> +config MTD_PARANOID
> +	bool "Read back written data (paranoid mode)"
> +	help
> +	  This option makes the MTD 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.
> +
> +	  It is up to the layer above MTD (e.g. the filesystem) to handle
> +	  this condition, for example by going read-only to prevent further
> +	  data corruption, or to mark a certain region of Flash as bad.
> +
> +	  If you are unsure, select 'n'.
> +
>  source "drivers/mtd/chips/Kconfig"
>  
>  source "drivers/mtd/maps/Kconfig"
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 5ba9a741f5ac..3f9874cd4126 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1745,8 +1745,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
>  }
>  EXPORT_SYMBOL_GPL(mtd_read_oob);
>  
> -int mtd_write_oob(struct mtd_info *mtd, loff_t to,
> -				struct mtd_oob_ops *ops)
> +static int _mtd_write_oob(struct mtd_info *mtd, loff_t to,
> +			  struct mtd_oob_ops *ops)

I don't like these '_' prefixes, they do not indicate much about the
content of the function. I don't think we need an extra function for
that, just include the check in mtd_write_oob?

>  {
>  	struct mtd_info *master = mtd_get_master(mtd);
>  	int ret;
> @@ -1771,6 +1771,53 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
>  
>  	return mtd_write_oob_std(mtd, to, ops);
>  }
> +
> +static int _mtd_verify(struct mtd_info *mtd, loff_t to, size_t len, const u8 *buf)
> +{
> +	struct device *dev = &mtd->dev;
> +	u_char *verify_buf;
> +	size_t r_retlen;
> +	int ret;
> +
> +	verify_buf = devm_kmalloc(dev, len, GFP_KERNEL);
> +	if (!verify_buf)
> +		return -ENOMEM;
> +
> +	ret = mtd_read(mtd, to, len, &r_retlen, verify_buf);
> +	if (ret < 0)
> +		goto err;
> +
> +	if (len != r_retlen) {
> +		/* We shouldn't see short reads */
> +		dev_err(dev, "Verify failed, written %zd but only read %zd",
> +			len, r_retlen);
> +		ret = -EIO;
> +		goto err;
> +	}
> +
> +	if (memcmp(verify_buf, buf, len)) {
> +		dev_err(dev, "Verify failed, compare mismatch!");
> +		ret = -EIO;
> +	}
> +
> +err:
> +	devm_kfree(dev, verify_buf);
> +	return ret;
> +}
> +
> +int mtd_write_oob(struct mtd_info *mtd, loff_t to,
> +		  struct mtd_oob_ops *ops)
> +{
> +	int ret = _mtd_write_oob(mtd, to, ops);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	if (IS_ENABLED(CONFIG_MTD_PARANOID))
> +		ret = _mtd_verify(mtd, to, ops->retlen, ops->datbuf);

Why _mtd_verify and not mtd_verify?