Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32159 )
Change subject: Documentation/security/vboot: Add logic to verify stage/blob using VBOOT 2.1 library ......................................................................
Patch Set 1:
(50 comments)
Patch Set 1:
Is it possible to schedule Google Hangouts VC for this Thursday,April 11th? Also note that I have to add a couple internal folks to the meeting. So, please send me the details once you finalize. Thanks!
Okay, sounds good... I added your email to a meeting for Thursday at 5pm (PDT). Please add whoever else wants to join from your side. If anyone else here (Philipp? David?) is interested in this let me know and I can add you as well.
Thanks, Julius.
https://review.coreboot.org/#/c/32159/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32159/1//COMMIT_MSG@7 PS1, Line 7: Documentation/security/vboot: Add logic to verify stage/blob using
Commit messages summaries have to fit on one line.
Ack
https://review.coreboot.org/#/c/32159/1//COMMIT_MSG@8 PS1, Line 8: VBOOT
vboot
Ack
https://review.coreboot.org/#/c/32159/1//COMMIT_MSG@7 PS1, Line 7: Documentation/security/vboot: Add logic to verify stage/blob using : VBOOT 2.1 library
Maybe: […]
This looks better although it appears to break from coreboot commit message convention where entire path of the file to be committed from root directory is listed at the beginning of the summary line. I'll make the change nevertheless so it can be reviewed.
https://review.coreboot.org/#/c/32159/1//COMMIT_MSG@10 PS1, Line 10: Added
Add
Ack
https://review.coreboot.org/#/c/32159/1//COMMIT_MSG@11 PS1, Line 11: Coreboot
coreboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/index.md File Documentation/security/index.md:
https://review.coreboot.org/#/c/32159/1/Documentation/security/index.md@8 PS1, Line 8: VBOOT
vboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... File Documentation/security/vboot/verified_boot_21.md:
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 1: BootGuard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 1: Coreboot
lowercase: coreboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 8: Bootguard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 10: Coreboot
coreboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 11: This document describes the mechanism implemented in Coreboot using Google VBOOT
Please add a blank line above to separate the paragraphs.
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 11: VBOOT
vboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 11: Coreboot
coreboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 12: Root
root
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 12: BootGuard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 13: Trust
trust
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 13: Coreboot
coreboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 16: VBOOT
vboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 16: Coreboot
coreboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 20: BootGuard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 22: BootGuard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 24: ACM
As this is documentation, please also give the full name or link to the corresponding documentation.
Added information about ACM.
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 24: measured in TPM) by a piece of firmware (ACM) which itself is verified by Intel : CPU microcode
Verified by microcode?
Removing microcode. Just CPU will suffice in this case.
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 26: BootGuard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 28: Authenticated Code module
Please move that above.
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 29: in
in the
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 29: PolicyManifest(BtG BPM)
Ditto.
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 29: Manifest(BtG KM)
Add a space before (.
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 35: BootGuard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 26: BootGuard are: 1. Use Intel FSP-T with Coreboot bootloader. It contains the : logic of correctly handling BtGuard enabled state. 2. Integrate ACM : (Authenticated Code module) binary in bootloader image. 3. Generate BtGuard Key : Manifest(BtG KM) and BtGuard Boot PolicyManifest(BtG BPM) and embed them in : bootloader image. a. BtG KM contains the hash of the key used for signing BtG : BPM. BtG KM is signed by the key whose hash is embedded in field-programmable : fuses. b. BtG BPM contains the hash of initial stage of boot loader. It also : stores other policies related to Intel TXT, BtG DMA protection etc. 4. Add : entries for CPU microcode patch, ACM, BtG KM and BtG BPM in FIT table. 5. : Update BootGuard related field-programmable fuses on the test platform.
Please format this as a list. […]
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 41: 2.0
Below you use 2. […]
Yes, 2.0 is the current implementation available in the open source code. 2.1 is our enhancement that makes use of vboot 2.1 libraries that is part of this upstreaming effort.
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 41: Coreboot
coreboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 42: has been
is
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 42: feature has been described here, : https://www.coreboot.org/git-docs/Intel/vboot.html
Make it a link.
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 45: Coreboot
coreboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 51: Coreboot
coreboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 63: ,
Please use a dot.
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 64: - Some hardware designs cannot support ‘read-only’ flash region as Root of Trust
Please add a blank line above.
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 64: Root of Trust
root of trust
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 65: BootGuard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 68:
Remove or align all lines.
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 75:
One space for consistency.
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 76: Bootguard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 78: Bootguard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 79: )
Remove.
Thanks for the catch. It was actually missing the open parenthesis. Will add.
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 80: VBOOT
vboot
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 86: Bootguard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 88: Bootguard
Boot Guard
Ack
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 89: GBB
Elaborate what GBB is?
On line 17, I have pasted the link to vboot git-docs in coreboot.org. That link explains what GBB is. If I also add it here, this document will bloat in size.
https://review.coreboot.org/#/c/32159/1/Documentation/security/vboot/verifie... PS1, Line 90: verified
verifies?
Ack