linux-kernel - Re: [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are non-volatile

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: <b490ed3a-c3ac-7fa6-cab1-161a74b15fe4@microchip.com>
Date:   Wed, 2 Dec 2020 14:09:03 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <michael@...le.cc>
CC:     <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <miquel.raynal@...tlin.com>, <richard@....at>, <vigneshr@...com>,
        <boris.brezillon@...labora.com>
Subject: Re: [PATCH v6 5/5] mtd: spi-nor: keep lock bits if they are
 non-volatile

On 12/2/20 1:25 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2020-12-02 12:10, schrieb Tudor.Ambarus@...rochip.com:
>> On 11/30/20 4:38 PM, Michael Walle wrote:
> [..]
>>>>> +        * indicated by SNOR_F_WP_IS_VOLATILE.
>>>>> +        */
>>>>> +       if (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE) ||
>>>>> +           (IS_ENABLED(CONFIG_MTD_SPI_NOR_WP_DISABLE_ON_VOLATILE)
>>>>> &&
>>>>> +            nor->flags & SNOR_F_WP_IS_VOLATILE)) {
>>>>> +               err = spi_nor_unlock_all(nor);
>>>>> +               if (err) {
>>>>> +                       dev_err(nor->dev, "Failed to unlock the
>>>>> entire
>>>>> flash memory array\n");
>>>>
>>>> dev_dbg for low level info
>>>
>>> Is this low level info or an actual error? Which raises the question:
>>> should spi_nor_unlock_all() in case SWRD couldn't be cleared and thus
>>> should all the spi_nor_init fail of this? Or should it rather be a
>>
>> yes, it should, because the flash will not work as expected/requested.
> 
> One counterargument: take our sl28 board, it has a hardware
> write-protected
> SPI flash. It actually works right now because the write_sr_and_check()
> doesn't work as intended and doesn't check what is written. So if you'd
> fix that (and these changes would be backported to the stable trees),
> you'd
> basically break spi-nor on these boards. And this _must_ be the case for
> all boards which are actually using (hard- or sofware) write-protection.
> That is the only way write-protection makes sense prior to this patch
> series. Because linux will happily unlock every flash on startup.
> Therefore,
> the hardware write protection is the only measure against this.
> 

I see. If WP# is asserted, spi_nor_unlock_all() would fail and would stop
the execution. One can avoid the fail by choosing MTD_SPI_NOR_SWP_KEEP,
but that would not be backward compatible. Having in mind that in the
past the unlock all was just a zero written to SR, without checking if
the write was successful, I would now say that your proposal with the
soft error if fair. Even if writes and erases will not work in case WP#
is asserted, we would at least let users do reads. The writes and erases
problems would be caught when enabling the debug traces. So please go
ahead and print just a dev_dbg when spi_nor_unlock_all() fails, without
stopping the execution.

Cheers,
ta