| Message ID | 20240726185825.142733-1-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: spi-nor: micron-st: Add n25q064a WP support | expand |
Hi, 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. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> This looks good: Reviewed-by: Michael Walle <mwalle@kernel.org> 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 Thanks, -michael > --- > > drivers/mtd/spi-nor/micron-st.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c > index 3c6499fdb712..e6bab2d00c92 100644 > --- a/drivers/mtd/spi-nor/micron-st.c > +++ b/drivers/mtd/spi-nor/micron-st.c > @@ -436,6 +436,8 @@ static const struct flash_info st_nor_parts[] = { > .id = SNOR_ID(0x20, 0xbb, 0x17), > .name = "n25q064a", > .size = SZ_8M, > + .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP | > + SPI_NOR_BP3_SR_BIT6, > .no_sfdp_flags = SECT_4K | SPI_NOR_QUAD_READ, > }, { > .id = SNOR_ID(0x20, 0xbb, 0x18),
On Tue, Jul 30 2024, Michael Walle wrote: > Hi, > > 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. > >> >> Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > This looks good: > Reviewed-by: Michael Walle <mwalle@kernel.org> Applied to spi-nor/next. Thanks, both!
Hi, Brian, On 7/30/24 7:51 AM, Michael Walle wrote: > Hi, > > 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. > >> >> Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > This looks good: > Reviewed-by: Michael Walle <mwalle@kernel.org> > > 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 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. Also, if you care, would be good to extend the SPI NOR documentation on how one shall test the Block Protection. Cheers, ta
Hi Tudor, Michael, On Tue, Jul 30, 2024 at 4:33 AM Tudor Ambarus <tudor.ambarus@linaro.org> 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@gmail.com> > > > > This looks good: > > Reviewed-by: Michael Walle <mwalle@kernel.org> > > > > 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 > 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. > 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? 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. Brian
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@linaro.org> 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@gmail.com> >>> >>> This looks good: >>> Reviewed-by: Michael Walle <mwalle@kernel.org> >>> >>> 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
> >> 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. We really need some kind of dump the relevant registers here. I have some very old patch, which dumps the status register, but is has it's own quirks. But IMHO we should (maybe additional to the functional tests) look at the locking bits in the corresponding registers. I.e. flash_lock foobar <verify the status register> flash_unlock foobar <verify the status register> flash_lock barfoo <verify the status register> etc. Just inferring the correctness from behavior (exercised by writing to the flash and verifying it) will lead to errors as it is hard to catch all the corner cases. From what I remember, flashrom has it's own drivers in userspace, no? -michael > > > > 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
Hi Tudor, On Wed, Jul 31, 2024 at 1:51 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > 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 # sha256sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp 2030d04758164491e414e1d55e16b04803f5653fb591fda50991c728c49c1c37 /sys/bus/spi/devices/spi0.0/spi-nor/sfdp > 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. OK. It's possible, but no guarantees. I think in the distant past (when I was still maintaining some of this area), I actually started to write such a mtd-utils test, but I never cleaned it up and submitted it. For one, a proper exhaustive test would be rather slow, as we'd want to test all the possible protection ranges, and then erase/write/read the whole thing. Some rough measurement on my system shows about a minute for erasing the whole chip, 25 seconds for writing, and 7 seconds for reading -- which means with 5 bits of protection range (4 bits, plus top/bottom bit) we have 32 combinations * ~1.5 minutes test = 48 minutes. That's ... doable I guess, and it could probably be optimized a bit to reduce the number of erase cycles. But it's not great. Anyway, maybe I'll play with it. Brian
On Wed, Jul 31, 2024 at 2:05 AM Michael Walle <mwalle@kernel.org> wrote: > We really need some kind of dump the relevant registers here. I have > some very old patch, which dumps the status register, but is has > it's own quirks. But IMHO we should (maybe additional to the > functional tests) look at the locking bits in the corresponding > registers. I.e. > flash_lock foobar > <verify the status register> > flash_unlock foobar > <verify the status register> > flash_lock barfoo > <verify the status register> > etc. I don't actually think that would be a very good test. It would be testing the implementation more than the functionality. What do you "verify" in the status register? Would the test just re-implement the swp.c protection-range logic? And notably, this omits *all* checks that the protection register actually protects from anything (write, erase). Or maybe I'm misinterpreting what you mean. > Just inferring the correctness from behavior (exercised by writing > to the flash and verifying it) will lead to errors as it is hard to > catch all the corner cases. Why would that lead to errors? It should be relatively easy to: 1. enumerate the supported protection ranges (MEMLOCK / MEMUNLOCK ioctls on known-likely ranges, looking for EINVAL return codes) 2. iterate through all such ranges; for a given range: 2(a). erase the whole flash 2(b). write the whole flash with a known pattern 2(c). read the whole flash 2(d). ensure that the expected-protected range remains 0xff 2(e). ensure that the expected-unprotected range contains the known pattern I suppose step #1 can be tough, because the full slate of possible protection ranges is technically ... enormous. But "likely" ranges are much fewer, with a few power-of-2 patterns, top/bottom, and maybe some "both top and bottom" ranges on some flashes? Anyway, like I said in my other reply, this should take on the order of 60 minutes on some flashes, which is expensive but not prohibitive. > From what I remember, flashrom has it's own drivers in userspace, > no? Yes, and that's all rather ugly. But it also has a linux_mtd backend since a few years back: https://review.coreboot.org/plugins/gitiles/flashrom/+/HEAD/linux_mtd.c Brian
Hi, > > We really need some kind of dump the relevant registers here. I have > > some very old patch, which dumps the status register, but is has > > it's own quirks. But IMHO we should (maybe additional to the > > functional tests) look at the locking bits in the corresponding > > registers. I.e. > > flash_lock foobar > > <verify the status register> > > flash_unlock foobar > > <verify the status register> > > flash_lock barfoo > > <verify the status register> > > etc. > > I don't actually think that would be a very good test. It would be > testing the implementation more than the functionality. What do you > "verify" in the status register? Would the test just re-implement the > swp.c protection-range logic? And notably, this omits *all* checks > that the protection register actually protects from anything (write, > erase). > > Or maybe I'm misinterpreting what you mean. No that's what I've meant. Maybe I'm having another perspective and I'm biased because of that, but I'm really not trusting the software layer, esp. not when it comes to flash locking. Because most of the time this involves the "very last resort recovery" on our products. So we really have to ensure the flash is locked and therefore, one *must* look at the corresponding bits in the hardware (using the simplest method possible). The beauty of the lock bits is that if you know they are set, there is nothing software can do to write or erase these sectors. Once you know all the bits are set correctly, you can just skip the write/erase/read test. > > Just inferring the correctness from behavior (exercised by writing > > to the flash and verifying it) will lead to errors as it is hard to > > catch all the corner cases. > > Why would that lead to errors? Ever tried to lock two different ranges after another? Or unlock a smaller range than the one that is currently locked? IIRC, that should work. But I've never tried it myself. Or just locking (parts) of a smaller partition (i.e. an mtd device which doesn't cover the whole spi flash). > It should be relatively easy to: > > 1. enumerate the supported protection ranges (MEMLOCK / MEMUNLOCK > ioctls on known-likely ranges, looking for EINVAL return codes) > 2. iterate through all such ranges; for a given range: > 2(a). erase the whole flash > 2(b). write the whole flash with a known pattern > 2(c). read the whole flash > 2(d). ensure that the expected-protected range remains 0xff > 2(e). ensure that the expected-unprotected range contains the known pattern Not sure 2(a) will work or if some flashes will reject the chip erase command if some sectors are locked. To sidestep this I guess you should use the smallest possible erase (i.e. ususally the 4k erase opcode). But yeah, once this is all in place you can probably do that with a tool, otherwise it's really tedious work and doing it by hand is error prone. > I suppose step #1 can be tough, because the full slate of possible > protection ranges is technically ... enormous. But "likely" ranges are > much fewer, with a few power-of-2 patterns, top/bottom, and maybe some > "both top and bottom" ranges on some flashes? Anyway, like I said in > my other reply, this should take on the order of 60 minutes on some > flashes, which is expensive but not prohibitive. Well we support 4 block protection bits and one TB bit. So there are 2^5 different configurations? -michael > > From what I remember, flashrom has it's own drivers in userspace, > > no? > > Yes, and that's all rather ugly. But it also has a linux_mtd backend > since a few years back: > > https://review.coreboot.org/plugins/gitiles/flashrom/+/HEAD/linux_mtd.c > > Brian
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c index 3c6499fdb712..e6bab2d00c92 100644 --- a/drivers/mtd/spi-nor/micron-st.c +++ b/drivers/mtd/spi-nor/micron-st.c @@ -436,6 +436,8 @@ static const struct flash_info st_nor_parts[] = { .id = SNOR_ID(0x20, 0xbb, 0x17), .name = "n25q064a", .size = SZ_8M, + .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP | + SPI_NOR_BP3_SR_BIT6, .no_sfdp_flags = SECT_4K | SPI_NOR_QUAD_READ, }, { .id = SNOR_ID(0x20, 0xbb, 0x18),
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. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- drivers/mtd/spi-nor/micron-st.c | 2 ++ 1 file changed, 2 insertions(+)