linux-kernel - Re: [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global SNOR_F_HAS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <9e521c50-dddf-618d-1472-8d7bf86ecd3e@microchip.com>
Date:   Thu, 1 Oct 2020 15:25:41 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <michael@...le.cc>
CC:     <linux-mtd@...ts.infradead.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <miquel.raynal@...tlin.com>,
        <richard@....at>, <vigneshr@...com>
Subject: Re: [RFC PATCH 1/2] mtd: spi-nor: atmel: remove global
 SNOR_F_HAS_LOCK

On 10/1/20 5:37 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2020-10-01 16:25, schrieb Tudor.Ambarus@...rochip.com:
>> On 10/1/20 5:12 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2020-10-01 16:06, schrieb Tudor.Ambarus@...rochip.com:
>>>> On 10/1/20 3:28 PM, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> This is considered bad for the following reasons:
>>>>>  (1) We only support the block protection with BPn bits for write
>>>>>      protection. Not all Atmel parts support this.
>>>>>  (2) Newly added flash chip will automatically inherit the "has
>>>>>      locking" support and thus needs to explicitly tested. Better
>>>>>      be opt-in instead of opt-out.
>>>>>  (3) There are already supported flashes which don't support the
>>>>> locking
>>>>>          scheme. So I assume this wasn't properly tested before
>>>>> adding
>>>>> that
>>>>>          chip; which enforces my previous argument that locking
>>>>> support should
>>>>>          be an opt-in.
>>>>>
>>>>> Remove the global flag and add individual flags to all flashes
>>>>> which supports BP locking. In particular the following flashes
>>>>> don't support the BP scheme:
>>>>>  - AT26F004
>>>>>  - AT25SL321
>>>>>  - AT45DB081D
>>>>>
>>>>> Signed-off-by: Michael Walle <michael@...le.cc>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/atmel.c | 28 +++++++++-------------------
>>>>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/atmel.c
>>>>> b/drivers/mtd/spi-nor/atmel.c
>>>>> index 3f5f21a473a6..49d392c6c8bc 100644
>>>>> --- a/drivers/mtd/spi-nor/atmel.c
>>>>> +++ b/drivers/mtd/spi-nor/atmel.c
>>>>> @@ -10,37 +10,27 @@
>>>>>
>>>>>  static const struct flash_info atmel_parts[] = {
>>>>>         /* Atmel -- some are (confusingly) marketed as "DataFlash"
>>>>> */
>>>>> -       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K)
>>>>> },
>>>>> -       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K)
>>>>> },
>>>>> +       { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K |
>>>>> SPI_NOR_HAS_LOCK) },
>>>>> +       { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K |
>>>>> SPI_NOR_HAS_LOCK) },
>>>>
>>>> after a quick look in the datasheets of these flashes, I suspect that
>>>> what we have now in the SPI NOR core for SR locking does not work for
>>>> them. They probably supported just "unlock all", clearing all the
>>>> BP bits. Anyway, different problem.
>>>>>
>>>>> -       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K)
>>>>> },
>>>>> -       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K)
>>>>> },
>>>>> -       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K)
>>>>> },
>>>>> -       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K)
>>>>> },
>>>>> +       { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K |
>>>>> SPI_NOR_HAS_LOCK) },
>>>>
>>>> this one does not support BP locking:
>>>> https://www.adestotech.com/wp-content/uploads/doc3668.pdf
>>>>
>>>>> +       { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K |
>>>>> SPI_NOR_HAS_LOCK) },
>>>>
>>>> neither this one:
>>>> https://datasheet.octopart.com/AT25DF321-S3U-Atmel-datasheet-8700896.pdf
>>>>
>>>>> +       { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K |
>>>>> SPI_NOR_HAS_LOCK) },
>>>>
>>>> nor this one:
>>>> https://www.adestotech.com/wp-content/uploads/doc3686.pdf
>>>>
>>>>> +       { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K |
>>>>> SPI_NOR_HAS_LOCK) },
>>>>
>>>> nor this one:
>>>> https://www.adestotech.com/wp-content/uploads/doc3680.pdf
>>>>
>>>> I stop here.
>>>
>>> These are all the ones which use the global unlock. I cannot just skip
>>> the HAS_LOCK bit here, because otherwise this patch wouldn't be
>>> backwards
>>> compatibe. Yes I missed that in the commit log, my bad.
>>>
>>
>> No worries.
>>
>> "unlock all at boot" just cleared the SR bits. Clearing the SR bits
>> unlocks
>> these flashes?
> 
> Clearing bits 5,4,3,2, yes (with SPRL=0)

oh, the horror. Those bits are described as Read Only when describing
Status Register. I'll re-read the datasheets.

> 
>   Conversely, to perform a Global Unprotect, the same WP and SPRL
> conditions
>   must be met but the system must write a Logical 0 to bits 5, 4, 3, and
> 2
>   of the first byte of the Status Register.

OK. I see this under "Global unprotect" section. Strange. Will get back to you.

Cheers,
ta
> 
> This will hopefully be cleaned up by my "mtd: spi-nor: keep lock bits if
> they
> are non-volatile" patch.
> 
> -michael