Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WiP: PR0 (SPI write prevention through chipset locking) for Skylake+ #1818

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tlaurion
Copy link
Collaborator

@tlaurion tlaurion commented Oct 20, 2024

@miczyg1 pushed some changes in dasharo coreboot fork to enable SMM PR0:
This cherry-pick this commit Dasharo/coreboot@ff22122

To test this PR for nv41 users (EC needs to be up to date otherwise problems might occur):

  • Flash this PR rom
  • go to recovery shell
  • call lock_chip from command line (locks PR0)
  • call flash-gui.sh
  • attempt to flash master's rom, see failing: expected
  • reboot (cancels PR0)
  • flash back master's rom from gui: see success.

At term, this PR will bring coreboot Skylake+ equivalent of <=Shylake for PR0 chipset locking of SPI WP (single boot protection from OS).

@MrChromebox you might take a look at #1659, which this PR will fix when working on all Skylake+ based boards merged for <Skyylake at #1373


NOTE: if for whatever reason, you would love to disable PR0 locking on a single boot option, you can do so through configuration settings menu under heads as screenshots show under #1373 (comment) :

signal-2023-06-20-135059
signal-2023-06-20-135116
signal-2023-06-20-135128
signal-2023-06-20-135145

On boot, the presence of "Finalizing chipset" disappears, where its present on normal boot path otherwise.


At term, this will fix #1659

TODO:

  • nv41 functional, flashrom tested internally after calling lock_chip: unsuccessful (ok!)
  • fix and merge Correct PR0 statement under lock_chip #1814
  • add patch to other forks
    • or merge this PR while other forks pick it up?
  • adapt lock_chip and verbiage if not just fro pre-skylake in codebase
  • +1 review on coreboot when patchset tested and working for all skylake+

--

@JonathonHall-Purism patch doesn't apply cleanly on purism fork

@tlaurion tlaurion marked this pull request as draft October 20, 2024 17:46
@tlaurion tlaurion changed the title WiP: Pr0 skylake and more recent WiP: Pr0 (SPI write prevention through chipset locking) for skylake and more recent Oct 20, 2024
@tlaurion tlaurion changed the title WiP: Pr0 (SPI write prevention through chipset locking) for skylake and more recent WiP: P00 (SPI write prevention through chipset locking) for skylake and more recent Oct 20, 2024
@tlaurion tlaurion changed the title WiP: P00 (SPI write prevention through chipset locking) for skylake and more recent WiP: PR0 (SPI write prevention through chipset locking) for skylake and more recent Oct 20, 2024
@tlaurion
Copy link
Collaborator Author

@miczyg1 false alarm, it was again my external programmer's fault. Tigard doesn't work well but again I have a leg of wson8 probe broken.

This is good to go!

@tlaurion tlaurion marked this pull request as ready for review October 20, 2024 18:47
@tlaurion
Copy link
Collaborator Author

Question here is what do we want prior of merging @miczyg1 @JonathonHall-Purism.

All Skylake+ platforms implementing PR0?

@tlaurion tlaurion self-assigned this Oct 20, 2024
@tlaurion tlaurion linked an issue Oct 22, 2024 that may be closed by this pull request
@tlaurion tlaurion mentioned this pull request Oct 23, 2024
15 tasks
@tlaurion tlaurion changed the title WiP: PR0 (SPI write prevention through chipset locking) for skylake and more recent WiP: PR0 (SPI write prevention through chipset locking) for Skylake+ Oct 29, 2024
@tlaurion
Copy link
Collaborator Author

Question here is what do we want prior of merging @miczyg1 @JonathonHall-Purism.

All Skylake+ platforms implementing PR0?

@macpijan @JonathonHall-Purism let me know if something else needs to be done prior of #1821 deadline. Could be in or not: I would prefer this to be in.

@tlaurion
Copy link
Collaborator Author

Rebasing on master.

@JonathonHall-Purism
Copy link
Collaborator

@tlaurion @miczyg1 Nice work here, I'm thrilled to see this. I hope we can get it to work 🤞

What SoCs has this been tested on so far? I applied the coreboot patch to Purism's coreboot fork and built this for the Librem 14; the OS is still able to write to the firmware. The coreboot patch is implemented for cannonlake but I don't know if that was tested.

I did see Finalizing chipset Write Protection through SMI PR0 lockdown call, so the config is right and Heads did appear to try to lock flash. (Full disclosure, I booted manually with basic-autoboot.sh from the recovery shell, but this uses the same kexec-boot path and I did see that trace.)

The OS was still able to write to flash (in this case I changed the serial number using our coreboot utility: that reads flash, uses cbfstool to change the serial, then writes flash).

I haven't looked into it any further. What SoCs have you all tested? I'd be happy to help get this working for CNL and other SoCs but I probably will not be able to do it prior to the feature freeze.

Question here is what do we want prior of merging @miczyg1 @JonathonHall-Purism.

All Skylake+ platforms implementing PR0?

I'd suggest:

  • Heads configures all supported platforms with this behavior (we want similar configs for all boards upstream)
  • Offer a configuration toggle to control it for now - I will want to have better integration with our coreboot utility and other tooling before I enable it in PureBoot (they need to tell the user what to do, can't just fail in confusion). (I'm happy to help with this too.)

@tlaurion
Copy link
Collaborator Author

tlaurion commented Oct 31, 2024

  • Offer a configuration toggle to control it for now

It's already there as per original PR0 PR: as of now, user can turn it off for one boot under config settings toggle.
@JonathonHall-Purism what would you suggest instead?

Per OP workflow screenshots:

signal-2023-06-20-135128

@tlaurion
Copy link
Collaborator Author

Rebasing on master

@miczyg1
Copy link
Contributor

miczyg1 commented Nov 5, 2024

@tlaurion @miczyg1 Nice work here, I'm thrilled to see this. I hope we can get it to work 🤞

What SoCs has this been tested on so far? I applied the coreboot patch to Purism's coreboot fork and built this for the Librem 14; the OS is still able to write to the firmware. The coreboot patch is implemented for cannonlake but I don't know if that was tested.

I did see Finalizing chipset Write Protection through SMI PR0 lockdown call, so the config is right and Heads did appear to try to lock flash. (Full disclosure, I booted manually with basic-autoboot.sh from the recovery shell, but this uses the same kexec-boot path and I did see that trace.)

The OS was still able to write to flash (in this case I changed the serial number using our coreboot utility: that reads flash, uses cbfstool to change the serial, then writes flash).

I haven't looked into it any further. What SoCs have you all tested? I'd be happy to help get this working for CNL and other SoCs but I probably will not be able to do it prior to the feature freeze.

Question here is what do we want prior of merging @miczyg1 @JonathonHall-Purism.
All Skylake+ platforms implementing PR0?

I'd suggest:

  • Heads configures all supported platforms with this behavior (we want similar configs for all boards upstream)
  • Offer a configuration toggle to control it for now - I will want to have better integration with our coreboot utility and other tooling before I enable it in PureBoot (they need to tell the user what to do, can't just fail in confusion). (I'm happy to help with this too.)

@JonathonHall-Purism: I checked it on Alder Lake and Comet Lake (so using coreboot's soc/cannonlake) and it worked.

tlaurion Edited: tagged Jonathon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Correct PR0 statement under lock_chip SMI Platform locking on newer platforms (Skylake+)
3 participants