Christian Walter 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.
Isnt it enough to explain in the details that we also added the ability to choose the *when*?
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. […]
It's more or less only a wrapper true. I can explain it in more detail in the commit message. But still I would rather not put it directly into boot_device.h. I like functions being implemented where the .h file is. Just my personal 2 cents. :)
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?
Changed it to "normal" if CONFIG(..).
https://review.coreboot.org/c/coreboot/+/32705/2/src/security/vboot/verstage... PS2, Line 36: security_lockdown_bootmedia((void *) NULL);
NULL is already void * ?
Ack