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 8: Code-Review+2
(4 comments)
LGTM basically, just some documentation stuff left.
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... File src/security/lockdown/Kconfig:
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... PS8, Line 17: (e.g. by the payload or the OS). The help text should explain that this is only supported on certain controllers (e.g. Intel).
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... PS8, Line 28: The locking will take place during the chipset lockdown, which : is either triggered by coreboot (when INTEL_CHIPSET_LOCKDOWN is set) : or has to be triggered later (e.g. by the payload or the OS). This is wrong for chip lockdown, isn't it? It happens immediately when the lockdown code runs.
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/Kconf... PS8, Line 39: Select this if you want to protect the firmware boot medium against : all further accesses. On platforms that memory map a part of the : boot medium the corresponding region is still readable. nit: Just curious... why do we need this option at all? If it only really works on Intel controllers, those are all memory-mapped and the memory-mapping still works afterwards anyway... what's the point in enabling this over CONTROLLER_RO?
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/lockd... File src/security/lockdown/lockdown.c:
https://review.coreboot.org/c/coreboot/+/32704/8/src/security/lockdown/lockd... PS8, Line 21: CTRL nit: why not spell out 'SPI controller'?