Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 3:
(9 comments)
What is the time penalty when enabling this?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN SECURITY_HAVE_BOOTMEDIA_LOCKDOWN
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 6: support supports
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 22: media medium?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 26: NOTE: If you trigger the chipset lockdown unconditionally, I wouldn’t indent the note. `NOTE:` is enough “markup”.
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 30: NO_ACCESS RW to be consistent with RO?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/Kconf... PS3, Line 36: The locking will take place during the chipset lockdown, which is Add a blank line above?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.c:
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 40: "whole bootmedia\n"); Won’t this be printed several time?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 49: } Add an else branch to inform the user about an error?
https://review.coreboot.org/c/coreboot/+/32704/3/src/security/lockdown/bootm... PS3, Line 53: Didn't Didn’t or couldn’t …
I think there should be a separate error message for users explicitly wanting to lock the device, but it did not work.