linux-kernel - Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag

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: <556fe468-0080-ad05-8228-5ff8f1b3dac6@ti.com>
Date:   Mon, 16 Dec 2019 14:24:15 +0530
From:   Vignesh Raghavendra <vigneshr@...com>
To:     Michael Walle <michael@...le.cc>, <linux-mtd@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC:     Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Tudor Ambarus <tudor.ambarus@...rochip.com>
Subject: Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag

Hi,

On 15/12/19 12:49 am, Michael Walle wrote:
> Document the new flag "no-unlock".
> 
> Signed-off-by: Michael Walle <michael@...le.cc>
> ---
> Does the property need a prefix? I couldn't find any hint. If so, what
> should it be? "m25p," or "spi-nor," ?
> 
>  Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> index f03be904d3c2..2d305c893ed7 100644
> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> @@ -78,6 +78,12 @@ Optional properties:
>  		   cannot reboot properly if the flash is left in the "wrong"
>  		   state. This boolean flag can be used on such systems, to
>  		   denote the absence of a reliable reset mechanism.
> +- no-unlock : By default, linux unlocks the whole flash because there
> +		   are legacy flash devices which are locked by default
> +		   after reset. Set this flag if you don't want linux to
> +		   unlock the whole flash automatically. In this case you
> +		   can control the non-volatile bits by the
> +		   flash_lock/flash_unlock tools.
>  

Current SPI NOR framework unconditionally unlocks entire flash which
I agree is not the best thing to do, but I don't think we need
new DT property here. MTD cmdline partitions and DT partitions already 
provide a way to specify that a partition should remain locked[1][2]

SPI NOR framework should instead set MTD_POWERUP_LOCK flags in mtd->flags
for flash devices that power up with lock bits set. And MTD core will 
take care of unlocking flash regions while taking into account partition
flags defined by user as part of MTD partitions defined in DT or
via cmdline args.

So that change should to be set MTD_POWERUP_LOCK for
in SPI NOR core. Can you check below[3] (untested) diff helps?
This should prevent unlocking partitions that are to remain locked 
as specified in DT/cmdline 

[1] Documentation/devicetree/bindings/mtd/partition.txt
[2] drivers/mtd/parsers/cmdlinepart.c (see "lk" parameter)

[3]

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 1082b6bb1393..6adb950849f6 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4914,23 +4914,6 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
 	return nor->params.quad_enable(nor);
 }
 
-/**
- * spi_nor_unlock_all() - Unlocks the entire flash memory array.
- * @nor:	pointer to a 'struct spi_nor'.
- *
- * Some SPI NOR flashes are write protected by default after a power-on reset
- * cycle, in order to avoid inadvertent writes during power-up. Backward
- * compatibility imposes to unlock the entire flash memory array at power-up
- * by default.
- */
-static int spi_nor_unlock_all(struct spi_nor *nor)
-{
-	if (nor->flags & SNOR_F_HAS_LOCK)
-		return spi_nor_unlock(&nor->mtd, 0, nor->params.size);
-
-	return 0;
-}
-
 static int spi_nor_init(struct spi_nor *nor)
 {
 	int err;
@@ -4941,11 +4924,11 @@ static int spi_nor_init(struct spi_nor *nor)
 		return err;
 	}
 
-	err = spi_nor_unlock_all(nor);
-	if (err) {
-		dev_dbg(nor->dev, "Failed to unlock the entire flash memory array\n");
-		return err;
-	}
+	/*
+	 * Flashes may power up locked. Set this flag so that MTD core
+	 * takes care of unlocking partitions as required.
+	 */
+	nor->mtd.flags |= MTD_POWERUP_LOCK;
 
 	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
 		/*




-- 
Regards
Vignesh