[PATCH v2 0/3] spi-nor block protection

[PATCH v2 0/3] spi-nor block protection

[Jonas Bonn]

Changed in v2:
* Provide the below cover letter
* Rebase patches so they apply cleanly on linux-next


In order to make device that's effectively read-only except to an
authorized user we need:

i)  Some way of defaulting block-protection to on when device is first
powered
ii)  Some way of controlling the write-protect signal so that the BP
(block protect) bits can't be changed

Some SPI flashes support the BPNV configuration register bit:  block
protect non-volatile.  When this bit is set, the block protection bits
BP0, BP1, and BP2 default to 1, effectively causing the flash memory to
becomes read-only at power on.  If we can set this bit, we solve problem
i) above.

Controlling the write-protect input is a matter for something external
to the flash itself.  Unfortunately, the WP# signal is only honoured if
the status register bit SRWD (status register write disable) is set.  If
we can have this bit always set, then we solve problem ii) above.

This short patch series provides the above bits, allowing for the
creation of a device that's effectively read-only to any actor who isn't
able to control the WP# signal.

Jonas Bonn (3):
  mtd: spi-nor: always respect write-protect input
  mtd: spi-nor: s25fl512s supports region locking
  mtd: spi-nor: allow setting the BPNV (default locked) bit

 drivers/mtd/mtdchar.c         |   6 ++
 drivers/mtd/mtdcore.c         |   8 +++
 drivers/mtd/spi-nor/spi-nor.c | 119 ++++++++++++++++++++++------------
 include/linux/mtd/mtd.h       |   2 +
 include/linux/mtd/spi-nor.h   |   1 +
 include/uapi/mtd/mtd-abi.h    |   1 +
 6 files changed, 97 insertions(+), 40 deletions(-)

-- 
2.19.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Jonas Bonn]

The status register bit SRWD (status register write disable) is
described in many words in the datasheets but effectively boils down to:

i) if set, respect WP# when trying to change protection bits;
ii) if unset, ignore WP# when trying to change protection bits

In short, the bit determines whether the WP# signal is honored or not.

It's difficult to imagine the use-case where the WP# is connected and
asserted but the user doesn't want to respect its setting.  As such,
this patch sets the SRWD bit unconditionally so that the WP# is _always_
respected; hardware that doesn't care about WP# normally won't even have
it connected.

Tested on a Cypress s25fl512s.  With this patch, the WP# is always
respected, irregardless of whether any flash protection bits are set.

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
CC: Marek Vasut <marek.vasut@gmail.com>
CC: David Woodhouse <dwmw2@infradead.org>
CC: Brian Norris <computersforpeace@gmail.com>
CC: Boris Brezillon <bbrezillon@kernel.org>
CC: Richard Weinberger <richard@nod.at>
CC: linux-mtd@lists.infradead.org
---
 drivers/mtd/spi-nor/spi-nor.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 13a5055e5f3f..4c8ce2b90838 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1231,9 +1231,6 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 
 	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;
 
@@ -1313,10 +1310,6 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 
 	status_new = (status_old & ~mask & ~SR_TB) | val;
 
-	/* Don't protect status register if we're fully unlocked */
-	if (lock_len == 0)
-		status_new &= ~SR_SRWD;
-
 	if (!use_top)
 		status_new |= SR_TB;
 
@@ -3898,6 +3891,7 @@ static int spi_nor_setup(struct spi_nor *nor,
 static int spi_nor_init(struct spi_nor *nor)
 {
 	int err;
+	int sr;
 
 	/*
 	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
@@ -3933,7 +3927,14 @@ static int spi_nor_init(struct spi_nor *nor)
 		set_4byte(nor, true);
 	}
 
-	return 0;
+	/* Always respect the WP# (write-protect) input */
+	sr = read_sr(nor);
+	if (sr < 0) {
+		dev_err(nor->dev, "error while reading status register\n");
+		return -EINVAL;
+	}
+	sr |= SR_SRWD;
+	return write_sr_and_check(nor, sr, SR_SRWD);
 }
 
 /* mtd resume handler */
-- 
2.19.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 2/3] mtd: spi-nor: s25fl512s supports region locking

[Jonas Bonn]

Both the BP0-2 bits and the TBPROT bit are supported on this chip.

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
CC: Marek Vasut <marek.vasut@gmail.com>
CC: David Woodhouse <dwmw2@infradead.org>
CC: Brian Norris <computersforpeace@gmail.com>
CC: Boris Brezillon <bbrezillon@kernel.org>
CC: Richard Weinberger <richard@nod.at>
CC: linux-mtd@lists.infradead.org
---
 drivers/mtd/spi-nor/spi-nor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 4c8ce2b90838..d6840c2b15dd 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1886,8 +1886,8 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
 	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
-	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
-	{ "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
+	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) },
+	{ "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) },
 	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
 	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
 	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
-- 
2.19.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 3/3] mtd: spi-nor: allow setting the BPNV (default locked) bit

[Jonas Bonn]

Setting the BPNV bit defaults the block protection bits BP0-2 to 1 at
reset.  This means that all the flash sectors are protected until they
are explicity unlocked by the user.

Together with protection of the status register via the SRWD bit and the
WP# signal, this allows a flash device to be effectively protected from
erasure by unauthorized actors.

The patch has been tested with a Cypress s25fl512s and works as
advertised.  The concern that I have with this, though, is that the BPNV
bit is "write once":  if somebody inadvertently sets this feature on the
device, it is irrevocable!  Whether or not this actually belongs in the
mainline kernel is therefore up for debate...

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
CC: Marek Vasut <marek.vasut@gmail.com>
CC: David Woodhouse <dwmw2@infradead.org>
CC: Brian Norris <computersforpeace@gmail.com>
CC: Boris Brezillon <bbrezillon@kernel.org>
CC: Richard Weinberger <richard@nod.at>
CC: linux-mtd@lists.infradead.org
---
 drivers/mtd/mtdchar.c         |   6 ++
 drivers/mtd/mtdcore.c         |   8 +++
 drivers/mtd/spi-nor/spi-nor.c | 102 +++++++++++++++++++++++-----------
 include/linux/mtd/mtd.h       |   2 +
 include/linux/mtd/spi-nor.h   |   1 +
 include/uapi/mtd/mtd-abi.h    |   1 +
 6 files changed, 88 insertions(+), 32 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index affb0ac39928..31dd308d34b1 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -803,6 +803,12 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
 		break;
 	}
 
+	case MEMSETDEFAULTLOCKED:
+	{
+		ret = mtd_set_default_locked(mtd);
+		break;
+	}
+
 	case MEMLOCK:
 	{
 		struct erase_info_user einfo;
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 254c3b413213..3cae22f161cb 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1759,6 +1759,14 @@ int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len)
 }
 EXPORT_SYMBOL_GPL(mtd_lock_user_prot_reg);
 
+int mtd_set_default_locked(struct mtd_info *mtd)
+{
+	if (!mtd->_set_default_locked)
+		return -EOPNOTSUPP;
+	return mtd->_set_default_locked(mtd);
+}
+EXPORT_SYMBOL_GPL(mtd_set_default_locked);
+
 /* Chip-supported device locking */
 int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d6840c2b15dd..4f9b82ff87ce 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -250,7 +250,7 @@ struct flash_info {
 	u16		page_size;
 	u16		addr_width;
 
-	u16		flags;
+	u32		flags;
 #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 */
@@ -279,6 +279,10 @@ struct flash_info {
 #define SPI_NOR_SKIP_SFDP	BIT(13)	/* Skip parsing of SFDP tables */
 #define USE_CLSR		BIT(14)	/* use CLSR command */
 #define SPI_NOR_OCTAL_READ	BIT(15)	/* Flash supports Octal Read */
+#define SPI_NOR_HAS_BPNV	BIT(16) /* Block protection bits can be set
+					 * non-volatile, meaning all blocks
+					 * are protected by default at reset
+					 */
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
@@ -1324,6 +1328,36 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	return write_sr_and_check(nor, status_new, mask);
 }
 
+/*
+ * Write status Register and configuration register with 2 bytes
+ * The first byte will be written to the status register, while the
+ * second byte will be written to the configuration register.
+ * Return negative if error occurred.
+ */
+static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr)
+{
+	int ret;
+
+	write_enable(nor);
+
+	ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2);
+	if (ret < 0) {
+		dev_err(nor->dev,
+			"error while writing configuration register\n");
+		return -EINVAL;
+	}
+
+	ret = spi_nor_wait_till_ready(nor);
+	if (ret) {
+		dev_err(nor->dev,
+			"timeout while writing configuration register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+
 /*
  * Check if a region of the flash is (completely) locked. See stm_lock() for
  * more info.
@@ -1342,6 +1376,35 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	return stm_is_locked_sr(nor, ofs, len, status);
 }
 
+static int spi_nor_set_default_locked(struct mtd_info *mtd)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	struct device *dev = nor->dev;
+	u8 sr_cr[2];
+	int ret;
+
+	ret = read_cr(nor);
+	if (ret < 0) {
+		dev_err(dev, "error while reading configuration register\n");
+		return -EINVAL;
+	}
+
+	if (ret & CR_BPNV)
+		return 0;
+
+	sr_cr[1] = ret | CR_BPNV;
+
+	/* Keep the current value of the Status Register. */
+	ret = read_sr(nor);
+	if (ret < 0) {
+		dev_err(dev, "error while reading status register\n");
+		return -EINVAL;
+	}
+	sr_cr[0] = ret;
+
+	return write_sr_cr(nor, sr_cr);
+}
+
 static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
@@ -1387,35 +1450,6 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	return ret;
 }
 
-/*
- * Write status Register and configuration register with 2 bytes
- * The first byte will be written to the status register, while the
- * second byte will be written to the configuration register.
- * Return negative if error occurred.
- */
-static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr)
-{
-	int ret;
-
-	write_enable(nor);
-
-	ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2);
-	if (ret < 0) {
-		dev_err(nor->dev,
-			"error while writing configuration register\n");
-		return -EINVAL;
-	}
-
-	ret = spi_nor_wait_till_ready(nor);
-	if (ret) {
-		dev_err(nor->dev,
-			"timeout while writing configuration register\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 /**
  * macronix_quad_enable() - set QE bit in Status Register.
  * @nor:	pointer to a 'struct spi_nor'
@@ -1886,8 +1920,8 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
 	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
-	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) },
-	{ "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) },
+	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_HAS_BPNV | USE_CLSR) },
+	{ "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_HAS_BPNV | USE_CLSR) },
 	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
 	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
 	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
@@ -4066,6 +4100,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 		mtd->_is_locked = spi_nor_is_locked;
 	}
 
+	if (info->flags & SPI_NOR_HAS_BPNV) {
+		mtd->_set_default_locked = spi_nor_set_default_locked;
+	}
+
 	/* sst nor chips use AAI word program */
 	if (info->flags & SST_WRITE)
 		mtd->_write = sst_write;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index b78e2c1b27b1..a07468b6677d 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -346,6 +346,7 @@ struct mtd_info {
 	int (*_writev) (struct mtd_info *mtd, const struct kvec *vecs,
 			unsigned long count, loff_t to, size_t *retlen);
 	void (*_sync) (struct mtd_info *mtd);
+	int (*_set_default_locked) (struct mtd_info *mtd);
 	int (*_lock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
 	int (*_unlock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
 	int (*_is_locked) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
@@ -535,6 +536,7 @@ static inline void mtd_sync(struct mtd_info *mtd)
 int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 int mtd_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len);
+int mtd_set_default_locked(struct mtd_info *mtd);
 int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs);
 int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs);
 int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs);
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 2353af8bac99..c7cc84a5c8f6 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -144,6 +144,7 @@
 #define FSR_PT_ERR		BIT(1)	/* Protection error bit */
 
 /* Configuration Register bits. */
+#define CR_BPNV			BIT(3)	/* Block protection non-volatile */
 #define CR_QUAD_EN_SPAN		BIT(1)	/* Spansion Quad I/O */
 
 /* Status Register 2 bits. */
diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
index aff5b5e59845..27b752796efa 100644
--- a/include/uapi/mtd/mtd-abi.h
+++ b/include/uapi/mtd/mtd-abi.h
@@ -204,6 +204,7 @@ struct otp_info {
  * without OOB, e.g., NOR flash.
  */
 #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
+#define MEMSETDEFAULTLOCKED	_IO('M', 25)
 
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace
-- 
2.19.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/3] spi-nor block protection

[Jonas Bonn]

Hi,

Just a gentle reminder that I'd like some feedback on these patches.

Thanks,
Jonas

On 29/01/2019 23:07, Jonas Bonn wrote:
> Changed in v2:
> * Provide the below cover letter
> * Rebase patches so they apply cleanly on linux-next
> 
> 
> In order to make device that's effectively read-only except to an
> authorized user we need:
> 
> i)  Some way of defaulting block-protection to on when device is first
> powered
> ii)  Some way of controlling the write-protect signal so that the BP
> (block protect) bits can't be changed
> 
> Some SPI flashes support the BPNV configuration register bit:  block
> protect non-volatile.  When this bit is set, the block protection bits
> BP0, BP1, and BP2 default to 1, effectively causing the flash memory to
> becomes read-only at power on.  If we can set this bit, we solve problem
> i) above.
> 
> Controlling the write-protect input is a matter for something external
> to the flash itself.  Unfortunately, the WP# signal is only honoured if
> the status register bit SRWD (status register write disable) is set.  If
> we can have this bit always set, then we solve problem ii) above.
> 
> This short patch series provides the above bits, allowing for the
> creation of a device that's effectively read-only to any actor who isn't
> able to control the WP# signal.
> 
> Jonas Bonn (3):
>    mtd: spi-nor: always respect write-protect input
>    mtd: spi-nor: s25fl512s supports region locking
>    mtd: spi-nor: allow setting the BPNV (default locked) bit
> 
>   drivers/mtd/mtdchar.c         |   6 ++
>   drivers/mtd/mtdcore.c         |   8 +++
>   drivers/mtd/spi-nor/spi-nor.c | 119 ++++++++++++++++++++++------------
>   include/linux/mtd/mtd.h       |   2 +
>   include/linux/mtd/spi-nor.h   |   1 +
>   include/uapi/mtd/mtd-abi.h    |   1 +
>   6 files changed, 97 insertions(+), 40 deletions(-)
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 0/3] spi-nor block protection

[Tudor.Ambarus]

On 02/14/2019 11:21 AM, Jonas Bonn wrote:
> Hi,

Hi,

> 
> Just a gentle reminder that I'd like some feedback on these patches.

yep, I'm going to review them these days, I still have few in my queue.

Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Tudor.Ambarus]

+ James and Yong from Cypress.

Hi, Jonas, James, Yong,

On 01/30/2019 12:07 AM, Jonas Bonn wrote:
> The status register bit SRWD (status register write disable) is
> described in many words in the datasheets but effectively boils down to:
> 
> i) if set, respect WP# when trying to change protection bits;
> ii) if unset, ignore WP# when trying to change protection bits
> 
> In short, the bit determines whether the WP# signal is honored or not.
> 
> It's difficult to imagine the use-case where the WP# is connected and
> asserted but the user doesn't want to respect its setting.  As such,
> this patch sets the SRWD bit unconditionally so that the WP# is _always_
> respected; hardware that doesn't care about WP# normally won't even have
> it connected.
> 

Always setting SRWD to 1, dismisses the need of a SRWD bit in the first place.
Do you know why Cypress made this bit configurable and didn't enable it by default?

Cheers,
ta

> Tested on a Cypress s25fl512s.  With this patch, the WP# is always
> respected, irregardless of whether any flash protection bits are set.
> 
> Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
> CC: Marek Vasut <marek.vasut@gmail.com>
> CC: David Woodhouse <dwmw2@infradead.org>
> CC: Brian Norris <computersforpeace@gmail.com>
> CC: Boris Brezillon <bbrezillon@kernel.org>
> CC: Richard Weinberger <richard@nod.at>
> CC: linux-mtd@lists.infradead.org
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 13a5055e5f3f..4c8ce2b90838 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1231,9 +1231,6 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  
>  	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;
>  
> @@ -1313,10 +1310,6 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  
>  	status_new = (status_old & ~mask & ~SR_TB) | val;
>  
> -	/* Don't protect status register if we're fully unlocked */
> -	if (lock_len == 0)
> -		status_new &= ~SR_SRWD;
> -
>  	if (!use_top)
>  		status_new |= SR_TB;
>  
> @@ -3898,6 +3891,7 @@ static int spi_nor_setup(struct spi_nor *nor,
>  static int spi_nor_init(struct spi_nor *nor)
>  {
>  	int err;
> +	int sr;
>  
>  	/*
>  	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
> @@ -3933,7 +3927,14 @@ static int spi_nor_init(struct spi_nor *nor)
>  		set_4byte(nor, true);
>  	}
>  
> -	return 0;
> +	/* Always respect the WP# (write-protect) input */
> +	sr = read_sr(nor);
> +	if (sr < 0) {
> +		dev_err(nor->dev, "error while reading status register\n");
> +		return -EINVAL;
> +	}
> +	sr |= SR_SRWD;
> +	return write_sr_and_check(nor, sr, SR_SRWD);
>  }
>  
>  /* mtd resume handler */
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 3/3] mtd: spi-nor: allow setting the BPNV (default locked) bit

[Tudor.Ambarus]

Hi, Jonas,

On 01/30/2019 12:07 AM, Jonas Bonn wrote:
> Setting the BPNV bit defaults the block protection bits BP0-2 to 1 at
> reset.  This means that all the flash sectors are protected until they
> are explicity unlocked by the user.
> 
> Together with protection of the status register via the SRWD bit and the
> WP# signal, this allows a flash device to be effectively protected from
> erasure by unauthorized actors.
> 
> The patch has been tested with a Cypress s25fl512s and works as
> advertised.  The concern that I have with this, though, is that the BPNV
> bit is "write once":  if somebody inadvertently sets this feature on the
> device, it is irrevocable!  Whether or not this actually belongs in the
> mainline kernel is therefore up for debate...

My opinion is that OTP bits should be set at manufacturer sites or in a client's
controlled environment. I wouldn't add this in kernel. But maybe Marek or Boris
think otherwise?

Cheers,
ta

> 
> Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
> CC: Marek Vasut <marek.vasut@gmail.com>
> CC: David Woodhouse <dwmw2@infradead.org>
> CC: Brian Norris <computersforpeace@gmail.com>
> CC: Boris Brezillon <bbrezillon@kernel.org>
> CC: Richard Weinberger <richard@nod.at>
> CC: linux-mtd@lists.infradead.org
> ---
>  drivers/mtd/mtdchar.c         |   6 ++
>  drivers/mtd/mtdcore.c         |   8 +++
>  drivers/mtd/spi-nor/spi-nor.c | 102 +++++++++++++++++++++++-----------
>  include/linux/mtd/mtd.h       |   2 +
>  include/linux/mtd/spi-nor.h   |   1 +
>  include/uapi/mtd/mtd-abi.h    |   1 +
>  6 files changed, 88 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index affb0ac39928..31dd308d34b1 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -803,6 +803,12 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg)
>  		break;
>  	}
>  
> +	case MEMSETDEFAULTLOCKED:
> +	{
> +		ret = mtd_set_default_locked(mtd);
> +		break;
> +	}
> +
>  	case MEMLOCK:
>  	{
>  		struct erase_info_user einfo;
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 254c3b413213..3cae22f161cb 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1759,6 +1759,14 @@ int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len)
>  }
>  EXPORT_SYMBOL_GPL(mtd_lock_user_prot_reg);
>  
> +int mtd_set_default_locked(struct mtd_info *mtd)
> +{
> +	if (!mtd->_set_default_locked)
> +		return -EOPNOTSUPP;
> +	return mtd->_set_default_locked(mtd);
> +}
> +EXPORT_SYMBOL_GPL(mtd_set_default_locked);
> +
>  /* Chip-supported device locking */
>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d6840c2b15dd..4f9b82ff87ce 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -250,7 +250,7 @@ struct flash_info {
>  	u16		page_size;
>  	u16		addr_width;
>  
> -	u16		flags;
> +	u32		flags;
>  #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 */
> @@ -279,6 +279,10 @@ struct flash_info {
>  #define SPI_NOR_SKIP_SFDP	BIT(13)	/* Skip parsing of SFDP tables */
>  #define USE_CLSR		BIT(14)	/* use CLSR command */
>  #define SPI_NOR_OCTAL_READ	BIT(15)	/* Flash supports Octal Read */
> +#define SPI_NOR_HAS_BPNV	BIT(16) /* Block protection bits can be set
> +					 * non-volatile, meaning all blocks
> +					 * are protected by default at reset
> +					 */
>  
>  	/* Part specific fixup hooks. */
>  	const struct spi_nor_fixups *fixups;
> @@ -1324,6 +1328,36 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	return write_sr_and_check(nor, status_new, mask);
>  }
>  
> +/*
> + * Write status Register and configuration register with 2 bytes
> + * The first byte will be written to the status register, while the
> + * second byte will be written to the configuration register.
> + * Return negative if error occurred.
> + */
> +static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr)
> +{
> +	int ret;
> +
> +	write_enable(nor);
> +
> +	ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2);
> +	if (ret < 0) {
> +		dev_err(nor->dev,
> +			"error while writing configuration register\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = spi_nor_wait_till_ready(nor);
> +	if (ret) {
> +		dev_err(nor->dev,
> +			"timeout while writing configuration register\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +
>  /*
>   * Check if a region of the flash is (completely) locked. See stm_lock() for
>   * more info.
> @@ -1342,6 +1376,35 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	return stm_is_locked_sr(nor, ofs, len, status);
>  }
>  
> +static int spi_nor_set_default_locked(struct mtd_info *mtd)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	struct device *dev = nor->dev;
> +	u8 sr_cr[2];
> +	int ret;
> +
> +	ret = read_cr(nor);
> +	if (ret < 0) {
> +		dev_err(dev, "error while reading configuration register\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ret & CR_BPNV)
> +		return 0;
> +
> +	sr_cr[1] = ret | CR_BPNV;
> +
> +	/* Keep the current value of the Status Register. */
> +	ret = read_sr(nor);
> +	if (ret < 0) {
> +		dev_err(dev, "error while reading status register\n");
> +		return -EINVAL;
> +	}
> +	sr_cr[0] = ret;
> +
> +	return write_sr_cr(nor, sr_cr);
> +}
> +
>  static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> @@ -1387,35 +1450,6 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  	return ret;
>  }
>  
> -/*
> - * Write status Register and configuration register with 2 bytes
> - * The first byte will be written to the status register, while the
> - * second byte will be written to the configuration register.
> - * Return negative if error occurred.
> - */
> -static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr)
> -{
> -	int ret;
> -
> -	write_enable(nor);
> -
> -	ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2);
> -	if (ret < 0) {
> -		dev_err(nor->dev,
> -			"error while writing configuration register\n");
> -		return -EINVAL;
> -	}
> -
> -	ret = spi_nor_wait_till_ready(nor);
> -	if (ret) {
> -		dev_err(nor->dev,
> -			"timeout while writing configuration register\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  /**
>   * macronix_quad_enable() - set QE bit in Status Register.
>   * @nor:	pointer to a 'struct spi_nor'
> @@ -1886,8 +1920,8 @@ static const struct flash_info spi_nor_ids[] = {
>  	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>  	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
>  	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> -	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) },
> -	{ "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) },
> +	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_HAS_BPNV | USE_CLSR) },
> +	{ "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_HAS_BPNV | USE_CLSR) },
>  	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>  	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>  	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
> @@ -4066,6 +4100,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  		mtd->_is_locked = spi_nor_is_locked;
>  	}
>  
> +	if (info->flags & SPI_NOR_HAS_BPNV) {
> +		mtd->_set_default_locked = spi_nor_set_default_locked;
> +	}
> +
>  	/* sst nor chips use AAI word program */
>  	if (info->flags & SST_WRITE)
>  		mtd->_write = sst_write;
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index b78e2c1b27b1..a07468b6677d 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -346,6 +346,7 @@ struct mtd_info {
>  	int (*_writev) (struct mtd_info *mtd, const struct kvec *vecs,
>  			unsigned long count, loff_t to, size_t *retlen);
>  	void (*_sync) (struct mtd_info *mtd);
> +	int (*_set_default_locked) (struct mtd_info *mtd);
>  	int (*_lock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  	int (*_unlock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  	int (*_is_locked) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
> @@ -535,6 +536,7 @@ static inline void mtd_sync(struct mtd_info *mtd)
>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  int mtd_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len);
> +int mtd_set_default_locked(struct mtd_info *mtd);
>  int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs);
>  int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs);
>  int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs);
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 2353af8bac99..c7cc84a5c8f6 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -144,6 +144,7 @@
>  #define FSR_PT_ERR		BIT(1)	/* Protection error bit */
>  
>  /* Configuration Register bits. */
> +#define CR_BPNV			BIT(3)	/* Block protection non-volatile */
>  #define CR_QUAD_EN_SPAN		BIT(1)	/* Spansion Quad I/O */
>  
>  /* Status Register 2 bits. */
> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
> index aff5b5e59845..27b752796efa 100644
> --- a/include/uapi/mtd/mtd-abi.h
> +++ b/include/uapi/mtd/mtd-abi.h
> @@ -204,6 +204,7 @@ struct otp_info {
>   * without OOB, e.g., NOR flash.
>   */
>  #define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
> +#define MEMSETDEFAULTLOCKED	_IO('M', 25)
>  
>  /*
>   * Obsolete legacy interface. Keep it in order not to break userspace
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Jonas Bonn]

Hi,

On 10/03/2019 10:58, Tudor.Ambarus@microchip.com wrote:
> + James and Yong from Cypress.
> 
> Hi, Jonas, James, Yong,
> 
> On 01/30/2019 12:07 AM, Jonas Bonn wrote:
>> The status register bit SRWD (status register write disable) is
>> described in many words in the datasheets but effectively boils down to:
>>
>> i) if set, respect WP# when trying to change protection bits;
>> ii) if unset, ignore WP# when trying to change protection bits
>>
>> In short, the bit determines whether the WP# signal is honored or not.
>>
>> It's difficult to imagine the use-case where the WP# is connected and
>> asserted but the user doesn't want to respect its setting.  As such,
>> this patch sets the SRWD bit unconditionally so that the WP# is _always_
>> respected; hardware that doesn't care about WP# normally won't even have
>> it connected.
>>
> 
> Always setting SRWD to 1, dismisses the need of a SRWD bit in the first place.
> Do you know why Cypress made this bit configurable and didn't enable it by default?

I suspect it has some historical relevance; as things currently stand, I 
don't see how it's useful in its current form.

We initially thought that the WP# signal turned on/off block protection 
but that's not the case; the block protection bits BP0-BP2 turn on/off 
block protection and the WP# signal only protects the BP[0-2] bits from 
being modified.  Turning off the WP# functionality via the SRWD bits 
means that the BP[0-2] bits are always modifiable and therefore the 
flash device is never really protected from writes by a malicious actor.

The key question here is:  if the WP# signal is connected, why would you 
ever want its state to be ignored by the device?  De-asserting the 
signal has the same effect as setting the SRWD bit.  And the default 
state is un-asserted if the signal is not connected, so that case is 
covered, too.  I see no reason not to just always set SRWD and thereby 
make the WP# signal do what one expects of it, always.

/Jonas


> 
> Cheers,
> ta
> 
>> Tested on a Cypress s25fl512s.  With this patch, the WP# is always
>> respected, irregardless of whether any flash protection bits are set.
>>
>> Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
>> CC: Marek Vasut <marek.vasut@gmail.com>
>> CC: David Woodhouse <dwmw2@infradead.org>
>> CC: Brian Norris <computersforpeace@gmail.com>
>> CC: Boris Brezillon <bbrezillon@kernel.org>
>> CC: Richard Weinberger <richard@nod.at>
>> CC: linux-mtd@lists.infradead.org
>> ---
>>   drivers/mtd/spi-nor/spi-nor.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 13a5055e5f3f..4c8ce2b90838 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1231,9 +1231,6 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>>   
>>   	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;
>>   
>> @@ -1313,10 +1310,6 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>>   
>>   	status_new = (status_old & ~mask & ~SR_TB) | val;
>>   
>> -	/* Don't protect status register if we're fully unlocked */
>> -	if (lock_len == 0)
>> -		status_new &= ~SR_SRWD;
>> -
>>   	if (!use_top)
>>   		status_new |= SR_TB;
>>   
>> @@ -3898,6 +3891,7 @@ static int spi_nor_setup(struct spi_nor *nor,
>>   static int spi_nor_init(struct spi_nor *nor)
>>   {
>>   	int err;
>> +	int sr;
>>   
>>   	/*
>>   	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
>> @@ -3933,7 +3927,14 @@ static int spi_nor_init(struct spi_nor *nor)
>>   		set_4byte(nor, true);
>>   	}
>>   
>> -	return 0;
>> +	/* Always respect the WP# (write-protect) input */
>> +	sr = read_sr(nor);
>> +	if (sr < 0) {
>> +		dev_err(nor->dev, "error while reading status register\n");
>> +		return -EINVAL;
>> +	}
>> +	sr |= SR_SRWD;
>> +	return write_sr_and_check(nor, sr, SR_SRWD);
>>   }
>>   
>>   /* mtd resume handler */
>>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Tudor.Ambarus]

Hi, Jonas,

On 03/10/2019 12:42 PM, Jonas Bonn wrote:
> Hi,
> 
> On 10/03/2019 10:58, Tudor.Ambarus@microchip.com wrote:
>> + James and Yong from Cypress.
>>
>> Hi, Jonas, James, Yong,
>>
>> On 01/30/2019 12:07 AM, Jonas Bonn wrote:
>>> The status register bit SRWD (status register write disable) is
>>> described in many words in the datasheets but effectively boils down to:
>>>
>>> i) if set, respect WP# when trying to change protection bits;
>>> ii) if unset, ignore WP# when trying to change protection bits
>>>
>>> In short, the bit determines whether the WP# signal is honored or not.
>>>
>>> It's difficult to imagine the use-case where the WP# is connected and
>>> asserted but the user doesn't want to respect its setting.  As such,
>>> this patch sets the SRWD bit unconditionally so that the WP# is _always_
>>> respected; hardware that doesn't care about WP# normally won't even have
>>> it connected.
>>>
>>
>> Always setting SRWD to 1, dismisses the need of a SRWD bit in the first place.
>> Do you know why Cypress made this bit configurable and didn't enable it by default?
> 
> I suspect it has some historical relevance; as things currently stand, I don't see how it's useful in its current form.
> 
> We initially thought that the WP# signal turned on/off block protection but that's not the case; the block protection bits BP0-BP2 turn on/off block protection and the WP# signal only protects the BP[0-2] bits from being modified.  Turning off the WP# functionality via the SRWD bits means that the BP[0-2] bits are always modifiable and therefore the flash device is never really protected from writes by a malicious actor.
> 
> The key question here is:  if the WP# signal is connected, why would you ever want its state to be ignored by the device?  De-asserting the signal has the same effect as setting the SRWD bit.  And the default state is un-asserted if the signal is not connected, so that case is covered, too.  I see no reason not to just always set SRWD and thereby make the WP# signal do what one expects of it, always.
> 

I tend to agree with you. Let's wait for a few days, maybe James or Yong will
tell us the Cypress's reasons of making SRWD configurable. In the meantime I'll
check the datasheets of the flashes that have SPI_NOR_HAS_LOCK declared, see if
SRWD is configurable and what were the reasons of making it so.

Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Yong Qin]

Hello all,

SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.

By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).

If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.

Flash data array can be protected by DYB, or PPB, or BP0-2 and TBPROT, or combination of these three methods. These 3 sector protection mechanisms and combinations provide flexibility (different security level) of data protection. If a sector is protected by any of DYB, or PPB, or BP bit, then it is not programmable or erasable. If a sector has none of DYB, PPB, BP bit set, then it is not protected and is programmable and erasable.

DYB is volatile bit. It can be easily and fast modified on the fly, and will be set to default value upon power cycle or hardware reset. So it provides least security level protection, normally is used to prevent unintended program/erase commands. It also does not have wear limitation (i.e., unlimited times of alternations). One example of the use case is that set DYB bits to protect all the data array sectors right after system powers on. Whenever program or erase the flash data array, clear the DYB bit (set to unprotect) for the target sector prior to sending program/erase command. After program/erase complete, set the relevant DYB bit to protect the sector again.

PPB is non-volatile bit. It is a little difficult to change than DYB. It provides medium security protection level. PPB bits have the same erase/program lifecycles as the flash data array.

BP0-2 and TBPROT provide another level of security protection.

SRWD bit and WP# can prevent BP0-2 and TBPROT bits to be modified, which can provide one more level of security protection.

Best Regards,
Yong

-----Original Message-----
From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
Sent: Monday, March 11, 2019 10:06 AM
To: jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com>; Yong Qin <Yong.Qin@cypress.com>
Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org
Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

Hi, Jonas,

On 03/10/2019 12:42 PM, Jonas Bonn wrote:
> Hi,
>
> On 10/03/2019 10:58, Tudor.Ambarus@microchip.com wrote:
>> + James and Yong from Cypress.
>>
>> Hi, Jonas, James, Yong,
>>
>> On 01/30/2019 12:07 AM, Jonas Bonn wrote:
>>> The status register bit SRWD (status register write disable) is
>>> described in many words in the datasheets but effectively boils down to:
>>>
>>> i) if set, respect WP# when trying to change protection bits;
>>> ii) if unset, ignore WP# when trying to change protection bits
>>>
>>> In short, the bit determines whether the WP# signal is honored or not.
>>>
>>> It's difficult to imagine the use-case where the WP# is connected
>>> and asserted but the user doesn't want to respect its setting.  As
>>> such, this patch sets the SRWD bit unconditionally so that the WP#
>>> is _always_ respected; hardware that doesn't care about WP# normally
>>> won't even have it connected.
>>>
>>
>> Always setting SRWD to 1, dismisses the need of a SRWD bit in the first place.
>> Do you know why Cypress made this bit configurable and didn't enable it by default?
>
> I suspect it has some historical relevance; as things currently stand, I don't see how it's useful in its current form.
>
> We initially thought that the WP# signal turned on/off block protection but that's not the case; the block protection bits BP0-BP2 turn on/off block protection and the WP# signal only protects the BP[0-2] bits from being modified.  Turning off the WP# functionality via the SRWD bits means that the BP[0-2] bits are always modifiable and therefore the flash device is never really protected from writes by a malicious actor.
>
> The key question here is:  if the WP# signal is connected, why would you ever want its state to be ignored by the device?  De-asserting the signal has the same effect as setting the SRWD bit.  And the default state is un-asserted if the signal is not connected, so that case is covered, too.  I see no reason not to just always set SRWD and thereby make the WP# signal do what one expects of it, always.
>

I tend to agree with you. Let's wait for a few days, maybe James or Yong will tell us the Cypress's reasons of making SRWD configurable. In the meantime I'll check the datasheets of the flashes that have SPI_NOR_HAS_LOCK declared, see if SRWD is configurable and what were the reasons of making it so.

Cheers,
ta

This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Tudor.Ambarus]

Hi, Yong,

Thank you for the explanation. There are still few things to clarify.

On 03/11/2019 10:14 PM, Yong Qin wrote:
> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
> 
> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
> 
> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.

Does the SRWD bit protect the Status and Configuration Register bits even when
in Quad Mode? WP# function is not available in Quad mode. How can one release
this protection when in Quad Mode and SRWD set to 1?

If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and
Configuration Register bits protection by default? I.e., remove SRWD bit from
SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits
protection enabled by default when not in Quad Mode.

Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Yong Qin]

Hi Tudor,

Good question.

The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected.

Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature.

Thanks,
Yong

-----Original Message-----
From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
Sent: Tuesday, March 12, 2019 5:30 AM
To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com>
Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org
Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

Hi, Yong,

Thank you for the explanation. There are still few things to clarify.

On 03/11/2019 10:14 PM, Yong Qin wrote:
> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
>
> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
>
> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.

Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1?

If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode.

Cheers,
ta

This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Tudor.Ambarus]

Jonas, Yong,

On 03/12/2019 09:27 PM, Yong Qin wrote:
> Hi Tudor,
> 
> Good question.
> 
> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected.
> 
> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature.

I think I found the reason why SRWD bit is configurable, and disabled by
default: => to allow the installation of the flash in a system with a grounded
WP# pin while still enabling write to the BP bits.

Jonas, Yong, what do you think?

Cheers,
ta

> 
> Thanks,
> Yong
> 
> -----Original Message-----
> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
> Sent: Tuesday, March 12, 2019 5:30 AM
> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com>
> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org
> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input
> 
> Hi, Yong,
> 
> Thank you for the explanation. There are still few things to clarify.
> 
> On 03/11/2019 10:14 PM, Yong Qin wrote:
>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
>>
>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
>>
>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.
> 
> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1?
> 
> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode.
> 
> Cheers,
> ta
> 
> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Jonas Bonn]

On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote:
> Jonas, Yong,
> 
> On 03/12/2019 09:27 PM, Yong Qin wrote:
>> Hi Tudor,
>>
>> Good question.
>>
>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected.
>>
>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature.
> 
> I think I found the reason why SRWD bit is configurable, and disabled by
> default: => to allow the installation of the flash in a system with a grounded
> WP# pin while still enabling write to the BP bits.

I think this is bogus.  Why would you ground the SRWD pin?  That's a 
design error.

/Jonas

> 
> Jonas, Yong, what do you think?
> 
> Cheers,
> ta
> 
>>
>> Thanks,
>> Yong
>>
>> -----Original Message-----
>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
>> Sent: Tuesday, March 12, 2019 5:30 AM
>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com>
>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org
>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input
>>
>> Hi, Yong,
>>
>> Thank you for the explanation. There are still few things to clarify.
>>
>> On 03/11/2019 10:14 PM, Yong Qin wrote:
>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
>>>
>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
>>>
>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.
>>
>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1?
>>
>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode.
>>
>> Cheers,
>> ta
>>
>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
>>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/3] mtd: spi-nor: s25fl512s supports region locking

[Tudor.Ambarus]

Jonas,

On 01/30/2019 12:07 AM, Jonas Bonn wrote:
> Both the BP0-2 bits and the TBPROT bit are supported on this chip.

It's always comfortable to see in the commit message that the change was tested.
Manufacturers can mistake and advertise things that they don't support :).

> 
> Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
> CC: Marek Vasut <marek.vasut@gmail.com>
> CC: David Woodhouse <dwmw2@infradead.org>
> CC: Brian Norris <computersforpeace@gmail.com>
> CC: Boris Brezillon <bbrezillon@kernel.org>
> CC: Richard Weinberger <richard@nod.at>
> CC: linux-mtd@lists.infradead.org

The list is quite large. I usually use git send-email --cc=someone@something to
cc people. And CC: explicitly in the commit message when I want the change to be
included in a stable kernel.

> ---
>  drivers/mtd/spi-nor/spi-nor.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 4c8ce2b90838..d6840c2b15dd 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1886,8 +1886,8 @@ static const struct flash_info spi_nor_ids[] = {
>  	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>  	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
>  	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> -	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> -	{ "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> +	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) },
> +	{ "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | USE_CLSR) },

Would you please get rid of the 80 char checkpatch warnings? Please use the
following format, it has fewer lines:

        { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512,
                         SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)

Thanks!
ta

>  	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>  	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>  	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Tudor.Ambarus]

Jonas,

On 03/19/2019 09:13 AM, Jonas Bonn wrote:
> 
> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote:
>> Jonas, Yong,
>>
>> On 03/12/2019 09:27 PM, Yong Qin wrote:
>>> Hi Tudor,
>>>
>>> Good question.
>>>
>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected.
>>>
>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature.
>>
>> I think I found the reason why SRWD bit is configurable, and disabled by
>> default: => to allow the installation of the flash in a system with a grounded
>> WP# pin while still enabling write to the BP bits.
> 
> I think this is bogus.  Why would you ground the SRWD pin?  That's a design error.

I've talked with Microchip hardware team, we have this feature on sst26 flashes too.

Grounding WP would not necessarily be a design error. The intention is that some
users might choose to populate the Flash chip onto their PCB, program the memory
in-circuit, and then lock down the write protection to use the chip like a ROM.
Grounding WP allows them to do this. Since SRWD is set to '0' from the factory,
so users can program the memory, set the block protection (BP) bits to protect
the memory, and then set the SRWD bit to enable the grounded WP input and
prevent changing the BP bits.

The grounded WP pin + SRWD bit method is an older, legacy approach to the
problem. We don't know how many users actually ground the WP pin in this manner,
but there are probably some users out there who do.

Cheers,
ta

> 
> /Jonas
> 
>>
>> Jonas, Yong, what do you think?
>>
>> Cheers,
>> ta
>>
>>>
>>> Thanks,
>>> Yong
>>>
>>> -----Original Message-----
>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
>>> Sent: Tuesday, March 12, 2019 5:30 AM
>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com>
>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org
>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input
>>>
>>> Hi, Yong,
>>>
>>> Thank you for the explanation. There are still few things to clarify.
>>>
>>> On 03/11/2019 10:14 PM, Yong Qin wrote:
>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
>>>>
>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
>>>>
>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.
>>>
>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1?
>>>
>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode.
>>>
>>> Cheers,
>>> ta
>>>
>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
>>>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Jonas Bonn]

On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote:
> Jonas,
> 
> On 03/19/2019 09:13 AM, Jonas Bonn wrote:
>>
>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote:
>>> Jonas, Yong,
>>>
>>> On 03/12/2019 09:27 PM, Yong Qin wrote:
>>>> Hi Tudor,
>>>>
>>>> Good question.
>>>>
>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected.
>>>>
>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature.
>>>
>>> I think I found the reason why SRWD bit is configurable, and disabled by
>>> default: => to allow the installation of the flash in a system with a grounded
>>> WP# pin while still enabling write to the BP bits.
>>
>> I think this is bogus.  Why would you ground the SRWD pin?  That's a design error.
> 
> I've talked with Microchip hardware team, we have this feature on sst26 flashes too.
> 
> Grounding WP would not necessarily be a design error. The intention is that some
> users might choose to populate the Flash chip onto their PCB, program the memory
> in-circuit, and then lock down the write protection to use the chip like a ROM.
> Grounding WP allows them to do this. Since SRWD is set to '0' from the factory,
> so users can program the memory, set the block protection (BP) bits to protect
> the memory, and then set the SRWD bit to enable the grounded WP input and
> prevent changing the BP bits.

Again, this is bogus.  The SRWD bit is non-volatile so just resetting 
the flash clears the bit and the BP bits can be changed.  This does not 
magically turn the flash into a ROM.  Grounded or not, the WP# is NOT 
respected until the SRWD bit is set; what you've described is yet 
another reason to default the SRWD to set.

With the BPNV bit (see patch 3 that I posted in the v2 series), at least 
the flash can be made to start up write-protected; however, there is 
nothing preventing modification of the PROTection bits until SRWD is set 
AND WP# is asserted (through grounding or otherwise).  Linux currently 
defaults SRWD to cleared which is "unprotected", irregardless of the 
state of WP#.

> 
> The grounded WP pin + SRWD bit method is an older, legacy approach to the
> problem. We don't know how many users actually ground the WP pin in this manner,
> but there are probably some users out there who do.

If they do so, they are fooling themselves... or they are running a 
patched kernel! :)

/Jonas




> 
> Cheers,
> ta
> 
>>
>> /Jonas
>>
>>>
>>> Jonas, Yong, what do you think?
>>>
>>> Cheers,
>>> ta
>>>
>>>>
>>>> Thanks,
>>>> Yong
>>>>
>>>> -----Original Message-----
>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
>>>> Sent: Tuesday, March 12, 2019 5:30 AM
>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com>
>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org
>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input
>>>>
>>>> Hi, Yong,
>>>>
>>>> Thank you for the explanation. There are still few things to clarify.
>>>>
>>>> On 03/11/2019 10:14 PM, Yong Qin wrote:
>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
>>>>>
>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
>>>>>
>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.
>>>>
>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1?
>>>>
>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode.
>>>>
>>>> Cheers,
>>>> ta
>>>>
>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
>>>>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Tudor.Ambarus]

On 03/20/2019 09:06 AM, Jonas Bonn wrote:
> External E-Mail
> 
> 
> 
> On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote:
>> Jonas,
>>
>> On 03/19/2019 09:13 AM, Jonas Bonn wrote:
>>>
>>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote:
>>>> Jonas, Yong,
>>>>
>>>> On 03/12/2019 09:27 PM, Yong Qin wrote:
>>>>> Hi Tudor,
>>>>>
>>>>> Good question.
>>>>>
>>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected.
>>>>>
>>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature.
>>>>
>>>> I think I found the reason why SRWD bit is configurable, and disabled by
>>>> default: => to allow the installation of the flash in a system with a grounded
>>>> WP# pin while still enabling write to the BP bits.
>>>
>>> I think this is bogus.  Why would you ground the SRWD pin?  That's a design error.
>>
>> I've talked with Microchip hardware team, we have this feature on sst26 flashes too.
>>
>> Grounding WP would not necessarily be a design error. The intention is that some
>> users might choose to populate the Flash chip onto their PCB, program the memory
>> in-circuit, and then lock down the write protection to use the chip like a ROM.
>> Grounding WP allows them to do this. Since SRWD is set to '0' from the factory,
>> so users can program the memory, set the block protection (BP) bits to protect
>> the memory, and then set the SRWD bit to enable the grounded WP input and
>> prevent changing the BP bits.
> 
> Again, this is bogus.  The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed.  This does not magically turn the flash into a ROM.  Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set.

SRWD is a non-volatile bit: default at power-up will be the setting prior to
power-down.

Cheers,
ta

> 
> With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise).  Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#.
> 
>>
>> The grounded WP pin + SRWD bit method is an older, legacy approach to the
>> problem. We don't know how many users actually ground the WP pin in this manner,
>> but there are probably some users out there who do.
> 
> If they do so, they are fooling themselves... or they are running a patched kernel! :)
> 
> /Jonas
> 
> 
> 
> 
>>
>> Cheers,
>> ta
>>
>>>
>>> /Jonas
>>>
>>>>
>>>> Jonas, Yong, what do you think?
>>>>
>>>> Cheers,
>>>> ta
>>>>
>>>>>
>>>>> Thanks,
>>>>> Yong
>>>>>
>>>>> -----Original Message-----
>>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
>>>>> Sent: Tuesday, March 12, 2019 5:30 AM
>>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com>
>>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org
>>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input
>>>>>
>>>>> Hi, Yong,
>>>>>
>>>>> Thank you for the explanation. There are still few things to clarify.
>>>>>
>>>>> On 03/11/2019 10:14 PM, Yong Qin wrote:
>>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
>>>>>>
>>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
>>>>>>
>>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.
>>>>>
>>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1?
>>>>>
>>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode.
>>>>>
>>>>> Cheers,
>>>>> ta
>>>>>
>>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
>>>>>
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Jonas Bonn]

On 20/03/2019 08:33, Tudor.Ambarus@microchip.com wrote:
> 
> 
> On 03/20/2019 09:06 AM, Jonas Bonn wrote:
>> External E-Mail
>>
>>
>>
>> On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote:
>>> Jonas,
>>>
>>> On 03/19/2019 09:13 AM, Jonas Bonn wrote:
>>>>
>>>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote:
>>>>> Jonas, Yong,
>>>>>
>>>>> On 03/12/2019 09:27 PM, Yong Qin wrote:
>>>>>> Hi Tudor,
>>>>>>
>>>>>> Good question.
>>>>>>
>>>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected.
>>>>>>
>>>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature.
>>>>>
>>>>> I think I found the reason why SRWD bit is configurable, and disabled by
>>>>> default: => to allow the installation of the flash in a system with a grounded
>>>>> WP# pin while still enabling write to the BP bits.
>>>>
>>>> I think this is bogus.  Why would you ground the SRWD pin?  That's a design error.
>>>
>>> I've talked with Microchip hardware team, we have this feature on sst26 flashes too.
>>>
>>> Grounding WP would not necessarily be a design error. The intention is that some
>>> users might choose to populate the Flash chip onto their PCB, program the memory
>>> in-circuit, and then lock down the write protection to use the chip like a ROM.
>>> Grounding WP allows them to do this. Since SRWD is set to '0' from the factory,
>>> so users can program the memory, set the block protection (BP) bits to protect
>>> the memory, and then set the SRWD bit to enable the grounded WP input and
>>> prevent changing the BP bits.
>>
>> Again, this is bogus.  The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed.  This does not magically turn the flash into a ROM.  Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set.
> 
> SRWD is a non-volatile bit: default at power-up will be the setting prior to
> power-down.

Ah, right you are! :)

By grounding the WP# pin, you are effectively turning the SRWD bit into 
an OTP bit.  Note that what you described above also requires setting 
the BPNV bit, otherwise protection is irrevocably reset to "unprotected" 
at reset.  The BPNV bit cannot be set from Linux so whoever is using the 
"grounded pin" method must be programming the board in-factory from 
another system than Linux.  Therefore it's OK for Linux to default SRWD 
to asserted, irregardless of the state of WP#.

/Jonas



> 
> Cheers,
> ta
> 
>>
>> With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise).  Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#.
>>
>>>
>>> The grounded WP pin + SRWD bit method is an older, legacy approach to the
>>> problem. We don't know how many users actually ground the WP pin in this manner,
>>> but there are probably some users out there who do.
>>
>> If they do so, they are fooling themselves... or they are running a patched kernel! :)
>>
>> /Jonas
>>
>>
>>
>>
>>>
>>> Cheers,
>>> ta
>>>
>>>>
>>>> /Jonas
>>>>
>>>>>
>>>>> Jonas, Yong, what do you think?
>>>>>
>>>>> Cheers,
>>>>> ta
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Yong
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
>>>>>> Sent: Tuesday, March 12, 2019 5:30 AM
>>>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James Tomasetta <James.Tomasetta@cypress.com>
>>>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; computersforpeace@gmail.com; dwmw2@infradead.org
>>>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input
>>>>>>
>>>>>> Hi, Yong,
>>>>>>
>>>>>> Thank you for the explanation. There are still few things to clarify.
>>>>>>
>>>>>> On 03/11/2019 10:14 PM, Yong Qin wrote:
>>>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
>>>>>>>
>>>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
>>>>>>>
>>>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.
>>>>>>
>>>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1?
>>>>>>
>>>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode.
>>>>>>
>>>>>> Cheers,
>>>>>> ta
>>>>>>
>>>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
>>>>>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Yong Qin]

Hi Jonas, Tudor,

I think Tudor described is a reasonable use case scenario.

BPNV bit default value is 0, which means BP0-2 bits are default Non-Volatile. In this case, BPNV bit does not need to be set. After the flash chip mounted on PCB board with WP# grounded, all need to do are: Program data into flash --> Set BP0-2 to 1 --> Set SRWD to 1, then both SRWD bit and BP0-2 bits are locked-down, unless disconnecting WP# from grounding. Reset will not change SRWD bit and BP0-2 bits since they all are Non-Volatile.

Best Regards,
Yong

-----Original Message-----
From: Jonas Bonn <jonas@norrbonn.se>
Sent: Wednesday, March 20, 2019 3:40 AM
To: Tudor.Ambarus@microchip.com; Yong Qin <Yong.Qin@cypress.com>; James Tomasetta <James.Tomasetta@cypress.com>
Cc: bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; linux-mtd@lists.infradead.org; computersforpeace@gmail.com; dwmw2@infradead.org
Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input



On 20/03/2019 08:33, Tudor.Ambarus@microchip.com wrote:
>
>
> On 03/20/2019 09:06 AM, Jonas Bonn wrote:
>> External E-Mail
>>
>>
>>
>> On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote:
>>> Jonas,
>>>
>>> On 03/19/2019 09:13 AM, Jonas Bonn wrote:
>>>>
>>>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote:
>>>>> Jonas, Yong,
>>>>>
>>>>> On 03/12/2019 09:27 PM, Yong Qin wrote:
>>>>>> Hi Tudor,
>>>>>>
>>>>>> Good question.
>>>>>>
>>>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected.
>>>>>>
>>>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature.
>>>>>
>>>>> I think I found the reason why SRWD bit is configurable, and
>>>>> disabled by
>>>>> default: => to allow the installation of the flash in a system
>>>>> with a grounded WP# pin while still enabling write to the BP bits.
>>>>
>>>> I think this is bogus.  Why would you ground the SRWD pin?  That's a design error.
>>>
>>> I've talked with Microchip hardware team, we have this feature on sst26 flashes too.
>>>
>>> Grounding WP would not necessarily be a design error. The intention
>>> is that some users might choose to populate the Flash chip onto
>>> their PCB, program the memory in-circuit, and then lock down the write protection to use the chip like a ROM.
>>> Grounding WP allows them to do this. Since SRWD is set to '0' from
>>> the factory, so users can program the memory, set the block
>>> protection (BP) bits to protect the memory, and then set the SRWD
>>> bit to enable the grounded WP input and prevent changing the BP bits.
>>
>> Again, this is bogus.  The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed.  This does not magically turn the flash into a ROM.  Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set.
>
> SRWD is a non-volatile bit: default at power-up will be the setting
> prior to power-down.

Ah, right you are! :)

By grounding the WP# pin, you are effectively turning the SRWD bit into an OTP bit.  Note that what you described above also requires setting the BPNV bit, otherwise protection is irrevocably reset to "unprotected"
at reset.  The BPNV bit cannot be set from Linux so whoever is using the "grounded pin" method must be programming the board in-factory from another system than Linux.  Therefore it's OK for Linux to default SRWD to asserted, irregardless of the state of WP#.

/Jonas



>
> Cheers,
> ta
>
>>
>> With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise).  Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#.
>>
>>>
>>> The grounded WP pin + SRWD bit method is an older, legacy approach
>>> to the problem. We don't know how many users actually ground the WP
>>> pin in this manner, but there are probably some users out there who do.
>>
>> If they do so, they are fooling themselves... or they are running a
>> patched kernel! :)
>>
>> /Jonas
>>
>>
>>
>>
>>>
>>> Cheers,
>>> ta
>>>
>>>>
>>>> /Jonas
>>>>
>>>>>
>>>>> Jonas, Yong, what do you think?
>>>>>
>>>>> Cheers,
>>>>> ta
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Yong
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
>>>>>> Sent: Tuesday, March 12, 2019 5:30 AM
>>>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James
>>>>>> Tomasetta <James.Tomasetta@cypress.com>
>>>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org;
>>>>>> richard@nod.at; marek.vasut@gmail.com;
>>>>>> computersforpeace@gmail.com; dwmw2@infradead.org
>>>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect
>>>>>> write-protect input
>>>>>>
>>>>>> Hi, Yong,
>>>>>>
>>>>>> Thank you for the explanation. There are still few things to clarify.
>>>>>>
>>>>>> On 03/11/2019 10:14 PM, Yong Qin wrote:
>>>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
>>>>>>>
>>>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
>>>>>>>
>>>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.
>>>>>>
>>>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1?
>>>>>>
>>>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode.
>>>>>>
>>>>>> Cheers,
>>>>>> ta
>>>>>>
>>>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
>>>>>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/

This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Jonas Bonn]

On 20/03/2019 19:56, Yong Qin wrote:
> Hi Jonas, Tudor,
> 
> I think Tudor described is a reasonable use case scenario.
> 
> BPNV bit default value is 0, which means BP0-2 bits are default Non-Volatile. In this case, BPNV bit does not need to be set. After the flash chip mounted on PCB board with WP# grounded, all need to do are: Program data into flash --> Set BP0-2 to 1 --> Set SRWD to 1, then both SRWD bit and BP0-2 bits are locked-down, unless disconnecting WP# from grounding. Reset will not change SRWD bit and BP0-2 bits since they all are Non-Volatile.

OK, my own use case is causing me to get confused over the volatility of 
these bits.

The use case I have is the following:
a)  The flash must be write-protected at power on
b)  The flash may be made writable by an authorized actor

So the idea is to do this:
i)  Set BPNV to 1 so that BP0-2 get reset to 1 at power on
ii)  WP# is asserted at power on (effectively grounded)
iii)  Authorized actor invokes action to de-assert WP#
iv)  BP0-2 bits can now be modified freely and flash is writable
v)  Authorized actor may invoke action to assert WP# but if that doesn't 
happen and the system gets restarted at least the flash will not be 
writable when it is restarted

Control of the BP bits in Linux is via stm_lock and stm_unlock.  These 
functions also set and clear SRWD.  This breaks the above use case 
because when the device is unlocked, SRWD is cleared, and then device is 
not protected after a power cycle.

Suggestions?  Can we separate the manipulation of SRWD from the 
manipulation of the BP bits without breaking existing users?

/Jonas

> 
> Best Regards,
> Yong
> 
> -----Original Message-----
> From: Jonas Bonn <jonas@norrbonn.se>
> Sent: Wednesday, March 20, 2019 3:40 AM
> To: Tudor.Ambarus@microchip.com; Yong Qin <Yong.Qin@cypress.com>; James Tomasetta <James.Tomasetta@cypress.com>
> Cc: bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; linux-mtd@lists.infradead.org; computersforpeace@gmail.com; dwmw2@infradead.org
> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input
> 
> 
> 
> On 20/03/2019 08:33, Tudor.Ambarus@microchip.com wrote:
>>
>>
>> On 03/20/2019 09:06 AM, Jonas Bonn wrote:
>>> External E-Mail
>>>
>>>
>>>
>>> On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote:
>>>> Jonas,
>>>>
>>>> On 03/19/2019 09:13 AM, Jonas Bonn wrote:
>>>>>
>>>>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote:
>>>>>> Jonas, Yong,
>>>>>>
>>>>>> On 03/12/2019 09:27 PM, Yong Qin wrote:
>>>>>>> Hi Tudor,
>>>>>>>
>>>>>>> Good question.
>>>>>>>
>>>>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected.
>>>>>>>
>>>>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature.
>>>>>>
>>>>>> I think I found the reason why SRWD bit is configurable, and
>>>>>> disabled by
>>>>>> default: => to allow the installation of the flash in a system
>>>>>> with a grounded WP# pin while still enabling write to the BP bits.
>>>>>
>>>>> I think this is bogus.  Why would you ground the SRWD pin?  That's a design error.
>>>>
>>>> I've talked with Microchip hardware team, we have this feature on sst26 flashes too.
>>>>
>>>> Grounding WP would not necessarily be a design error. The intention
>>>> is that some users might choose to populate the Flash chip onto
>>>> their PCB, program the memory in-circuit, and then lock down the write protection to use the chip like a ROM.
>>>> Grounding WP allows them to do this. Since SRWD is set to '0' from
>>>> the factory, so users can program the memory, set the block
>>>> protection (BP) bits to protect the memory, and then set the SRWD
>>>> bit to enable the grounded WP input and prevent changing the BP bits.
>>>
>>> Again, this is bogus.  The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed.  This does not magically turn the flash into a ROM.  Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set.
>>
>> SRWD is a non-volatile bit: default at power-up will be the setting
>> prior to power-down.
> 
> Ah, right you are! :)
> 
> By grounding the WP# pin, you are effectively turning the SRWD bit into an OTP bit.  Note that what you described above also requires setting the BPNV bit, otherwise protection is irrevocably reset to "unprotected"
> at reset.  The BPNV bit cannot be set from Linux so whoever is using the "grounded pin" method must be programming the board in-factory from another system than Linux.  Therefore it's OK for Linux to default SRWD to asserted, irregardless of the state of WP#.
> 
> /Jonas
> 
> 
> 
>>
>> Cheers,
>> ta
>>
>>>
>>> With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise).  Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#.
>>>
>>>>
>>>> The grounded WP pin + SRWD bit method is an older, legacy approach
>>>> to the problem. We don't know how many users actually ground the WP
>>>> pin in this manner, but there are probably some users out there who do.
>>>
>>> If they do so, they are fooling themselves... or they are running a
>>> patched kernel! :)
>>>
>>> /Jonas
>>>
>>>
>>>
>>>
>>>>
>>>> Cheers,
>>>> ta
>>>>
>>>>>
>>>>> /Jonas
>>>>>
>>>>>>
>>>>>> Jonas, Yong, what do you think?
>>>>>>
>>>>>> Cheers,
>>>>>> ta
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Yong
>>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
>>>>>>> Sent: Tuesday, March 12, 2019 5:30 AM
>>>>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James
>>>>>>> Tomasetta <James.Tomasetta@cypress.com>
>>>>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org;
>>>>>>> richard@nod.at; marek.vasut@gmail.com;
>>>>>>> computersforpeace@gmail.com; dwmw2@infradead.org
>>>>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect
>>>>>>> write-protect input
>>>>>>>
>>>>>>> Hi, Yong,
>>>>>>>
>>>>>>> Thank you for the explanation. There are still few things to clarify.
>>>>>>>
>>>>>>> On 03/11/2019 10:14 PM, Yong Qin wrote:
>>>>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
>>>>>>>>
>>>>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
>>>>>>>>
>>>>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.
>>>>>>>
>>>>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1?
>>>>>>>
>>>>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> ta
>>>>>>>
>>>>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
>>>>>>>
>>>
>>> ______________________________________________________
>>> Linux MTD discussion mailing list
>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input

[Tudor.Ambarus]

Hi, Jonas,

On 03/20/2019 11:05 PM, Jonas Bonn wrote:
> External E-Mail
> 
> 
> 
> On 20/03/2019 19:56, Yong Qin wrote:
>> Hi Jonas, Tudor,
>>
>> I think Tudor described is a reasonable use case scenario.
>>
>> BPNV bit default value is 0, which means BP0-2 bits are default Non-Volatile. In this case, BPNV bit does not need to be set. After the flash chip mounted on PCB board with WP# grounded, all need to do are: Program data into flash --> Set BP0-2 to 1 --> Set SRWD to 1, then both SRWD bit and BP0-2 bits are locked-down, unless disconnecting WP# from grounding. Reset will not change SRWD bit and BP0-2 bits since they all are Non-Volatile.
> 
> OK, my own use case is causing me to get confused over the volatility of these bits.
> 
> The use case I have is the following:
> a)  The flash must be write-protected at power on
> b)  The flash may be made writable by an authorized actor

Can you control who is authorized or not?

Just for the fun of conversation, there are flashes that come write-protected by
default after a power-on reset cycle (see sst26vf064B). Winbound (ex. W25Q128FV)
and Macronix (ex. MX25U12835F) have this feature too.

> 
> So the idea is to do this:
> i)  Set BPNV to 1 so that BP0-2 get reset to 1 at power on

You would also want to set SRWD to 1, to lock the state of SRWD and BP0-2 bits.

Setting BPNV to 1 will make the BP0-2 protection bits volatile with default
value of 111 after power on or reset.

> ii)  WP# is asserted at power on (effectively grounded)

If SRWD was previously set to 1, SRWD and BP0-2 are protected, indeed.

> iii)  Authorized actor invokes action to de-assert WP#

SRWD and BP0-2 are now writable.

> iv)  BP0-2 bits can now be modified freely and flash is writable

OK

> v)  Authorized actor may invoke action to assert WP# but if that doesn't happen and the system gets restarted at least the flash will not be writable when it is restarted

I see your problem. SRWD is not OTP, and when cleared will not protect the state
of the BP0-2 bits and implicitly will not protect the flash after a power-on
reset cycle. This is way you want to always set SRWD to 1 in software.

> 
> Control of the BP bits in Linux is via stm_lock and stm_unlock.  These functions also set and clear SRWD.  This breaks the above use case because when the device is unlocked, SRWD is cleared, and then device is not protected after a power cycle.
> 
> Suggestions?  Can we separate the manipulation of SRWD from the manipulation of the BP bits without breaking existing users?
>

Probably not, or at least I don't have a solution for now. Have you read what
other data protection mechanisms offers Cypress?

Cheers,
ta

> /Jonas
> 
>>
>> Best Regards,
>> Yong
>>
>> -----Original Message-----
>> From: Jonas Bonn <jonas@norrbonn.se>
>> Sent: Wednesday, March 20, 2019 3:40 AM
>> To: Tudor.Ambarus@microchip.com; Yong Qin <Yong.Qin@cypress.com>; James Tomasetta <James.Tomasetta@cypress.com>
>> Cc: bbrezillon@kernel.org; richard@nod.at; marek.vasut@gmail.com; linux-mtd@lists.infradead.org; computersforpeace@gmail.com; dwmw2@infradead.org
>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input
>>
>>
>>
>> On 20/03/2019 08:33, Tudor.Ambarus@microchip.com wrote:
>>>
>>>
>>> On 03/20/2019 09:06 AM, Jonas Bonn wrote:
>>>> External E-Mail
>>>>
>>>>
>>>>
>>>> On 20/03/2019 07:33, Tudor.Ambarus@microchip.com wrote:
>>>>> Jonas,
>>>>>
>>>>> On 03/19/2019 09:13 AM, Jonas Bonn wrote:
>>>>>>
>>>>>> On 19/03/2019 07:59, Tudor.Ambarus@microchip.com wrote:
>>>>>>> Jonas, Yong,
>>>>>>>
>>>>>>> On 03/12/2019 09:27 PM, Yong Qin wrote:
>>>>>>>> Hi Tudor,
>>>>>>>>
>>>>>>>> Good question.
>>>>>>>>
>>>>>>>> The WP# function is not available when the Quad mode is enabled (CR[1]=1). The WP# function is replaced by IO2 for input and output during Quad mode. With that said, if CR[1] is set to 1 (Quad mode enabled), the registers will not be protected.
>>>>>>>>
>>>>>>>> Technically default SRWD bit can be set as either 0 or 1. However, as most customers (applications) don't use this feature to protect registers, the default SRWD bit set to 0 might be a better choice, and reserve the option to change to 1 for the applications do need this feature.
>>>>>>>
>>>>>>> I think I found the reason why SRWD bit is configurable, and
>>>>>>> disabled by
>>>>>>> default: => to allow the installation of the flash in a system
>>>>>>> with a grounded WP# pin while still enabling write to the BP bits.
>>>>>>
>>>>>> I think this is bogus.  Why would you ground the SRWD pin?  That's a design error.
>>>>>
>>>>> I've talked with Microchip hardware team, we have this feature on sst26 flashes too.
>>>>>
>>>>> Grounding WP would not necessarily be a design error. The intention
>>>>> is that some users might choose to populate the Flash chip onto
>>>>> their PCB, program the memory in-circuit, and then lock down the write protection to use the chip like a ROM.
>>>>> Grounding WP allows them to do this. Since SRWD is set to '0' from
>>>>> the factory, so users can program the memory, set the block
>>>>> protection (BP) bits to protect the memory, and then set the SRWD
>>>>> bit to enable the grounded WP input and prevent changing the BP bits.
>>>>
>>>> Again, this is bogus.  The SRWD bit is non-volatile so just resetting the flash clears the bit and the BP bits can be changed.  This does not magically turn the flash into a ROM.  Grounded or not, the WP# is NOT respected until the SRWD bit is set; what you've described is yet another reason to default the SRWD to set.
>>>
>>> SRWD is a non-volatile bit: default at power-up will be the setting
>>> prior to power-down.
>>
>> Ah, right you are! :)
>>
>> By grounding the WP# pin, you are effectively turning the SRWD bit into an OTP bit.  Note that what you described above also requires setting the BPNV bit, otherwise protection is irrevocably reset to "unprotected"
>> at reset.  The BPNV bit cannot be set from Linux so whoever is using the "grounded pin" method must be programming the board in-factory from another system than Linux.  Therefore it's OK for Linux to default SRWD to asserted, irregardless of the state of WP#.
>>
>> /Jonas
>>
>>
>>
>>>
>>> Cheers,
>>> ta
>>>
>>>>
>>>> With the BPNV bit (see patch 3 that I posted in the v2 series), at least the flash can be made to start up write-protected; however, there is nothing preventing modification of the PROTection bits until SRWD is set AND WP# is asserted (through grounding or otherwise).  Linux currently defaults SRWD to cleared which is "unprotected", irregardless of the state of WP#.
>>>>
>>>>>
>>>>> The grounded WP pin + SRWD bit method is an older, legacy approach
>>>>> to the problem. We don't know how many users actually ground the WP
>>>>> pin in this manner, but there are probably some users out there who do.
>>>>
>>>> If they do so, they are fooling themselves... or they are running a
>>>> patched kernel! :)
>>>>
>>>> /Jonas
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> Cheers,
>>>>> ta
>>>>>
>>>>>>
>>>>>> /Jonas
>>>>>>
>>>>>>>
>>>>>>> Jonas, Yong, what do you think?
>>>>>>>
>>>>>>> Cheers,
>>>>>>> ta
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Yong
>>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>
>>>>>>>> Sent: Tuesday, March 12, 2019 5:30 AM
>>>>>>>> To: Yong Qin <Yong.Qin@cypress.com>; jonas@norrbonn.se; James
>>>>>>>> Tomasetta <James.Tomasetta@cypress.com>
>>>>>>>> Cc: linux-mtd@lists.infradead.org; bbrezillon@kernel.org;
>>>>>>>> richard@nod.at; marek.vasut@gmail.com;
>>>>>>>> computersforpeace@gmail.com; dwmw2@infradead.org
>>>>>>>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect
>>>>>>>> write-protect input
>>>>>>>>
>>>>>>>> Hi, Yong,
>>>>>>>>
>>>>>>>> Thank you for the explanation. There are still few things to clarify.
>>>>>>>>
>>>>>>>> On 03/11/2019 10:14 PM, Yong Qin wrote:
>>>>>>>>> SRWD bit (along with WP#) provides a way to protect Status and Configuration Registers from been modified unintendedly or by a malicious actor.
>>>>>>>>>
>>>>>>>>> By default, SRWD bit is 0, which means no protection on registers alternations. Registers can be modified easily by WRR command. (this is most of the application use cases).
>>>>>>>>>
>>>>>>>>> If set SRWD bit to 1, then when WP# is driven low during WRR command, WRR command will be ignored and Registers can't be modified. This provides a way to protect Registers, meanwhile still reserve the capability to modify Registers when necessary by driving WP# to high during WRR command.
>>>>>>>>
>>>>>>>> Does the SRWD bit protect the Status and Configuration Register bits even when in Quad Mode? WP# function is not available in Quad mode. How can one release this protection when in Quad Mode and SRWD set to 1?
>>>>>>>>
>>>>>>>> If SRWD bit is ignored in Quad Mode, then why didn't Cypress enable Status and Configuration Register bits protection by default? I.e., remove SRWD bit from SR1, make BIT(7) a NOP, and consider the Status and Configuration Register bits protection enabled by default when not in Quad Mode.
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> ta
>>>>>>>>
>>>>>>>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
>>>>>>>>
>>>>
>>>> ______________________________________________________
>>>> Linux MTD discussion mailing list
>>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
>> This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
>>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v3 1/1] spi-nor: always respect write-protect input

[Jonas Bonn]

The status register bit SRWD (status register write disable) is
described in many words in the datasheets but effectively boils down to:

i) if set, respect WP# when trying to change protection bits;
ii) if unset, ignore WP# when trying to change protection bits

In short, the bit determines whether the WP# signal is honored or not.

This protection bit is used in a couple of different ways:

i)  Some designs attach WP# directly to ground.  At first boot, they
write to the NOR and call flash_lock; this results in the BP0-2 block
protection bits and SRWD being set.  As WP# is permanently grounded,
this block protection cannot be undone and the NOR is effectively
read-only.

ii)  Some designs can control WP#, thus allowing the protection of the
SRWD bit itself to be managed.  When the hardware pulls WP# low, SRWD
and the BP[0-2] block protection bits cannot be changed; when hardware
sets WP# high, the block protection bits can be freely changed,
_irregardless_ of the state of SRWD.

iii)  In the third configuration WP# is pulled high internally, thereby
rendering the SRWD bit ineffective.  In this configuration, the BP[0-2]
block protection bits can always be freely modified; this puts the
writability of the NOR flash exclusively under software control.

Linux currently sets SRWD when flash_lock is invoked.  This prevents
further modification of the BP[0-2] bits and the SRWD bit itself, _if_
WP# is in play; if WP# is not in play, this setting has no effect.  This
behaviour is rational.

Linux, however, clears SRWD when the flash_unlock is invoked to clear
all BP[0-2] bits.  If WP# is low, this will fail and SRWD and the block
protection bits will remain unchanged.  If WP# is high, changing the
BP[0-2] bits will succeed irregardless of the state of SRWD, so clearing
SRWD has no meaningful effect here.

There is, however, another scenario where clearing SRWD when calling
flash_unlock causes unwanted behaviour.  If the BPNV bit is set, the
BP[0-2] bits revert to "fully protected" at reset.  In this
configuration, only someone who can control WP# is able to call
flash_unlock and clear the block protection bits.  In this
configuration, it is desirable that SRWD is not ever cleared so that the
WP# always has full control over the writability of the BP[0-2] bits.
This allows the NOR flash to be made writable _only_ by someone who has
control over the WP# signal; for others, the NOR is read-only.

Given the above, this patch removes the clearing of the SRWD bit from
the flash_unlock path.  With this, Linux only ever sets the SRWD; it
will never clear it.  This should be compatible with all three
configuration outlined above:

i)  If WP# is permanently grounded, SRWD is not clearable anyway.
ii)  If WP# is controllable, SRWD is moot because WP# takes over its
role
iii)  If WP# is floating and thereby pulled permanently high, SRWD has
no effect on the writability of the block protection bits.

Tested on a Cypress s25fl512s.

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
---

Hi Tudor,

I think this problem is simpler than we think.  Just removing the clear
of SRWD in the flash_unlock path is sufficient to cover all the various
cases, as far as I can tell.

Best regards,
Jonas


 drivers/mtd/spi-nor/spi-nor.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index fae147452aff..bc3317f2bc7c 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1313,10 +1313,6 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 
 	status_new = (status_old & ~mask & ~SR_TB) | val;
 
-	/* Don't protect status register if we're fully unlocked */
-	if (lock_len == 0)
-		status_new &= ~SR_SRWD;
-
 	if (!use_top)
 		status_new |= SR_TB;
 
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/