Am 01.10.2018 23:38 schrieb Youness Alaoui:
On Sat, Sep 29, 2018 at 12:21 PM Nico Huber nico.h@gmx.de wrote:
On 9/27/18 10:29 PM, Youness Alaoui wrote:
Thanks everyone for the responses. So far my understanding on the task at hand is :
- Add a CONFIG to decide if we set FLOCKDN or not (and one to decide
if we lock it on the resume path?)
Maybe no config at all, see discussion of PRR34_LOCKDN below. But if, then only one config, IMO: On the resume path FLOCKDN should always be set.
- Remove the chipset_lockdown devicetree config and change the code to
always assume it's LOCKDOWN_COREBOOT (this applies to all platforms, right?)
Yes, unless somebody objects. Please make sure to add the right reviewers to this patch. I expect some resistance.
Ok, this is already done, but I want to finish/test my work before I send for review and I might not have time until the end of the week.
- remove the call to fast_spi_pr_dlock() and fix the comment above
that function to be more accurate
Please also check the tree for code that sets PRRs. IIRC, it is used somewhere to protect the MRC cache. I would set the discrete lock directly after writing the region.
Ok, thanks, will do. Note: I've also changed the fast_spi_pr_dlock() function to take 5 u8 arguments to allow the caller to lock each prr individually instead of being an all or nothing.
- Add .config CONFIG options or maybe optional devicetree configs for
how to setup the registers so coreboot can prepare them (due to the resume path) ?
Hmm, I would add Kconfig options for this as well. I you want to match the configuration of your payload, it definitely doesn't belong into the devicetree. Note that "prepare" also implies to lock them on the resume path.
Yeah, so far, the KConfigs I added are : FAST_SPI_LOCK_REGISTERS FAST_SPI_LOCK_REGISTERS_ON_RESUME FAST_SPI_LOCK_PRR34_REGISTERS FAST_SPI_LOCK_PRR34_REGISTERS_ON_RESUME FAST_SPI_PRR3_REGISTER (hex value) FAST_SPI_PRR4_REGISTER (hex value)
This way, a user can lock/unlock the register (since it affects more than just the PRRs) and they can override/set the PRR3 and PRR4 values and decide whether to lock them or not or only on resume, etc.. I think those should be enough for most use cases and it's enough for what I need.
AIUI, FLOCKDN always locks the PRRs.
Actually from what I can see, the FLOCKDN will lock a few registers, among which it will lock the "discrete PR locks" register, and it will lock the PR 0, 1 and 2, but there's also a PRR34_LOCKDN bit to separately lock the PRR 3 and 4. So technically, I could leave FLOCKDN set, and in the payload, just set protected range in PRR3 or PRR4, then set the PRR34_LOCKDN. Unfortunately, I can't do that because the PRR3 and 4 were already locked with the discrete lock register (if I remove the call to fast_spi_pr_dlock(), it should be fine for my needs though I think, since you said swseq would also fail for protected ranges).
Ah, that PRR34_LOCKDN was only introduced with SKL. Didn't know it or at least didn't remember it. In this case it seems best to always set FLOCKDN in coreboot. The payload can then still protect additional regions with PRR3/4. On the resume path, coreboot would set both FLOCKDN and PRR34_LOCKDN, right?
On resume, even if coreboot sets them, the PRRs need to be filled, which is why I also added the hex value of the PRR in the KConfig. I also want to keep FLOCKDN unlocked, so Heads can set the write protect bits in the flash then prevent writing to the status register via hwseq before locking it with FLOCKDN.
Also note that swseq is not an option anymore since Skylake (IIRC, the ME protects the flash-cycle go bit in its default configuration). With only hwseq, you can only access the flash chip's first status register which makes many security features unusable. So we have to rely on the protected regions :-/ I hope Intel is going to fix that for future plat- forms (or with an ME update if that could add more hwseq commands).
Well, I saw the opmenu/optypes/etc.. registers still being set in skylake (their offsets were not in the datasheet though), so I figured swseq is still there, just undocumented. It might work on systems where the ME is disabled. I'd have to test. I was thinking of using swseq and add the write-register-2 opcode and use it to set SPR1 and lock the entire flash until it gets power cycled. I don't know if that would work during suspend, but if it does, it might help with the resume path at least (but would prevent updating flash after a reboot and would force user to shut down and boot). I guess I have some trial-error to do.
The tricky part is to get access to the flash chip's second status register. I couldn't figure any way to do that with SKL (this cost me some weeks of work once because FSP also locked empty flash pro- tection).
Only way I see is through swseq, but you say it doesn't work because of the ME. I'd have to test if the disabled ME allows swseq to work.
Actually, if status register remains the same through a reboot/suspend/resume, and swseq doesn't work on skylake at all, I could just set the protect bits and prevent writing to status register on resume path.
I don't understand, is there a lock to prevent status register writes with hwseq? Also I though, you can't disable these status register bits w/o a reset. Whether the flash chip is reset, depends on your board design and power sequencing ofc.
Yes, see bit 11 of HSFSC register (WRSDIS). That one already has a Kconfig to decide if we want it locked or not.
One way would be to let coreboot decide, e.g. prepare the configuration and don't lock it, and let the payload lock. The payload could validate this configuration before locking (and issue a warning if coreboot didn't set the expected bits).
Thanks, that's a good idea, I forgot about the suspend/resume case. This implies I need coreboot to know about what config the payload will want to set. I'm not sure if that should go into a CONFIG or a devicetree config. Since it might be user-configured, it makes more sense for it to go into .config, right ?
Yes, Kconfig seems to be the right place.
Nico
Thanks!
is there already a gerrit change you are working on this? Do plan to support / test a X230 flash chip's lock bits? * MX25L6406E/MX25L6408E * EN25QH64 * MX25L3206E
thanks,
martin
Hi,
Am 05.10.18 um 08:39 schrieb Martin Kepplinger:
is there already a gerrit change you are working on this? Do plan to support / test a X230 flash chip's lock bits?
- MX25L6406E/MX25L6408E
- EN25QH64
- MX25L3206E
not about this request in particular but such flash-chip features in general: IMO, the best solution is to implement these in (lib)flashrom and integrate that into coreboot. I wouldn't say that it's wasted time to implement it in coreboot directly. But, in the long run, taking shortcuts or maintaining redundant implementations will slow the pro- ject down overall.
Nico
Hi Everyone!
Sorry for the 2 weeks late reply, I've read your responses, but I've been too busy and dealing with stuff and haven't had/taken the time to reply but your input was very much appreciated and not ignored! One thing to note is that this week will be my last week at Purism as I'm going on sabbatical for a few months, and I may or may not (most likely won't) come back to Purism in a few months. So this feature will probably be my last coreboot contribution (for a while at least).
I'm going to try and reply quickly to some of the comments. @Sam
This seems to imply that each time a Librem user wants to internally
flash the ROM, she would have to:
Yes, updating the flash would be a relatively complicated process which is ok, because for me and you, sure, we'd update the rom a few times a day/week, but for 90% of the users, they would either : - never do it - only do it when a new update is released and interests them, i.e: once or twice a year. So it's a complicated process (as you've outlined) but it would be far from annoying to users since most won't care or notice that all that is needed, and if they do, they won't care to have to do it so rarely. It will however, especially with cover-opening protections (either using glitter/whatever methods on screws to notice if they'd been opened, or with an EC handling a cover switch notification), that could be very helpful to increase their security.
Your assumption fails against a BadHeads attack.
Yes indeed, I wrote a proof of concept which was a Heads that would extend PCRs with the same values that coreboot did (and have a coreboot with measured boot disabled) and it passed the TOTP authentication. That's something that other possible solutions would try to mitigate (such as vboot I think).
@Nico:
You two might have different kinds of updates in mind. You don't need
a switch for every update. Well, I hadn't thought this through to be honest, but in theory you could still flash early in the boot process (in heads) if the protection bits aren't set on the flash. So using the jumper might only allow flashing within the OS itself. Or maybe if the bootblock locks its own region, maybe setting the jumpre would allow the user to flash a new root of trust only. I suppose that's where vboot comes through, and I've left Kyle to investigate all that (though he says we don't need it, but I'm starting to think he's wrong and I think he could be convinced easily enough, I'll point him to this thread which should do the trick). Anyways, I'm leaving Purism now and going on vacation so I'm not going to bother trying to wrap my head around all that.
FYI: What I'm trying to do is to improve the security for current machines, so the discussion on the jumper/etc.. is irrelevant for machines already in the hands of customers.
@ Sam :
Youness and others at Purism: it would be great if Purism could be sure not only to spec, but also to list on the Librem specification pages on the website, a SPI flash ROM chip that *does* obey its write-protect pin regardless of firmware. Thanks :)
I didn't realize that "some chips don't obey the write-protect pin", but rather my understanding is that the write protect pin is there to say "the protected blocks are protected/not-protected according to hardware. The SPI flash (according to the datasheets I've read) can protect regions either with "don't protect" or "protected by WP#" or "protected until power-cycle". The status register bits can be written to either as volatile or non-volatile (for non volatile, you need another command iirc, and you can't do it with hardware sequencing, same for the 'protect until power cycle', which also needs to write to status-register-2 which can't be done with hardware sequencing either). So, really, a flash rom chip does obey its WP pin, it just depends on how it's set.
On Fri, Oct 5, 2018 at 6:14 AM Nico Huber nico.huber@secunet.com wrote:
Hi,
Am 05.10.18 um 08:39 schrieb Martin Kepplinger:
is there already a gerrit change you are working on this? Do plan to support / test a X230 flash chip's lock bits?
- MX25L6406E/MX25L6408E
- EN25QH64
- MX25L3206E
not about this request in particular but such flash-chip features in general: IMO, the best solution is to implement these in (lib)flashrom and integrate that into coreboot. I wouldn't say that it's wasted time to implement it in coreboot directly. But, in the long run, taking shortcuts or maintaining redundant implementations will slow the pro- ject down overall.
Nico
coreboot mailing list: coreboot@coreboot.org https://mail.coreboot.org/mailman/listinfo/coreboot
-
Martin Kepplinger -
Nico Huber -
Youness Alaoui