Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32705 )
Change subject: security/lockdown: Write-protect WP_RO ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/32705/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32705/2//COMMIT_MSG@7 PS2, Line 7: security/lockdown: Write-protect WP_RO I think this about *when* to perform those actions. I think the subject could be more precise.
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/lockdown/bootm... File src/security/lockdown/bootmedia.h:
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/lockdown/bootm... PS2, Line 16: void security_lockdown_bootmedia(void *unused); Should we move this to boot_device.h ? And how is this fundamentally different than boot_device_wp_region() ?
After digging into the patch series I see this is more of a wrapper for boot_device_wp_region(). I feel like we should provide more consistency, though. A first reading doesn't immediately explain the relation. I do think it'd be better to add directly to boot_device.h API. Thoughts?
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/vboot/verstage... File src/security/vboot/verstage.c:
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/vboot/verstage... PS2, Line 35: #if CONFIG(BOOTMEDIA_LOCK_IN_VERSTAGE) What do we have #if guards?
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/vboot/verstage... PS2, Line 36: security_lockdown_bootmedia((void *) NULL); NULL is already void * ?