[PATCH 0/8] mtd: spi-nor: locking fixes and updates

[PATCH 0/8] mtd: spi-nor: locking fixes and updates

[Brian Norris]

Hi,

These are an assortment of fixes and updates to the SPI NOR lock/unlock
feature. The biggest new features are:
(a) Status Register protection; I don't see why this shouldn't be enabled by
    default. See patch 4's description.
(b) Bottom-block protection support (via TB status bit)
(c) Lock/unlock support for a few Winbond flash

Patch 7 (Top/Bottom protection support) is still an RFC, as I think it deserves
a bit more work. But I'm just getting it out there, as it's been sitting on my
plate for a while.

Regards,
Brian

Brian Norris (8):
  mtd: spi-nor: wait for SR_WIP to clear on initial unlock
  mtd: spi-nor: guard against underflows in stm_is_locked_sr
  mtd: spi-nor: silently drop lock/unlock for already locked/unlocked
    region
  mtd: spi-nor: disallow further writes to SR if WP# is low
  mtd: spi-nor: use BIT() for flash_info flags
  mtd: spi-nor: add SPI_NOR_HAS_LOCK flag
  mtd: spi-nor: add TB (Top/Bottom) protect support
  mtd: spi-nor: support lock/unlock for a few Winbond chips

 drivers/mtd/spi-nor/spi-nor.c |  153 +++++++++++++++++++++++++++++++++--------
 include/linux/mtd/spi-nor.h   |    2 +
 2 files changed, 128 insertions(+), 27 deletions(-)

-- 
1.7.9.5

[PATCH 1/8] mtd: spi-nor: wait for SR_WIP to clear on initial unlock

[Brian Norris]

Fixup a piece leftover by commit 32321e950d8a ("mtd: spi-nor: wait until
lock/unlock operations are ready"). That commit made us wait for the WIP
bit to settle after lock/unlock operations, but it missed the open-coded
"unlock" that happens at probe() time.

We should probably have this code utilize the unlock() routines in the
future, to avoid duplication, but unfortunately, flash which need to be
unlocked don't all have a proper ->flash_unlock() callback.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Stas Sergeev <stsp@users.sourceforge.net>
Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mtd/spi-nor/spi-nor.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index ed0c19c558b5..ef89bed1e5ea 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1236,6 +1236,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	    JEDEC_MFR(info) == SNOR_MFR_SST) {
 		write_enable(nor);
 		write_sr(nor, 0);
+		spi_nor_wait_till_ready(nor);
 	}
 
 	if (!mtd->name)
-- 
1.7.9.5

[PATCH 2/8] mtd: spi-nor: guard against underflows in stm_is_locked_sr

[Brian Norris]

Users of stm_is_locked_sr() might do arithmetic that could result in a
negative offset. For example, when stm_unlock() tries to check the
status of the eraseblock below the range, it doesn't check for:

  ofs - mtd->erasesize < 0

Instead of forcing callers to be extra careful, let's just make
stm_is_locked_sr() do the right thing and report errors for invalid
ranges.

Also, fixup the calculations in stm_unlock(), so we:
(a) can handle non-eraseblock-aligned offsets and
(b) don't look for a negative offset when checking the first block

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index ef89bed1e5ea..c19674573eec 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -447,6 +447,9 @@ static int stm_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
 	loff_t lock_offs;
 	uint64_t lock_len;
 
+	if (ofs < 0 || ofs + len > nor->mtd.size)
+		return -EINVAL;
+
 	stm_get_locked_range(nor, sr, &lock_offs, &lock_len);
 
 	return (ofs + len <= lock_offs + lock_len) && (ofs >= lock_offs);
@@ -543,9 +546,13 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	if (status_old < 0)
 		return status_old;
 
-	/* Cannot unlock; would unlock larger region than requested */
-	if (stm_is_locked_sr(nor, ofs - mtd->erasesize, mtd->erasesize,
-			     status_old))
+	/*
+	 * Check the eraseblock next to us; if locked, then this would unlock
+	 * larger region than requested
+	 */
+	if (ofs > 0 && stm_is_locked_sr(nor, ALIGN(ofs - mtd->erasesize,
+					mtd->erasesize), mtd->erasesize,
+					status_old))
 		return -EINVAL;
 
 	/*
-- 
1.7.9.5

[PATCH 3/8] mtd: spi-nor: silently drop lock/unlock for already locked/unlocked region

[Brian Norris]

If, for instance, the entire flash is already unlocked and I try to
mtd_unlock() the entire device, I don't expect to see an EINVAL error.
It should just silently succeed. Ditto for mtd_lock().

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c19674573eec..3a08aa53c171 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -518,8 +518,12 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 
 	status_new = (status_old & ~mask) | val;
 
+	/* Don't bother if they're the same */
+	if (status_new == status_old)
+		return 0;
+
 	/* Only modify protection if it will not unlock other areas */
-	if ((status_new & mask) <= (status_old & mask))
+	if ((status_new & mask) < (status_old & mask))
 		return -EINVAL;
 
 	write_enable(nor);
@@ -576,8 +580,12 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 
 	status_new = (status_old & ~mask) | val;
 
+	/* Don't bother if they're the same */
+	if (status_new == status_old)
+		return 0;
+
 	/* Only modify protection if it will not lock other areas */
-	if ((status_new & mask) >= (status_old & mask))
+	if ((status_new & mask) > (status_old & mask))
 		return -EINVAL;
 
 	write_enable(nor);
-- 
1.7.9.5

[PATCH 4/8] mtd: spi-nor: disallow further writes to SR if WP# is low

[Brian Norris]

Locking the flash is most useful if it provides real hardware security.
Otherwise, it's little more than a software permission bit.

A reasonable use case that provides real HW security might be like
follows:

(1) hardware WP# is deasserted
(2) program flash
(3) flash range is protected via status register
(4) hardware WP# is asserted
(5) flash protection range can no longer be changed, until WP# is
    deasserted

In this way, flash protection is co-owned by hardware and software.

Now, one would expect to be able to perform step (3) with
ioctl(MEMLOCK), except that the spi-nor driver does not set the Status
Register Protect bit (a.k.a. Status Register Write Disable (SRWD)), so
even though the range is now locked, it does not satisfy step (5) -- it
can still be changed by a call to ioctl(MEMUNLOCK).

So, let's enable status register protection after the first lock
command.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 3a08aa53c171..46da6bb706fa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -518,6 +518,9 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 
 	status_new = (status_old & ~mask) | val;
 
+	/* Disallow further writes if WP pin is asserted */
+	status_new |= SR_SRWD;
+
 	/* Don't bother if they're the same */
 	if (status_new == status_old)
 		return 0;
-- 
1.7.9.5

[PATCH 5/8] mtd: spi-nor: use BIT() for flash_info flags

[Brian Norris]

It's a little easier to read and make sure there are no collisions
(IMO).

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 46da6bb706fa..ac930e902ee3 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -61,14 +61,14 @@ struct flash_info {
 	u16		addr_width;
 
 	u16		flags;
-#define	SECT_4K			0x01	/* SPINOR_OP_BE_4K works uniformly */
-#define	SPI_NOR_NO_ERASE	0x02	/* No erase command needed */
-#define	SST_WRITE		0x04	/* use SST byte programming */
-#define	SPI_NOR_NO_FR		0x08	/* Can't do fastread */
-#define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works uniformly */
-#define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
-#define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
-#define	USE_FSR			0x80	/* use flag status register */
+#define SECT_4K			BIT(0)	/* SPINOR_OP_BE_4K works uniformly */
+#define SPI_NOR_NO_ERASE	BIT(1)	/* No erase command needed */
+#define SST_WRITE		BIT(2)	/* use SST byte programming */
+#define SPI_NOR_NO_FR		BIT(3)	/* Can't do fastread */
+#define SECT_4K_PMC		BIT(4)	/* SPINOR_OP_BE_4K_PMC works uniformly */
+#define SPI_NOR_DUAL_READ	BIT(5)	/* Flash supports Dual Read */
+#define SPI_NOR_QUAD_READ	BIT(6)	/* Flash supports Quad Read */
+#define USE_FSR			BIT(7)	/* use flag status register */
 };
 
 #define JEDEC_MFR(info)	((info)->id[0])
-- 
1.7.9.5

[PATCH 6/8] mtd: spi-nor: add SPI_NOR_HAS_LOCK flag

[Brian Norris]

We can't determine this purely by manufacturer type (see commit
67b9bcd36906 ("mtd: spi-nor: fix Spansion regressions (aliased with
Winbond)")), and it's not autodetectable by anything like SFDP. So make
a new flag for it.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index ac930e902ee3..8846b575b526 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -69,6 +69,7 @@ struct flash_info {
 #define SPI_NOR_DUAL_READ	BIT(5)	/* Flash supports Dual Read */
 #define SPI_NOR_QUAD_READ	BIT(6)	/* Flash supports Quad Read */
 #define USE_FSR			BIT(7)	/* use flag status register */
+#define SPI_NOR_HAS_LOCK	BIT(8)	/* Flash supports lock/unlock via SR */
 };
 
 #define JEDEC_MFR(info)	((info)->id[0])
@@ -1251,7 +1252,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 
 	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
 	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
-	    JEDEC_MFR(info) == SNOR_MFR_SST) {
+	    JEDEC_MFR(info) == SNOR_MFR_SST ||
+	    info->flags & SPI_NOR_HAS_LOCK) {
 		write_enable(nor);
 		write_sr(nor, 0);
 		spi_nor_wait_till_ready(nor);
@@ -1268,7 +1270,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	mtd->_read = spi_nor_read;
 
 	/* NOR protection support for STmicro/Micron chips and similar */
-	if (JEDEC_MFR(info) == SNOR_MFR_MICRON) {
+	if (JEDEC_MFR(info) == SNOR_MFR_MICRON ||
+			info->flags & SPI_NOR_HAS_LOCK) {
 		nor->flash_lock = stm_lock;
 		nor->flash_unlock = stm_unlock;
 		nor->flash_is_locked = stm_is_locked;
-- 
1.7.9.5

[RFC PATCH 7/8] mtd: spi-nor: add TB (Top/Bottom) protect support

[Brian Norris]

Some flash support a bit in the status register that inverts protection
so that it applies to the bottom of the flash, not the top. This yields
additions to the protection range table, as noted in the comments.

Because this feature is not universal to all flash that support
lock/unlock, control it via a new flag.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
This one hasn't been exhaustively tested yet, so I labeled it 'RFC'. I hope to
have some sort of test scripts (possibly targeting mtd-utils) ready, to help
make sure the locking APIs behave as expected. (And "as expected" is a little
ill-defined, IMO.)

 drivers/mtd/spi-nor/spi-nor.c |   85 ++++++++++++++++++++++++++++++++++++-----
 include/linux/mtd/spi-nor.h   |    2 +
 2 files changed, 77 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 8846b575b526..5e84e3c543aa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -70,6 +70,11 @@ struct flash_info {
 #define SPI_NOR_QUAD_READ	BIT(6)	/* Flash supports Quad Read */
 #define USE_FSR			BIT(7)	/* use flag status register */
 #define SPI_NOR_HAS_LOCK	BIT(8)	/* Flash supports lock/unlock via SR */
+#define SPI_NOR_HAS_TB		BIT(9)	/*
+					 * Flash SR has Top/Bottom (TB) protect
+					 * bit. Must be used with
+					 * SPI_NOR_HAS_LOCK.
+					 */
 };
 
 #define JEDEC_MFR(info)	((info)->id[0])
@@ -435,7 +440,10 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
 	} else {
 		pow = ((sr & mask) ^ mask) >> shift;
 		*len = mtd->size >> pow;
-		*ofs = mtd->size - *len;
+		if (nor->flags & SNOR_F_HAS_SR_TB && sr & SR_TB)
+			*ofs = 0;
+		else
+			*ofs = mtd->size - *len;
 	}
 }
 
@@ -458,12 +466,14 @@ static int stm_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
 
 /*
  * Lock a region of the flash. Compatible with ST Micro and similar flash.
- * Supports only the block protection bits BP{0,1,2} in the status register
+ * Supports the block protection bits BP{0,1,2} in the status register
  * (SR). Does not support these features found in newer SR bitfields:
- *   - TB: top/bottom protect - only handle TB=0 (top protect)
  *   - SEC: sector/block protect - only handle SEC=0 (block protect)
  *   - CMP: complement protect - only support CMP=0 (range is not complemented)
  *
+ * Support for the following is provided conditionally for some flash:
+ *   - TB: top/bottom protect
+ *
  * Sample table portion for 8MB flash (Winbond w25q64fw):
  *
  *   SEC  |  TB   |  BP2  |  BP1  |  BP0  |  Prot Length  | Protected Portion
@@ -476,6 +486,13 @@ static int stm_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
  *    0   |   0   |   1   |   0   |   1   |  2 MB         | Upper 1/4
  *    0   |   0   |   1   |   1   |   0   |  4 MB         | Upper 1/2
  *    X   |   X   |   1   |   1   |   1   |  8 MB         | ALL
+ *  ------|-------|-------|-------|-------|---------------|-------------------
+ *    0   |   1   |   0   |   0   |   1   |  128 KB       | Lower 1/64
+ *    0   |   1   |   0   |   1   |   0   |  256 KB       | Lower 1/32
+ *    0   |   1   |   0   |   1   |   1   |  512 KB       | Lower 1/16
+ *    0   |   1   |   1   |   0   |   0   |  1 MB         | Lower 1/8
+ *    0   |   1   |   1   |   0   |   1   |  2 MB         | Lower 1/4
+ *    0   |   1   |   1   |   1   |   0   |  4 MB         | Lower 1/2
  *
  * Returns negative on errors, 0 on success.
  */
@@ -485,6 +502,8 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	int status_old, status_new;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 shift = ffs(mask) - 1, pow, val;
+	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
+	bool use_top;
 	int ret;
 
 	status_old = read_sr(nor);
@@ -496,10 +515,24 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 		/* Does combined region extend to end? */
 		if (!stm_is_locked_sr(nor, ofs + len, mtd->size - ofs - len,
 				      status_old))
-			return -EINVAL;
-		len = mtd->size - ofs;
+			can_be_top = false;
+		else
+			len = mtd->size - ofs;
 	}
 
+	if (can_be_bottom && ofs != 0) {
+		/* Does combined region extend to beginning? */
+		if (!stm_is_locked_sr(nor, 0, ofs, status_old))
+			can_be_bottom = false;
+		else
+			ofs = 0;
+	}
+
+	if (!can_be_bottom && !can_be_top)
+		return -EINVAL;
+	/* Prefer top, if both are valid */
+	use_top = can_be_top;
+
 	/*
 	 * Need smallest pow such that:
 	 *
@@ -517,11 +550,14 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	if (!(val & mask))
 		return -EINVAL;
 
-	status_new = (status_old & ~mask) | val;
+	status_new = (status_old & ~mask & ~SR_TB) | val;
 
 	/* Disallow further writes if WP pin is asserted */
 	status_new |= SR_SRWD;
 
+	if (!use_top)
+		status_new |= SR_TB;
+
 	/* Don't bother if they're the same */
 	if (status_new == status_old)
 		return 0;
@@ -548,6 +584,9 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	int status_old, status_new;
 	u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
 	u8 shift = ffs(mask) - 1, pow, val;
+	loff_t lock_len;
+	bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
+	bool use_top;
 	int ret;
 
 	status_old = read_sr(nor);
@@ -555,13 +594,35 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 		return status_old;
 
 	/*
-	 * Check the eraseblock next to us; if locked, then this would unlock
+	 * Check the eraseblock below us; if locked, then this would unlock
 	 * larger region than requested
 	 */
 	if (ofs > 0 && stm_is_locked_sr(nor, ALIGN(ofs - mtd->erasesize,
 					mtd->erasesize), mtd->erasesize,
 					status_old))
+		can_be_top = false;
+
+	/*
+	 * If TB is supported: Check the eraseblock above us; if locked, then
+	 * this would unlock larger region than requested
+	 */
+	if (can_be_bottom && ofs + len < mtd->size && stm_is_locked_sr(nor,
+				mtd_mod_by_eb(ofs + len + mtd->erasesize, mtd),
+				mtd->erasesize, status_old))
+		can_be_bottom = false;
+
+	if (!can_be_bottom && !can_be_top)
 		return -EINVAL;
+	/* Prefer top, if both are valid */
+	use_top = can_be_top;
+	/*
+	 * lock_len: length of region (either top or bottom) that should remain
+	 * locked
+	 */
+	if (use_top)
+		lock_len = mtd->size - (ofs + len);
+	else
+		lock_len = ofs;
 
 	/*
 	 * Need largest pow such that:
@@ -572,8 +633,8 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	 *
 	 *   pow = floor(log2(size / len)) = log2(size) - ceil(log2(len))
 	 */
-	pow = ilog2(mtd->size) - order_base_2(mtd->size - (ofs + len));
-	if (ofs + len == mtd->size) {
+	pow = ilog2(mtd->size) - order_base_2(lock_len);
+	if (lock_len == 0) {
 		val = 0; /* fully unlocked */
 	} else {
 		val = mask - (pow << shift);
@@ -582,7 +643,9 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 			return -EINVAL;
 	}
 
-	status_new = (status_old & ~mask) | val;
+	status_new = (status_old & ~mask & ~SR_TB) | val;
+	if (!use_top)
+		status_new |= SR_TB;
 
 	/* Don't bother if they're the same */
 	if (status_new == status_old)
@@ -1291,6 +1354,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 
 	if (info->flags & USE_FSR)
 		nor->flags |= SNOR_F_USE_FSR;
+	if (info->flags & SPI_NOR_HAS_TB)
+		nor->flags |= SNOR_F_HAS_SR_TB;
 
 #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
 	/* prefer "small sector" erase if possible */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 62356d50815b..3c36113a88e1 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -85,6 +85,7 @@
 #define SR_BP0			BIT(2)	/* Block protect 0 */
 #define SR_BP1			BIT(3)	/* Block protect 1 */
 #define SR_BP2			BIT(4)	/* Block protect 2 */
+#define SR_TB			BIT(5)	/* Top/Bottom protect */
 #define SR_SRWD			BIT(7)	/* SR write protect */
 
 #define SR_QUAD_EN_MX		BIT(6)	/* Macronix Quad I/O */
@@ -116,6 +117,7 @@ enum spi_nor_ops {
 
 enum spi_nor_option_flags {
 	SNOR_F_USE_FSR		= BIT(0),
+	SNOR_F_HAS_SR_TB	= BIT(1),
 };
 
 /**
-- 
1.7.9.5

[PATCH 8/8] mtd: spi-nor: support lock/unlock for a few Winbond chips

[Brian Norris]

These are recent Winbond models that are known to have lock/unlock
support via writing the Status Register, and that also support the TB
(Top/Bottom) protection bit.

Tested on w25q32dw.

[Note on style: these entries are getting pretty long lines, so I picked
a style that seems reasonable for splitting up the flags separate from
the other mostly-similar fields.]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 5e84e3c543aa..cd3908e26712 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -942,11 +942,23 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "w25x16", INFO(0xef3015, 0, 64 * 1024,  32, SECT_4K) },
 	{ "w25x32", INFO(0xef3016, 0, 64 * 1024,  64, SECT_4K) },
 	{ "w25q32", INFO(0xef4016, 0, 64 * 1024,  64, SECT_4K) },
-	{ "w25q32dw", INFO(0xef6016, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+	{
+		"w25q32dw", INFO(0xef6016, 0, 64 * 1024,  64,
+			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
+	},
 	{ "w25x64", INFO(0xef3017, 0, 64 * 1024, 128, SECT_4K) },
 	{ "w25q64", INFO(0xef4017, 0, 64 * 1024, 128, SECT_4K) },
-	{ "w25q64dw", INFO(0xef6017, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
-	{ "w25q128fw", INFO(0xef6018, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+	{
+		"w25q64dw", INFO(0xef6017, 0, 64 * 1024, 128,
+			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
+	},
+	{
+		"w25q128fw", INFO(0xef6018, 0, 64 * 1024, 256,
+			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
+	},
 	{ "w25q80", INFO(0xef5014, 0, 64 * 1024,  16, SECT_4K) },
 	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16, SECT_4K) },
 	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256, SECT_4K) },
-- 
1.7.9.5

Re: [PATCH 4/8] mtd: spi-nor: disallow further writes to SR if WP# is low

[Ezequiel Garcia]

On 28 January 2016 at 02:51, Brian Norris <computersforpeace@gmail.com> wrote:
> Locking the flash is most useful if it provides real hardware security.
> Otherwise, it's little more than a software permission bit.
>
> A reasonable use case that provides real HW security might be like
> follows:
>
> (1) hardware WP# is deasserted
> (2) program flash
> (3) flash range is protected via status register
> (4) hardware WP# is asserted
> (5) flash protection range can no longer be changed, until WP# is
>     deasserted
>
> In this way, flash protection is co-owned by hardware and software.
>
> Now, one would expect to be able to perform step (3) with
> ioctl(MEMLOCK), except that the spi-nor driver does not set the Status
> Register Protect bit (a.k.a. Status Register Write Disable (SRWD)), so
> even though the range is now locked, it does not satisfy step (5) -- it
> can still be changed by a call to ioctl(MEMUNLOCK).
>
> So, let's enable status register protection after the first lock
> command.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 3a08aa53c171..46da6bb706fa 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -518,6 +518,9 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>
>         status_new = (status_old & ~mask) | val;
>
> +       /* Disallow further writes if WP pin is asserted */
> +       status_new |= SR_SRWD;
> +

No need to clear SR_SRWD in stm_unlock?

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 0/8] mtd: spi-nor: locking fixes and updates

[Ezequiel Garcia]

On 28 January 2016 at 02:51, Brian Norris <computersforpeace@gmail.com> wrote:
> Hi,
>
> These are an assortment of fixes and updates to the SPI NOR lock/unlock
> feature. The biggest new features are:
> (a) Status Register protection; I don't see why this shouldn't be enabled by
>     default. See patch 4's description.
> (b) Bottom-block protection support (via TB status bit)
> (c) Lock/unlock support for a few Winbond flash
>
> Patch 7 (Top/Bottom protection support) is still an RFC, as I think it deserves
> a bit more work. But I'm just getting it out there, as it's been sitting on my
> plate for a while.
>
> Regards,
> Brian
>
> Brian Norris (8):
>   mtd: spi-nor: wait for SR_WIP to clear on initial unlock
>   mtd: spi-nor: guard against underflows in stm_is_locked_sr
>   mtd: spi-nor: silently drop lock/unlock for already locked/unlocked
>     region
>   mtd: spi-nor: disallow further writes to SR if WP# is low
>   mtd: spi-nor: use BIT() for flash_info flags
>   mtd: spi-nor: add SPI_NOR_HAS_LOCK flag
>   mtd: spi-nor: add TB (Top/Bottom) protect support
>   mtd: spi-nor: support lock/unlock for a few Winbond chips
>

For all patches, except RFC 7/8:

Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 4/8] mtd: spi-nor: disallow further writes to SR if WP# is low

[Brian Norris]

Hi Ezequiel,

Thanks for the review.

On Thu, Jan 28, 2016 at 11:36:13AM -0300, Ezequiel Garcia wrote:
> On 28 January 2016 at 02:51, Brian Norris <computersforpeace@gmail.com> wrote:
> > Locking the flash is most useful if it provides real hardware security.
> > Otherwise, it's little more than a software permission bit.
> >
> > A reasonable use case that provides real HW security might be like
> > follows:
> >
> > (1) hardware WP# is deasserted
> > (2) program flash
> > (3) flash range is protected via status register
> > (4) hardware WP# is asserted
> > (5) flash protection range can no longer be changed, until WP# is
> >     deasserted
> >
> > In this way, flash protection is co-owned by hardware and software.
> >
> > Now, one would expect to be able to perform step (3) with
> > ioctl(MEMLOCK), except that the spi-nor driver does not set the Status
> > Register Protect bit (a.k.a. Status Register Write Disable (SRWD)), so
> > even though the range is now locked, it does not satisfy step (5) -- it
> > can still be changed by a call to ioctl(MEMUNLOCK).
> >
> > So, let's enable status register protection after the first lock
> > command.
> >
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c |    3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index 3a08aa53c171..46da6bb706fa 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -518,6 +518,9 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> >
> >         status_new = (status_old & ~mask) | val;
> >
> > +       /* Disallow further writes if WP pin is asserted */
> > +       status_new |= SR_SRWD;
> > +
> 
> No need to clear SR_SRWD in stm_unlock?

Good point.

I actually had thought about that earlier, but I didn't come up with a
great plan, and then I forgot about it when I was preparing this RFC. I
don't think we want *all* "unlock" operations to unprotect the status
register. What if we had the whole flash locked, and we're just calling
unlock on the top half, with the intention of leaving the bottom half
protected still?

So, maybe we want to clear SR_SRWD only when we unlock the *entire*
flash? What do you think?

Brian

Re: [PATCH 4/8] mtd: spi-nor: disallow further writes to SR if WP# is low

[Ezequiel Garcia]

On 28 January 2016 at 14:59, Brian Norris <computersforpeace@gmail.com> wrote:
> Hi Ezequiel,
>
> Thanks for the review.
>
> On Thu, Jan 28, 2016 at 11:36:13AM -0300, Ezequiel Garcia wrote:
>> On 28 January 2016 at 02:51, Brian Norris <computersforpeace@gmail.com> wrote:
>> > Locking the flash is most useful if it provides real hardware security.
>> > Otherwise, it's little more than a software permission bit.
>> >
>> > A reasonable use case that provides real HW security might be like
>> > follows:
>> >
>> > (1) hardware WP# is deasserted
>> > (2) program flash
>> > (3) flash range is protected via status register
>> > (4) hardware WP# is asserted
>> > (5) flash protection range can no longer be changed, until WP# is
>> >     deasserted
>> >
>> > In this way, flash protection is co-owned by hardware and software.
>> >
>> > Now, one would expect to be able to perform step (3) with
>> > ioctl(MEMLOCK), except that the spi-nor driver does not set the Status
>> > Register Protect bit (a.k.a. Status Register Write Disable (SRWD)), so
>> > even though the range is now locked, it does not satisfy step (5) -- it
>> > can still be changed by a call to ioctl(MEMUNLOCK).
>> >
>> > So, let's enable status register protection after the first lock
>> > command.
>> >
>> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>> > ---
>> >  drivers/mtd/spi-nor/spi-nor.c |    3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> > index 3a08aa53c171..46da6bb706fa 100644
>> > --- a/drivers/mtd/spi-nor/spi-nor.c
>> > +++ b/drivers/mtd/spi-nor/spi-nor.c
>> > @@ -518,6 +518,9 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>> >
>> >         status_new = (status_old & ~mask) | val;
>> >
>> > +       /* Disallow further writes if WP pin is asserted */
>> > +       status_new |= SR_SRWD;
>> > +
>>
>> No need to clear SR_SRWD in stm_unlock?
>
> Good point.
>
> I actually had thought about that earlier, but I didn't come up with a
> great plan, and then I forgot about it when I was preparing this RFC. I
> don't think we want *all* "unlock" operations to unprotect the status
> register. What if we had the whole flash locked, and we're just calling
> unlock on the top half, with the intention of leaving the bottom half
> protected still?
>

Right.

> So, maybe we want to clear SR_SRWD only when we unlock the *entire*
> flash? What do you think?
>

How about this:

1) ioctl(MEMLOCK) the entire flash (SR_SRWD is set)
2) ioctl(MEMUNLOCK) partially (SW_SRWD keeps set)
3) ioctl(MEMLOCK) the entire flash again

Not sure this use case make sense, but would (3)  be allowed given
SW_SRWD is set?
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 4/8] mtd: spi-nor: disallow further writes to SR if WP# is low

[Brian Norris]

On Thu, Jan 28, 2016 at 04:24:50PM -0300, Ezequiel Garcia wrote:
> On 28 January 2016 at 14:59, Brian Norris <computersforpeace@gmail.com> wrote:
> > So, maybe we want to clear SR_SRWD only when we unlock the *entire*
> > flash? What do you think?

I'll paste in the relevant datasheet details from w25q32fw, to make sure
we're on the same page here, noting that 'SRP0' is our 'SR_SRWD', and
we're not touching SRP1 (i.e., SRP1=0):

 "SRP1=0, SRP0=0, /WP=X: Software Protection
    /WP pin has no control. The Status register can be written to after
    a Write Enable instruction, WEL=1. [Factory Default]
  SRP1=0, SRP0=1, /WP=0: Hardware Protected
    When /WP pin is low the Status Register locked [sic] and cannot be
    written to.
  SRP1=0, SRP0=1, /WP=1: Hardware Unprotected
    When /WP pin is high the Status register is unlocked and can be
    written to after a Write Enable instruction, WEL=1."

> How about this:
> 
> 1) ioctl(MEMLOCK) the entire flash (SR_SRWD is set)
> 2) ioctl(MEMUNLOCK) partially (SW_SRWD keeps set)
> 3) ioctl(MEMLOCK) the entire flash again

I might be confused; are you making a suggestion of a new behavior, or
are you just trying to clarify my proposal? Because this sounds like it
matches my proposal.

> Not sure this use case make sense,

I suppose it could make sense, if you (e.g.) have some intermediate
steps toward determining the locked regions during factory programming.
Maybe a process would start by doing #1 and #2, then decide
conditionally whether to do #3. And only after the whole process is done
does something assert /WP=0 (in my case, a factory process would tie /WP
low).

> but would (3)  be allowed given
> SW_SRWD is set?

Yes, if /WP=1 (high).

Brian

Re: [RFC PATCH 7/8] mtd: spi-nor: add TB (Top/Bottom) protect support

[Brian Norris]

On Wed, Jan 27, 2016 at 09:51:46PM -0800, Brian Norris wrote:
> This one hasn't been exhaustively tested yet, so I labeled it 'RFC'. I hope to
> have some sort of test scripts (possibly targeting mtd-utils) ready, to help
> make sure the locking APIs behave as expected. (And "as expected" is a little
> ill-defined, IMO.)

I've written up some ugly (but effective) tests locally and found a few
edge case problems with this patch, so I'm going to refactor both this
patch and patch 2. Stay tuned.

Brian

Re: [PATCH 4/8] mtd: spi-nor: disallow further writes to SR if WP# is low

[Ezequiel Garcia]

On 28 January 2016 at 16:48, Brian Norris <computersforpeace@gmail.com> wrote:
> On Thu, Jan 28, 2016 at 04:24:50PM -0300, Ezequiel Garcia wrote:
>> On 28 January 2016 at 14:59, Brian Norris <computersforpeace@gmail.com> wrote:
>> > So, maybe we want to clear SR_SRWD only when we unlock the *entire*
>> > flash? What do you think?
>
> I'll paste in the relevant datasheet details from w25q32fw, to make sure
> we're on the same page here, noting that 'SRP0' is our 'SR_SRWD', and
> we're not touching SRP1 (i.e., SRP1=0):
>
>  "SRP1=0, SRP0=0, /WP=X: Software Protection
>     /WP pin has no control. The Status register can be written to after
>     a Write Enable instruction, WEL=1. [Factory Default]
>   SRP1=0, SRP0=1, /WP=0: Hardware Protected
>     When /WP pin is low the Status Register locked [sic] and cannot be
>     written to.
>   SRP1=0, SRP0=1, /WP=1: Hardware Unprotected
>     When /WP pin is high the Status register is unlocked and can be
>     written to after a Write Enable instruction, WEL=1."
>

Yes, we are on the same page.

>> How about this:
>>
>> 1) ioctl(MEMLOCK) the entire flash (SR_SRWD is set)
>> 2) ioctl(MEMUNLOCK) partially (SW_SRWD keeps set)
>> 3) ioctl(MEMLOCK) the entire flash again
>
> I might be confused; are you making a suggestion of a new behavior, or
> are you just trying to clarify my proposal? Because this sounds like it
> matches my proposal.
>

I was trying to clarify how the SRWD would work in that case, but I
forgot about /WP when I asked that!

>> Not sure this use case make sense,
>
> I suppose it could make sense, if you (e.g.) have some intermediate
> steps toward determining the locked regions during factory programming.
> Maybe a process would start by doing #1 and #2, then decide
> conditionally whether to do #3. And only after the whole process is done
> does something assert /WP=0 (in my case, a factory process would tie /WP
> low).
>
>> but would (3)  be allowed given
>> SW_SRWD is set?
>
> Yes, if /WP=1 (high).
>

Right. So, after giving some more thought do this, I'd say it might
make sense to clear SRWD only when unlocking the entire flash. If
anything else, it would allow a path to disable hardware protection on
the lock range?

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 4/8] mtd: spi-nor: disallow further writes to SR if WP# is low

[Brian Norris]

On Fri, Jan 29, 2016 at 10:22:34AM -0300, Ezequiel Garcia wrote:
> On 28 January 2016 at 16:48, Brian Norris <computersforpeace@gmail.com> wrote:
> > On Thu, Jan 28, 2016 at 04:24:50PM -0300, Ezequiel Garcia wrote:
> >> How about this:
> >>
> >> 1) ioctl(MEMLOCK) the entire flash (SR_SRWD is set)
> >> 2) ioctl(MEMUNLOCK) partially (SW_SRWD keeps set)
> >> 3) ioctl(MEMLOCK) the entire flash again

...

> >> but would (3)  be allowed given
> >> SW_SRWD is set?
> >
> > Yes, if /WP=1 (high).
> >
> 
> Right. So, after giving some more thought do this, I'd say it might
> make sense to clear SRWD only when unlocking the entire flash. If
> anything else, it would allow a path to disable hardware protection on
> the lock range?

Yes, that sounds fine to me, as it does allow removal of the HW
protection. So one could, for example, do:

0. bring /WP=1 (high)
1. unlock the whole flash
2. bring /WP=0 (low) -- flash is still unlocked
3. allow a one-time relocking of the flash via MEMLOCK
4. no more locking changes

I'll send out v2.

Brian