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

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

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@xxxxxxxxxxx>
>> Sent: Wednesday, March 20, 2019 3:40 AM
>> To: Tudor.Ambarus@xxxxxxxxxxxxx; Yong Qin <Yong.Qin@xxxxxxxxxxx>; James Tomasetta <James.Tomasetta@xxxxxxxxxxx>
>> Cc: bbrezillon@xxxxxxxxxx; richard@xxxxxx; marek.vasut@xxxxxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; computersforpeace@xxxxxxxxx; dwmw2@xxxxxxxxxxxxx
>> Subject: Re: [PATCH v2 1/3] mtd: spi-nor: always respect write-protect input
>>
>>
>>
>> On 20/03/2019 08:33, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
>>>
>>>
>>> On 03/20/2019 09:06 AM, Jonas Bonn wrote:
>>>> External E-Mail
>>>>
>>>>
>>>>
>>>> On 20/03/2019 07:33, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
>>>>> Jonas,
>>>>>
>>>>> On 03/19/2019 09:13 AM, Jonas Bonn wrote:
>>>>>>
>>>>>> On 19/03/2019 07:59, Tudor.Ambarus@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx <Tudor.Ambarus@xxxxxxxxxxxxx>
>>>>>>>> Sent: Tuesday, March 12, 2019 5:30 AM
>>>>>>>> To: Yong Qin <Yong.Qin@xxxxxxxxxxx>; jonas@xxxxxxxxxxx; James
>>>>>>>> Tomasetta <James.Tomasetta@xxxxxxxxxxx>
>>>>>>>> Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx; bbrezillon@xxxxxxxxxx;
>>>>>>>> richard@xxxxxx; marek.vasut@xxxxxxxxx;
>>>>>>>> computersforpeace@xxxxxxxxx; dwmw2@xxxxxxxxxxxxx
>>>>>>>> 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/