Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32704 )
Change subject: security: Add common boot media write protection ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/32704/5/src/drivers/spi/Kconfig File src/drivers/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/5/src/drivers/spi/Kconfig@64 PS5, Line 64: config SPI_FLASH_CTRL_PROTECT Technically coreboot supports multiple SPI flashes on multiple SPI controllers, so you can't really generalize over all of them. I'm not sure you need this... you already have runtime checks to make sure you print an error and continue gracefully if this is not supported, I think that should be good enough.
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... PS5, Line 2: config SECURITY_BOOTMEDIA_LOCKDOWN Again, I'm not really sure what this option is for, I would just display the 'choice' menu below directly. If the selected choice is NONE, you can skip compiling lockdown/bootmedia.c.
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/Kconf... PS5, Line 19: config BOOTMEDIA_LOCK_RO It feels a bit odd that you force both media and controller lockdown into a single option. Do you even need the case where both are tried on any platform? Why not just offer BOOTMEDIA_LOCK_NONE, BOOTMEDIA_LOCK_CHIP_RO, BOOTMEDIA_LOCK_CONTROLLER_RO and BOOTMEDIA_LOCK_CONTROLLER_NO_ACCESS? Or if you do have a platform where you really need to have it try both ways, you could take all of those and add another BOOTMEDIA_LOCK_BOTH_RO. With every platform having its own constraints here I think having that extra granularity would be helpful.
(In order to not blow it up to too many options, I would make a separate 'choice' for the VBOOT_RO in the next patch, which is only shown when a version with chip locking is selected.)
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.c:
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/bootm... PS5, Line 26: nit: one space too many?
https://review.coreboot.org/c/coreboot/+/32704/5/src/security/lockdown/bootm... PS5, Line 35: #%zu nit: can we use meaningful strings here?