Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29547 )
Change subject: security/vboot: Add measured boot mode ......................................................................
Patch Set 63:
(5 comments)
Still a lot of line length issues and whitespace fixes that have nothing to do with this CL, I think those need to be resolved before this can go in.
https://review.coreboot.org/#/c/29547/63/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/#/c/29547/63/src/lib/prog_loaders.c@51 PS63, Line 51: if (vboot_measure_prog_hook(prog)) I still don't understand why we don't just cover progs under vboot_measure_cbfs_hook(). Doesn't that make it way simpler? You can still pick the right PCR based on CBFS type (TYPE_STAGE, TYPE_SELF and TYPE_FIT into one bucket, everything else into the other).
https://review.coreboot.org/#/c/29547/63/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/29547/63/src/security/vboot/Kconfig@42 PS63, Line 42: list This doesn't seem like such a great idea to me... everyone will end up measuring different stuff. If anything, I think you might rather want a blacklist instead of a whitelist, and you'd probably want the default supplied in SoC Kconfigs rather than being user configurable.
But it's your feature, so if you want it this way that's fine by me too.
https://review.coreboot.org/#/c/29547/63/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/29547/63/src/security/vboot/vboot_crtm.c@91 PS63, Line 91: static bool is_runtime_data(const char *name) nit: maybe better to call it runtime_data_should_measure(), because it *is* all runtime data, you just don't want to measure it all.
https://review.coreboot.org/#/c/29547/63/src/security/vboot/vboot_crtm.c@94 PS63, Line 94: size_t whitelist_len = strlen(whitelist); nit: sizeof(CONFIG_...) - 1 could be evaluated at compile time, strlen() will (I believe) count again on every call.
https://review.coreboot.org/#/c/29547/63/src/security/vboot/vboot_crtm.c@102 PS63, Line 102: if (!strncmp(whitelist + i, name, name_len)) nit: strncmp() kinda unnecesary, since you strlen()ed the name anyway, might as well just be strcmp().
-
Julius Werner (Code Review)