linux-kernel - Re: [PATCH v2 5/5] mtd: spi-nor: add Kconfig option to disable write protection at power-up

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: <0ea41a21-a1bb-9d19-f739-8e6ccdd6a421@ti.com>
Date:   Wed, 7 Aug 2019 21:54:37 +0530
From:   Vignesh Raghavendra <vigneshr@...com>
To:     <Tudor.Ambarus@...rochip.com>, <marek.vasut@...il.com>
CC:     <dwmw2@...radead.org>, <computersforpeace@...il.com>,
        <miquel.raynal@...tlin.com>, <richard@....at>,
        <boris.brezillon@...labora.com>, <linux-mtd@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <Nicolas.Ferre@...rochip.com>
Subject: Re: [PATCH v2 5/5] mtd: spi-nor: add Kconfig option to disable write
 protection at power-up


Hi Tudor

On 17-Jul-19 5:00 PM, Tudor.Ambarus@...rochip.com wrote:
> From: Tudor Ambarus <tudor.ambarus@...rochip.com>
[...]
> 
>  drivers/mtd/spi-nor/Kconfig   | 8 ++++++++
>  drivers/mtd/spi-nor/spi-nor.c | 7 +++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 6de83277ce8b..b550e10657f1 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -22,6 +22,14 @@ config MTD_SPI_NOR_USE_4K_SECTORS
>  	  Please note that some tools/drivers/filesystems may not work with
>  	  4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
>  
> +config MTD_SPI_NOR_DISABLE_POWER_UP_WRITE_PROTECTION
> +	bool "Disable write protection during power-up"
> +	default y
> +	help
> +	   Some spi-nor flashes are write protected by default after a power-on
> +	   reset cycle, in order to avoid inadvertend writes during power-up.
> +	   Disable the write protection during power-up.
> +


Hmm, how about setting MTD_POWERUP_LOCK flag in mtd->flags whenever
flash powers up with WP enabled? User can disable WP by declaring
partitions rw or keep partitions locked with ro. MTD core takes care of
calling mtd->unlock() depending up on the rw/ro flag as part of
add_mtd_device()

We could probably enhance spi_nor_unlock() to use
spi_nor_unlock_global_block_protection() when asked to unlock entire flash.

Kconfig option does not scale well for multi-platform build. There would
not be a way to have WP enabled on one platform but disabled on other.


Regards
Vignesh

>  config SPI_ASPEED_SMC
>  	tristate "Aspeed flash controllers in SPI mode"
>  	depends on ARCH_ASPEED || COMPILE_TEST
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index ffb53740031c..9b948295ef27 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1684,7 +1684,7 @@ static int spi_nor_clear_sr_bp(struct spi_nor *nor)
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor)
> +static int __maybe_unused spi_nor_spansion_clear_sr_bp(struct spi_nor *nor)
>  {
>  	int ret;
>  	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> @@ -1726,7 +1726,8 @@ static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor)
>  	return spi_nor_clear_sr_bp(nor);
>  }
>  
> -static int spi_nor_unlock_global_block_protection(struct spi_nor *nor)
> +static int __maybe_unused
> +spi_nor_unlock_global_block_protection(struct spi_nor *nor)
>  {
>  	int ret;
>  
> @@ -4049,6 +4050,7 @@ static int spi_nor_init(struct spi_nor *nor)
>  {
>  	int err;
>  
> +#ifdef CONFIG_MTD_SPI_NOR_DISABLE_POWER_UP_WRITE_PROTECTION
>  	/*
>  	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
>  	 * with the software protection bits set.
> @@ -4082,6 +4084,7 @@ static int spi_nor_init(struct spi_nor *nor)
>  			return err;
>  		}
>  	}
> +#endif
>  
>  	if (nor->quad_enable) {
>  		err = nor->quad_enable(nor);
>