Re: [PATCH] mtd: spi-nor: micron-st: Add n25q064a WP support

[Tudor Ambarus]

On 7/30/24 6:28 PM, Brian Norris wrote:
> Hi Tudor, Michael,

Hi, Brian!
> 
> On Tue, Jul 30, 2024 at 4:33 AM Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote:
>> On 7/30/24 7:51 AM, Michael Walle wrote:
>>> On Fri Jul 26, 2024 at 8:58 PM CEST, Brian Norris wrote:
>>>> These flash chips are used on Google / TP-Link / ASUS OnHub devices, and
>>>> OnHub devices are write-protected by default (same as any other
>>>> ChromeOS/Chromebook system). I've referred to datasheets, and tested on
>>>> OnHub devices.
>>>
>>> Out of curiosity, there is also a hardware write protect switch
>>> somehow, right? At least that's my understanding how verify boot
>>> works.
> 
> There's a whole doc on this:
> https://www.chromium.org/chromium-os/developer-library/reference/security/write-protection/
> 
> Short answer: yes.
> 
> If you want to see how this fits together in practice on a
> non-ChromiumOS system, you can see my companion OpenWrt work here:
> https://github.com/openwrt/openwrt/pull/16014
> 
> Basically, I just try to make it easier for tools (in this case, the
> CrOS vboot tools) to find the write-protect GPIO, and cross-reference
> that with the software (MTD "locked" ioctls) protection status. We
> need to understand and manipulate both if we want to (temporarily)
> remove write protection, while otherwise retaining verified boot
> integrity.
> 
>>>> Signed-off-by: Brian Norris <computersforpeace@xxxxxxxxx>
>>>
>>> This looks good:
>>> Reviewed-by: Michael Walle <mwalle@xxxxxxxxxx>
>>>
>>> But could you have a look whether this flash supports SFDP.
>>> According to the datasheet it looks like it does. In that case,
>>> could you please dump it according to:
>>> https://docs.kernel.org/driver-api/mtd/spi-nor.html
> 
> Sorry, I didn't notice this doc. It's not technically a "new" flash,
> so it doesn't necessarily apply, but for the mail-archive record:
> 
> # xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> 53464450000100ff00000109300000ffffffffffffffffffffffffffffff
> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0329eb276b
> 083b27bbffffffffffff27bbffff29eb0c2010d800000000
> 

Thanks! We keep a database of SFDP dumps and when we touch a flash, we
compare the SFDP dumps of the flavors of that flash ID to make informed
decisions. There are lots of flashes that have wrong SFDP data, so we
introduced some SFDP fixups, where we tweak the incorrect parameters.
Also, we encountered cases where flashes with same ID have different
SFDP data and we ended up differentiating the two based on a SFDP
difference. So we should know the SFDP even for flash updates, it help
us having a more relevant database.

With time we'll add a SFDP decoder, but we couldn't allocate time for
that yet.

A shasum on the SFDP dump would be good to have some sort of integrity
assurance, e.g.:
sha256sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp

>> This would help getting rid of the no_sfdp_flags and size from the flash
>> definition. Another reason is that the SFDP dump can help us
>> differentiate between flavors of the same flash, if it'll ever be the
>> case, and help us keep backward compatibility.
> 
> I wonder, since manufacturers seem to reuse IDs sometimes, is it
> possible (or, likely) for SFDP and non-SFDP versions of the same flash
> to exist? I'm not really sure whether I can truly drop the non-SFDP
> definitions.

It's possible, yes. We lean towards using only SFDP if possible. The
mechanism to determine the correct flash parameters when we have a flash
ID with 2 flavors, one with SFDP and one without, is in discussion,
we'll get to a conclusion soon. I'm willing to break backward
compatibility for non-SFDP flashes, and if/when complains arise, we'll
use the fallback mechanism to non-SFDP params. But after we'll have such
mechanism.
> 
>> Also, if you care, would be good to extend the SPI NOR documentation on
>> how one shall test the Block Protection.
> 
> That sounds tougher. I don't know that there's really a standard
> toolset for handling protection -- I guess the 'flash_{un,}lock'
> utilities in mtd-utils qualify, but they aren't even packaged by
> relevant distros (e.g., OpenWrt; but I guess they're in Debian). I
> typically use flashrom, since that's what ChromiumOS uses, and it's
> available in OpenWrt -- would that be an OK basis for docs?

yes, why not. At least for people using OpenWrt.
> 
> It's also highly conditional on what sort of range(s) the flash
> supports. So it's almost like we might want a programmatic
> write-protection test suite as part of mtd-utils/tests/, rather than a
> bespoke narrative document. Which ... is getting a little larger than
> I signed up for.
> 

Some test in mtd-utils would be good indeed, but narrative shall be also
ok for now. What I fear is that people just use just a flash lock/unlock
all sectors test, which is not ideal. We shall also test locking on some
sectors from the top and bottom, to verify the correctness of the TB
bit, check if BP3 is working by locking some sectors in that area.
Haven't looked at the BP area in a while, but you get my point, I feel
testing is not ideal and a guideline would help.

If you ever feel that you can spend some time on this, help is appreciated.

Thanks,
ta