Philipp Deppenwiese has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31597
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M src/lib/fmap.c M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 3 files changed, 76 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/1
diff --git a/src/lib/fmap.c b/src/lib/fmap.c index 9602134..ae0aaf8 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -20,6 +20,7 @@ #include <commonlib/fmap_serialized.h> #include <stddef.h> #include <string.h> +#include <security/vboot/vboot_crtm.h>
#include "fmap_config.h"
@@ -77,7 +78,12 @@ if (fmap_locate_area(name, &ar)) return -1;
- return boot_device_ro_subregion(&ar, area); + int ret = boot_device_ro_subregion(&ar, area); + if (!ret) + if (vboot_measure_fmap_hook(area, name)) + return -1; + + return ret; }
int fmap_locate_area_as_rdev_rw(const char *name, struct region_device *area) @@ -87,7 +93,12 @@ if (fmap_locate_area(name, &ar)) return -1;
- return boot_device_rw_subregion(&ar, area); + int ret = boot_device_rw_subregion(&ar, area); + if (!ret) + if (vboot_measure_fmap_hook(area, name)) + return -1; + + return ret; }
int fmap_locate_area(const char *name, struct region *ar) diff --git a/src/security/vboot/vboot_crtm.c b/src/security/vboot/vboot_crtm.c index 1914f20..a474fd0 100644 --- a/src/security/vboot/vboot_crtm.c +++ b/src/security/vboot/vboot_crtm.c @@ -18,6 +18,16 @@ #include <security/vboot/vboot_crtm.h> #include <security/vboot/misc.h>
+const static char *fmap_runtime_data[] = { + "UNIFIED_MRC_CACHE", + "RW_MRC_CACHE", + "RW_ELOG", + "RW_VPD", + "RW_NVRAM", + "RECOVERY_MRC_CACHE", + "RW_VAR_MRC_CACHE", + "SMMSTORE"}; + uint32_t vboot_init_crtm(void) { struct prog bootblock = PROG_INIT(PROG_BOOTBLOCK, "bootblock"); @@ -88,6 +98,36 @@ } }
+ /* IFD measurements */ + struct region_device fmap; + if (fmap_locate_area_as_rdev("RO_VPD", &fmap) == 0) + if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Read-only VPD") != TPM_SUCCESS) + return VB2_ERROR_UNKNOWN; + + if (fmap_locate_area_as_rdev("GBB", &fmap) == 0) + if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Google Binary Blob") != TPM_SUCCESS) + return VB2_ERROR_UNKNOWN; + + if (fmap_locate_area_as_rdev("SI_DESC", &fmap) == 0) + if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel Flash Descriptor") != TPM_SUCCESS) + return VB2_ERROR_UNKNOWN; + + if (fmap_locate_area_as_rdev("SI_ME", &fmap) == 0) + if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Intel ME") != TPM_SUCCESS) + return VB2_ERROR_UNKNOWN; + + if (fmap_locate_area_as_rdev("SI_EC", &fmap) == 0) + if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "EC firmware") != TPM_SUCCESS) + return VB2_ERROR_UNKNOWN; + + if (fmap_locate_area_as_rdev("SI_GBE", &fmap) == 0) + if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel GbE") != TPM_SUCCESS) + return VB2_ERROR_UNKNOWN; + + if (fmap_locate_area_as_rdev("SI_PDR", &fmap) == 0) + if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Platform Data") != TPM_SUCCESS) + return VB2_ERROR_UNKNOWN; + return VB2_SUCCESS; }
@@ -142,3 +182,18 @@ return tpm_measure_region(&rdev, pcr_index, name); } + +uint32_t vboot_measure_fmap_hook(struct region_device *rdev, const char *name) +{ + int i; + + if (!vb2_logic_executed()) + return 0; + + for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { + if (!strncmp(fmap_runtime_data[i], name, sizeof(fmap_runtime_data[i]))) + return tpm_measure_region(rdev, TPM_RUNTIME_DATA_PCR, name); + } + + return 0; +} diff --git a/src/security/vboot/vboot_crtm.h b/src/security/vboot/vboot_crtm.h index d28e96e..259e486 100644 --- a/src/security/vboot/vboot_crtm.h +++ b/src/security/vboot/vboot_crtm.h @@ -54,8 +54,16 @@ */ uint32_t vboot_measure_cbfs_hook(struct cbfsf *fh, const char *name);
+/* + * Measures fmap data via hook (fmap) + * rdev is the region device handle to measure + * return 0 if successful, else an error + */ +uint32_t vboot_measure_fmap_hook(struct region_device *rdev, const char *name); + #else #define vboot_measure_cbfs_hook(fh, name) 0 +#define vboot_measure_fmap_hook(rdev, name) 0 #endif
#endif /* __VBOOT_VBOOT_CRTM_H__ */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 1:
(11 comments)
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@104 PS1, Line 104: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Read-only VPD") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@108 PS1, Line 108: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Google Binary Blob") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@112 PS1, Line 112: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel Flash Descriptor") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@116 PS1, Line 116: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Intel ME") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@120 PS1, Line 120: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "EC firmware") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@124 PS1, Line 124: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel GbE") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@128 PS1, Line 128: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Platform Data") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@193 PS1, Line 193: for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { line over 80 characters
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@193 PS1, Line 193: for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { Prefer ARRAY_SIZE(fmap_runtime_data)
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@194 PS1, Line 194: if (!strncmp(fmap_runtime_data[i], name, sizeof(fmap_runtime_data[i]))) line over 80 characters
https://review.coreboot.org/#/c/31597/1/src/security/vboot/vboot_crtm.c@195 PS1, Line 195: return tpm_measure_region(rdev, TPM_RUNTIME_DATA_PCR, name); line over 80 characters
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#2).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M src/lib/fmap.c M src/security/tpm/Makefile.inc M src/security/vboot/Makefile.inc M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 5 files changed, 83 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 2:
(11 comments)
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@104 PS2, Line 104: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Read-only VPD") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@108 PS2, Line 108: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Google Binary Blob") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@112 PS2, Line 112: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel Flash Descriptor") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@116 PS2, Line 116: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Intel ME") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@120 PS2, Line 120: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "EC firmware") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@124 PS2, Line 124: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel GbE") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@128 PS2, Line 128: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Platform Data") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@193 PS2, Line 193: for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { line over 80 characters
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@193 PS2, Line 193: for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { Prefer ARRAY_SIZE(fmap_runtime_data)
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@194 PS2, Line 194: if (!strncmp(fmap_runtime_data[i], name, sizeof(fmap_runtime_data[i]))) line over 80 characters
https://review.coreboot.org/#/c/31597/2/src/security/vboot/vboot_crtm.c@195 PS2, Line 195: return tpm_measure_region(rdev, TPM_RUNTIME_DATA_PCR, name); line over 80 characters
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#3).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M src/lib/fmap.c M src/security/tpm/Makefile.inc M src/security/vboot/Makefile.inc M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 5 files changed, 84 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 3:
(12 comments)
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.h File src/security/vboot/vboot_crtm.h:
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.h@49 PS3, Line 49: #if (IS_ENABLED(CONFIG_VBOOT_MEASURED_BOOT) && !ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && !ENV_SMM) line over 80 characters
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@104 PS3, Line 104: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Read-only VPD") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@108 PS3, Line 108: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Google Binary Blob") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@112 PS3, Line 112: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel Flash Descriptor") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@116 PS3, Line 116: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Intel ME") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@120 PS3, Line 120: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "EC firmware") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@124 PS3, Line 124: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel GbE") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@128 PS3, Line 128: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Platform Data") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@193 PS3, Line 193: for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { line over 80 characters
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@193 PS3, Line 193: for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { Prefer ARRAY_SIZE(fmap_runtime_data)
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@194 PS3, Line 194: if (!strncmp(fmap_runtime_data[i], name, sizeof(fmap_runtime_data[i]))) line over 80 characters
https://review.coreboot.org/#/c/31597/3/src/security/vboot/vboot_crtm.c@195 PS3, Line 195: return tpm_measure_region(rdev, TPM_RUNTIME_DATA_PCR, name); line over 80 characters
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#4).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M src/lib/fmap.c M src/security/vboot/Makefile.inc M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 4 files changed, 78 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/4
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#5).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M src/lib/fmap.c M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 3 files changed, 77 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 4:
(12 comments)
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.h File src/security/vboot/vboot_crtm.h:
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.h@49 PS4, Line 49: #if (IS_ENABLED(CONFIG_VBOOT_MEASURED_BOOT) && !ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && !ENV_SMM) line over 80 characters
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@104 PS4, Line 104: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Read-only VPD") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@108 PS4, Line 108: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Google Binary Blob") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@112 PS4, Line 112: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel Flash Descriptor") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@116 PS4, Line 116: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Intel ME") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@120 PS4, Line 120: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "EC firmware") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@124 PS4, Line 124: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel GbE") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@128 PS4, Line 128: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Platform Data") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@193 PS4, Line 193: for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { line over 80 characters
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@193 PS4, Line 193: for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { Prefer ARRAY_SIZE(fmap_runtime_data)
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@194 PS4, Line 194: if (!strncmp(fmap_runtime_data[i], name, sizeof(fmap_runtime_data[i]))) line over 80 characters
https://review.coreboot.org/#/c/31597/4/src/security/vboot/vboot_crtm.c@195 PS4, Line 195: return tpm_measure_region(rdev, TPM_RUNTIME_DATA_PCR, name); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 5:
(12 comments)
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.h File src/security/vboot/vboot_crtm.h:
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.h@49 PS5, Line 49: #if (IS_ENABLED(CONFIG_VBOOT_MEASURED_BOOT) && !ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && !ENV_SMM) line over 80 characters
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@104 PS5, Line 104: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Read-only VPD") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@108 PS5, Line 108: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Google Binary Blob") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@112 PS5, Line 112: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel Flash Descriptor") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@116 PS5, Line 116: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Intel ME") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@120 PS5, Line 120: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "EC firmware") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@124 PS5, Line 124: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel GbE") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@128 PS5, Line 128: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Platform Data") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@193 PS5, Line 193: for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { line over 80 characters
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@193 PS5, Line 193: for (i = 0; i < sizeof(fmap_runtime_data) / sizeof(fmap_runtime_data[0]); i++) { Prefer ARRAY_SIZE(fmap_runtime_data)
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@194 PS5, Line 194: if (!strncmp(fmap_runtime_data[i], name, sizeof(fmap_runtime_data[i]))) line over 80 characters
https://review.coreboot.org/#/c/31597/5/src/security/vboot/vboot_crtm.c@195 PS5, Line 195: return tpm_measure_region(rdev, TPM_RUNTIME_DATA_PCR, name); line over 80 characters
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#6).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M src/lib/fmap.c M src/security/tpm/Makefile.inc M src/security/vboot/Makefile.inc M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 5 files changed, 85 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 6:
(10 comments)
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.h File src/security/vboot/vboot_crtm.h:
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.h@49 PS6, Line 49: #if (IS_ENABLED(CONFIG_VBOOT_MEASURED_BOOT) && !ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && !ENV_SMM) line over 80 characters
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.c@104 PS6, Line 104: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Read-only VPD") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.c@108 PS6, Line 108: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Google Binary Blob") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.c@113 PS6, Line 113: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel Flash Descriptor") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.c@117 PS6, Line 117: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Intel ME") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.c@121 PS6, Line 121: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "EC firmware") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.c@125 PS6, Line 125: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel GbE") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.c@129 PS6, Line 129: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Platform Data") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.c@195 PS6, Line 195: if (!strncmp(fmap_runtime_data[i], name, sizeof(fmap_runtime_data[i]))) line over 80 characters
https://review.coreboot.org/#/c/31597/6/src/security/vboot/vboot_crtm.c@196 PS6, Line 196: return tpm_measure_region(rdev, TPM_RUNTIME_DATA_PCR, name); line over 80 characters
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#7).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M src/lib/fmap.c M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 3 files changed, 78 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 7:
(10 comments)
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.h File src/security/vboot/vboot_crtm.h:
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.h@49 PS7, Line 49: #if (IS_ENABLED(CONFIG_VBOOT_MEASURED_BOOT) && !ENV_BOOTBLOCK && !ENV_DECOMPRESSOR && !ENV_SMM) line over 80 characters
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.c@104 PS7, Line 104: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Read-only VPD") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.c@108 PS7, Line 108: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Google Binary Blob") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.c@113 PS7, Line 113: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel Flash Descriptor") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.c@117 PS7, Line 117: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Intel ME") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.c@121 PS7, Line 121: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "EC firmware") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.c@125 PS7, Line 125: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, "Intel GbE") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.c@129 PS7, Line 129: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, "Platform Data") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.c@195 PS7, Line 195: if (!strncmp(fmap_runtime_data[i], name, sizeof(fmap_runtime_data[i]))) line over 80 characters
https://review.coreboot.org/#/c/31597/7/src/security/vboot/vboot_crtm.c@196 PS7, Line 196: return tpm_measure_region(rdev, TPM_RUNTIME_DATA_PCR, name); line over 80 characters
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#8).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 4 files changed, 106 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/#/c/31597/8/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/8/src/security/vboot/vboot_crtm.c@222 PS8, Line 222: if (!strncmp(fmap_runtime_data[i], name, sizeof(fmap_runtime_data[i]))) line over 80 characters
https://review.coreboot.org/#/c/31597/8/src/security/vboot/vboot_crtm.c@223 PS8, Line 223: return tpm_measure_region(rdev, TPM_RUNTIME_DATA_PCR, name); line over 80 characters
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#9).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 4 files changed, 108 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/9
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#10).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 4 files changed, 108 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/10
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/#/c/31597/11/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/11/src/security/vboot/vboot_crtm.c@139 PS11, Line 139: fmap_locate_area_as_rdev("SI_ME", &fmap) == 0) just wondering, does fmap_locate_area_as_rdev properly return non-zero when locked?
https://review.coreboot.org/#/c/31597/11/src/security/vboot/vboot_crtm.c@133 PS11, Line 133: /* IFD measurements (optional) */ : if (fmap_locate_area_as_rdev("SI_DESC", &fmap) == 0) : if (tpm_measure_region(&fmap, TPM_CRTM_PCR, : "Intel Flash Descriptor") != TPM_SUCCESS) : return VB2_ERROR_UNKNOWN; : : if (fmap_locate_area_as_rdev("SI_ME", &fmap) == 0) : if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, : "Intel ME") != TPM_SUCCESS) : return VB2_ERROR_UNKNOWN; : : if (fmap_locate_area_as_rdev("SI_EC", &fmap) == 0) : if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, : "EC firmware") != TPM_SUCCESS) : return VB2_ERROR_UNKNOWN; : : if (fmap_locate_area_as_rdev("SI_GBE", &fmap) == 0) : if (tpm_measure_region(&fmap, TPM_CRTM_PCR, : "Intel GbE") != TPM_SUCCESS) : return VB2_ERROR_UNKNOWN; : : if (fmap_locate_area_as_rdev("SI_PDR", &fmap) == 0) : if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, : "Platform Data") != TPM_SUCCESS) : return VB2_ERROR_UNKNOWN; these are Intel specific. Is is the best location or would a soc_tpm_measure_region hook be a better idea?
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#12).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 4 files changed, 106 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/12
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#13).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 4 files changed, 124 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/13
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 13:
(5 comments)
https://review.coreboot.org/#/c/31597/13/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/13/src/security/vboot/vboot_crtm.c@51 PS13, Line 51: FMAP_PREFIX "SI_EC // EC firmware") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/13/src/security/vboot/vboot_crtm.c@56 PS13, Line 56: FMAP_PREFIX "SI_GBE // Intel GbE") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/13/src/security/vboot/vboot_crtm.c@61 PS13, Line 61: FMAP_PREFIX "SI_PDR // Platform Data") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/13/src/security/vboot/vboot_crtm.c@241 PS13, Line 241: strncpy(prefix_name, FMAP_PREFIX, TCPA_PCR_HASH_NAME - 1); line over 80 characters
https://review.coreboot.org/#/c/31597/13/src/security/vboot/vboot_crtm.c@243 PS13, Line 243: name, TCPA_PCR_HASH_NAME - sizeof(FMAP_PREFIX) - 1); line over 80 characters
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#14).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 4 files changed, 126 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/14
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 14:
(5 comments)
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@53 PS14, Line 53: FMAP_PREFIX "SI_EC // EC firmware") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@58 PS14, Line 58: FMAP_PREFIX "SI_GBE // Intel GbE") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@63 PS14, Line 63: FMAP_PREFIX "SI_PDR // Platform Data") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@243 PS14, Line 243: strncpy(prefix_name, FMAP_PREFIX, TCPA_PCR_HASH_NAME - 1); line over 80 characters
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@245 PS14, Line 245: name, TCPA_PCR_HASH_NAME - sizeof(FMAP_PREFIX) - 1); line over 80 characters
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 14:
(6 comments)
https://review.coreboot.org/#/c/31597/14/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/#/c/31597/14/src/lib/fmap.c@83 PS14, Line 83: if (vboot_measure_fmap_hook(area, name)) I still don't really think this is a good overall approach for FMAP sections. FMAP sections aren't used like CBFS files and almost all contain runtime data anyway. You'll end up measuring many things twice, and you'll blow boot time like crazy because many FMAP sections aren't meant to be read in full on every boot. There's really no point in measuring things like vboot's NVRAM section anyway, it will probably have been updated by the time you finished booting.
I think a better approach would be to put an explicit call to measure something in the very few code pieces that access an FMAP section you'd actually want to measure.
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@32 PS14, Line 32: "SMMSTORE"}; (I also think you'll never be able to keep this list properly maintained. A lot of this can be platform-specific anyway. Another good reason to do this for individual instances only.)
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@36 PS14, Line 36: if (IS_ENABLED(CONFIG_SOC_INTEL_COMMON) || I think this should be factored out into some Intel-specific file (e.g. make this a weak function and implement it in src/soc/intel/common/crtm.c or something).
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@240 PS14, Line 240: sizeof(fmap_runtime_data[i]) I'm pretty sure this doesn't work... (FWIW strcmp() should be fine because both strings should be guaranteed to be null-terminated).
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@243 PS14, Line 243: strncpy(prefix_name, FMAP_PREFIX, TCPA_PCR_HASH_NAME - 1); snprintf(), like Patrick mentioned in the other CL too
https://review.coreboot.org/#/c/31597/14/src/security/vboot/vboot_crtm.c@252 PS14, Line 252: return 0; Oh, so the idea is that *only* the stuff in fmap_runtime_data is measured, and everything else (that isn't mentioned explicitly elsewhere) is not measured at all? Well, then that's all the more reason to just put the tpm_measure_region() call right into the code that deals with it, I think. Since the amount of regions is already enumerated anyway, it won't make a difference (and I think that's nicer than scanning through a list on every single access, and it makes it much easier to avoid duplicate measurements).
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 15:
(5 comments)
https://review.coreboot.org/#/c/31597/15/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/15/src/security/vboot/vboot_crtm.c@98 PS15, Line 98: FMAP_PREFIX "SI_EC // EC firmware") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/15/src/security/vboot/vboot_crtm.c@103 PS15, Line 103: FMAP_PREFIX "SI_GBE // Intel GbE") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/15/src/security/vboot/vboot_crtm.c@108 PS15, Line 108: FMAP_PREFIX "SI_PDR // Platform Data") != TPM_SUCCESS) line over 80 characters
https://review.coreboot.org/#/c/31597/15/src/security/vboot/vboot_crtm.c@300 PS15, Line 300: strncpy(prefix_name, FMAP_PREFIX, TCPA_PCR_HASH_NAME - 1); line over 80 characters
https://review.coreboot.org/#/c/31597/15/src/security/vboot/vboot_crtm.c@302 PS15, Line 302: name, TCPA_PCR_HASH_NAME - sizeof(FMAP_PREFIX) - 1); line over 80 characters
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#16).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 5 files changed, 123 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/16
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/#/c/31597/16/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/16/src/security/vboot/vboot_crtm.c@245 PS16, Line 245: if (is_runtime_data(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name)) line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/#/c/31597/17/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/17/src/security/vboot/vboot_crtm.c@245 PS17, Line 245: if (is_runtime_data(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name)) line over 80 characters
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#18).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 5 files changed, 123 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/18
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/#/c/31597/18/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/18/src/security/vboot/vboot_crtm.c@245 PS18, Line 245: if (is_runtime_data(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name)) line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/#/c/31597/19/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/19/src/security/vboot/vboot_crtm.c@245 PS19, Line 245: if (is_runtime_data(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name)) line over 80 characters
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#20).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 5 files changed, 123 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/20
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/#/c/31597/21/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/21/src/security/vboot/vboot_crtm.c@245 PS21, Line 245: if (is_runtime_data(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name)) line over 80 characters
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 21: Code-Review+1
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/#/c/31597/22/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/22/src/security/vboot/vboot_crtm.c@245 PS22, Line 245: if (is_runtime_data(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name)) line over 80 characters
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 22:
(9 comments)
https://review.coreboot.org/#/c/31597/22/Documentation/security/vboot/measur... File Documentation/security/vboot/measured_boot.md:
https://review.coreboot.org/#/c/31597/22/Documentation/security/vboot/measur... PS22, Line 22: SMM is currently excluded from measurements but will be added in a later stage. phase
https://review.coreboot.org/#/c/31597/22/Documentation/security/vboot/measur... PS22, Line 26: by measuring code before it is loaded. At the moment measurements of cbfs and CBFS
https://review.coreboot.org/#/c/31597/22/Documentation/security/vboot/measur... PS22, Line 27: FMAP content is possible. This includes the Intel IFD as well. are
https://review.coreboot.org/#/c/31597/22/Documentation/security/vboot/measur... PS22, Line 28: list which partitions could be measured
https://review.coreboot.org/#/c/31597/22/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/22/src/security/vboot/vboot_crtm.c@80 PS22, Line 80: if (fmap_locate_area_as_rdev("SI_ME", &fmap) == 0) TPM_RUNTIME_DATA_PCR
https://review.coreboot.org/#/c/31597/22/src/security/vboot/vboot_crtm.c@85 PS22, Line 85: if (fmap_locate_area_as_rdev("SI_EC", &fmap) == 0) TPM_RUNTIME_DATA_PCR
https://review.coreboot.org/#/c/31597/22/src/security/vboot/vboot_crtm.c@95 PS22, Line 95: if (fmap_locate_area_as_rdev("SI_PDR", &fmap) == 0) TPM_RUNTIME_DATA_PCR
https://review.coreboot.org/#/c/31597/22/src/security/vboot/vboot_crtm.c@116 PS22, Line 116: /* fmap measurement */ FMAP
https://review.coreboot.org/#/c/31597/22/src/security/vboot/vboot_crtm.c@267 PS22, Line 267: if (!strcmp("COREBOOT", name) || Use FMAP flags to detect a "CBFS" FMAP partition.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/#/c/31597/23/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/23/src/security/vboot/vboot_crtm.c@227 PS23, Line 227: if (is_runtime_data(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name)) line over 80 characters
Hello Werner Zeh, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#24).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 5 files changed, 136 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/24
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/#/c/31597/24/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/24/src/security/vboot/vboot_crtm.c@227 PS24, Line 227: if (is_runtime_data(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name)) line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/#/c/31597/25/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/25/src/security/vboot/vboot_crtm.c@227 PS25, Line 227: if (is_runtime_data(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name)) line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 26:
(1 comment)
https://review.coreboot.org/#/c/31597/26/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/26/src/security/vboot/vboot_crtm.c@227 PS26, Line 227: if (is_runtime_data(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name)) line over 80 characters
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 27:
(10 comments)
https://review.coreboot.org/#/c/31597/27/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/31597/27/src/security/vboot/Kconfig@43 PS27, Line 43: list The semantics of this variable are not clear. It's a list of filenames but what is done with this list?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/Kconfig@51 PS27, Line 51: list ditto
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.h File src/security/vboot/vboot_crtm.h:
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.h@63 PS27, Line 63: uint32_t What was the choice here for uint32_t return types? If it's to be consistent with other vboot return values why aren't the values commented in terms of VB2_x ?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@57 PS27, Line 57: if (fmap_locate_area_as_rdev("SI_DESC", &fmap) == 0) Why not just put these in an array with the name, string, and PCR type and loop over them?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@80 PS27, Line 80: return VB2_ERROR_UNKNOWN; There's currently no caching in fmap code. How costly is doing all these lookups?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@196 PS27, Line 196: if (!strcmp(list + i, name)) Where is the comma separate aspect dealt with in list? This looks to me that the only way possible to match is if the name is in the last part of the whitelist.
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@245 PS27, Line 245: if (!vb2_logic_executed()) Please comment as to why this check is here.
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@249 PS27, Line 249: if (!strcmp("COREBOOT", name) || We were getting flags in the fmap region metadata to indicate cbfs, right?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@257 PS27, Line 257: pcr_index = TPM_RUNTIME_DATA_PCR; Why doesn't this helper function just return the pcr index? In both cases it's the same pattern:
if (helper(...)) pcr_index = ...; else pcr_index = ...;
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@261 PS27, Line 261: snprintf(tcpa_metadata, TCPA_PCR_HASH_NAME, "FMAP: %s", name); there's no checks for remeasuring over the same region and seeing if they are the same it seems. It's just extending the pcr.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 27:
(6 comments)
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@57 PS27, Line 57: if (fmap_locate_area_as_rdev("SI_DESC", &fmap) == 0)
Why not just put these in an array with the name, string, and PCR type and loop over them?
Done
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@80 PS27, Line 80: return VB2_ERROR_UNKNOWN;
There's currently no caching in fmap code. […]
¯_(ツ)_/¯ My next patch would be adding timestamp support
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@245 PS27, Line 245: if (!vb2_logic_executed())
Please comment as to why this check is here.
Ack
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@249 PS27, Line 249: if (!strcmp("COREBOOT", name) ||
We were getting flags in the fmap region metadata to indicate cbfs, right?
mhhhhhhh? Where is the documentation?
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@257 PS27, Line 257: pcr_index = TPM_RUNTIME_DATA_PCR;
Why doesn't this helper function just return the pcr index? In both cases it's the same pattern: […]
Ack
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@261 PS27, Line 261: snprintf(tcpa_metadata, TCPA_PCR_HASH_NAME, "FMAP: %s", name);
there's no checks for remeasuring over the same region and seeing if they are the same it seems. […]
No problem. The case only exists if data is loaded more than once which is okay
Hello Werner Zeh, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#28).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/lib/fmap.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h 5 files changed, 134 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/28
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 28:
(6 comments)
https://review.coreboot.org/#/c/31597/28/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/31597/28/src/security/vboot/Kconfig@43 PS28, Line 43: list same comments as patch 27
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/27/src/security/vboot/vboot_crtm.c@249 PS27, Line 249: if (!strcmp("COREBOOT", name) ||
mhhhhhhh? Where is the documentation?
I thought that was being done? It was a real question. If it isn't then I am mistaken.
https://review.coreboot.org/#/c/31597/28/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/28/src/security/vboot/vboot_crtm.c@69 PS28, Line 69: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, All these got moved to RUNTIME_DATA_PCR while some were CRTM_PCR (SI_DESC, SI_BE)
https://review.coreboot.org/#/c/31597/28/src/security/vboot/vboot_crtm.c@179 PS28, Line 179: static uint32_t is_runtime_data(const char *list, const char *name) Probably rename this to something indicating it's now returning pcr index.
https://review.coreboot.org/#/c/31597/28/src/security/vboot/vboot_crtm.c@187 PS28, Line 187: // foo,blah,blub ?
https://review.coreboot.org/#/c/31597/28/src/security/vboot/vboot_crtm.c@191 PS28, Line 191: !memcmp(list, name, name_len)) strncmp might be better and let it not match if it encounters NUL or ','. What do you think? Seems more straight forward.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 28:
Please note that there are still comments in patch set 14 as well. Most notably the "I don't think this is a good approach in the first place" comment.
Hello Werner Zeh, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#29).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/drivers/intel/fsp1_0/fastboot_cache.c M src/drivers/mrc_cache/mrc_cache.c M src/drivers/smmstore/store.c M src/lib/cbfs.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h M src/soc/intel/apollolake/cse.c 9 files changed, 145 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/29
Hello Werner Zeh, Patrick Rudolph, Aaron Durbin, Huang Jin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#30).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/drivers/intel/fsp1_0/fastboot_cache.c M src/drivers/mrc_cache/mrc_cache.c M src/drivers/smmstore/store.c M src/lib/cbfs.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h M src/soc/intel/apollolake/cse.c 9 files changed, 138 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/30
Hello Werner Zeh, Patrick Rudolph, Aaron Durbin, Huang Jin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#31).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/drivers/intel/fsp1_0/fastboot_cache.c M src/drivers/mrc_cache/mrc_cache.c M src/drivers/smmstore/store.c M src/lib/cbfs.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h M src/soc/intel/apollolake/cse.c 9 files changed, 138 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/31
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 31:
(4 comments)
https://review.coreboot.org/#/c/31597/31/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/31/src/security/vboot/vboot_crtm.c@68 PS31, Line 68: if (fmap_locate_area_as_rdev(ifd_static[i], &fmap) == 0) { line over 80 characters
https://review.coreboot.org/#/c/31597/31/src/security/vboot/vboot_crtm.c@80 PS31, Line 80: if (fmap_locate_area_as_rdev(ifd_dynamic[i], &fmap) == 0) { line over 80 characters
https://review.coreboot.org/#/c/31597/31/src/security/vboot/vboot_crtm.c@85 PS31, Line 85: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, line over 80 characters
https://review.coreboot.org/#/c/31597/31/src/security/vboot/vboot_crtm.c@239 PS31, Line 239: pcr_index = get_pcr_index(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name); line over 80 characters
Hello Werner Zeh, Patrick Rudolph, Aaron Durbin, Huang Jin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#32).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/drivers/intel/fsp1_0/fastboot_cache.c M src/drivers/mrc_cache/mrc_cache.c M src/drivers/smmstore/store.c M src/lib/cbfs.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h M src/soc/intel/apollolake/cse.c 9 files changed, 140 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/32
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 32:
(4 comments)
https://review.coreboot.org/#/c/31597/32/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/32/src/security/vboot/vboot_crtm.c@68 PS32, Line 68: if (fmap_locate_area_as_rdev(ifd_static[i], &fmap) == 0) { line over 80 characters
https://review.coreboot.org/#/c/31597/32/src/security/vboot/vboot_crtm.c@80 PS32, Line 80: if (fmap_locate_area_as_rdev(ifd_dynamic[i], &fmap) == 0) { line over 80 characters
https://review.coreboot.org/#/c/31597/32/src/security/vboot/vboot_crtm.c@85 PS32, Line 85: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, line over 80 characters
https://review.coreboot.org/#/c/31597/32/src/security/vboot/vboot_crtm.c@239 PS32, Line 239: pcr_index = get_pcr_index(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name); line over 80 characters
Hello Werner Zeh, Patrick Rudolph, Aaron Durbin, Huang Jin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#33).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/drivers/intel/fsp1_0/fastboot_cache.c M src/drivers/mrc_cache/mrc_cache.c M src/drivers/smmstore/store.c M src/lib/cbfs.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h M src/soc/intel/apollolake/cse.c 9 files changed, 140 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/33
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 33:
(4 comments)
https://review.coreboot.org/#/c/31597/33/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/33/src/security/vboot/vboot_crtm.c@68 PS33, Line 68: if (fmap_locate_area_as_rdev(ifd_static[i], &fmap) == 0) { line over 80 characters
https://review.coreboot.org/#/c/31597/33/src/security/vboot/vboot_crtm.c@80 PS33, Line 80: if (fmap_locate_area_as_rdev(ifd_dynamic[i], &fmap) == 0) { line over 80 characters
https://review.coreboot.org/#/c/31597/33/src/security/vboot/vboot_crtm.c@85 PS33, Line 85: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, line over 80 characters
https://review.coreboot.org/#/c/31597/33/src/security/vboot/vboot_crtm.c@239 PS33, Line 239: pcr_index = get_pcr_index(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name); line over 80 characters
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 33: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31597/33/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/33/src/security/vboot/vboot_crtm.c@59 PS33, Line 59: "SI_DESC", There are configurations that are possible where the host has no read access to the descriptor (r/w access defined in the descriptor). Shall we take this into account here? Maybe check that access is possible by reading the descripor magic at offset 0x10?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 33:
(6 comments)
https://review.coreboot.org/#/c/31597/33/Documentation/security/vboot/measur... File Documentation/security/vboot/measured_boot.md:
https://review.coreboot.org/#/c/31597/33/Documentation/security/vboot/measur... PS33, Line 42: what does it mean?
https://review.coreboot.org/#/c/31597/33/Documentation/security/vboot/measur... PS33, Line 43: #### CBFS files (stages, blobs) only if EC is in same boot flash
https://review.coreboot.org/#/c/31597/33/src/drivers/intel/fsp1_0/fastboot_c... File src/drivers/intel/fsp1_0/fastboot_cache.c:
https://review.coreboot.org/#/c/31597/33/src/drivers/intel/fsp1_0/fastboot_c... PS33, Line 64: if (vboot_measure_fmap(&rdev, "RW_MRC_CACHE")) { would be possibly measured twice, once on read and once before update.
https://review.coreboot.org/#/c/31597/33/src/drivers/mrc_cache/mrc_cache.c File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/#/c/31597/33/src/drivers/mrc_cache/mrc_cache.c@3... PS33, Line 317: return -1; why does it return on error? it's not in verified mode, is it? Applies to all calls to this function.
https://review.coreboot.org/#/c/31597/33/src/drivers/smmstore/store.c File src/drivers/smmstore/store.c:
https://review.coreboot.org/#/c/31597/33/src/drivers/smmstore/store.c@103 PS33, Line 103: if (vboot_measure_fmap(&store, CONFIG_SMMSTORE_REGION)) that would cause to code to run on every call to SMM store. What happens if TPM is in use by operating system?
https://review.coreboot.org/#/c/31597/33/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/#/c/31597/33/src/lib/cbfs.c@101 PS33, Line 101: return -1; seams unrelated.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 34:
(4 comments)
https://review.coreboot.org/#/c/31597/34/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/34/src/security/vboot/vboot_crtm.c@68 PS34, Line 68: if (fmap_locate_area_as_rdev(ifd_static[i], &fmap) == 0) { line over 80 characters
https://review.coreboot.org/#/c/31597/34/src/security/vboot/vboot_crtm.c@80 PS34, Line 80: if (fmap_locate_area_as_rdev(ifd_dynamic[i], &fmap) == 0) { line over 80 characters
https://review.coreboot.org/#/c/31597/34/src/security/vboot/vboot_crtm.c@85 PS34, Line 85: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, line over 80 characters
https://review.coreboot.org/#/c/31597/34/src/security/vboot/vboot_crtm.c@239 PS34, Line 239: pcr_index = get_pcr_index(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 35:
(4 comments)
https://review.coreboot.org/#/c/31597/35/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/31597/35/src/security/vboot/vboot_crtm.c@68 PS35, Line 68: if (fmap_locate_area_as_rdev(ifd_static[i], &fmap) == 0) { line over 80 characters
https://review.coreboot.org/#/c/31597/35/src/security/vboot/vboot_crtm.c@80 PS35, Line 80: if (fmap_locate_area_as_rdev(ifd_dynamic[i], &fmap) == 0) { line over 80 characters
https://review.coreboot.org/#/c/31597/35/src/security/vboot/vboot_crtm.c@85 PS35, Line 85: if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, line over 80 characters
https://review.coreboot.org/#/c/31597/35/src/security/vboot/vboot_crtm.c@239 PS35, Line 239: pcr_index = get_pcr_index(CONFIG_VBOOT_MEASURED_BOOT_CBFS_RUNTIME_DATA, name); line over 80 characters
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 35:
This change is ready for review.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 35: Code-Review+1
(10 comments)
Thanks, I like this a lot better now! A few more nits and optional suggestions but I'm generally okay with this now.
https://review.coreboot.org/c/coreboot/+/31597/35/Documentation/security/vbo... File Documentation/security/vboot/measured_boot.md:
https://review.coreboot.org/c/coreboot/+/31597/35/Documentation/security/vbo... PS35, Line 35: * All non CBFS partitions are measured on lookup. Rewrite to match the implementation you have now?
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/intel/fsp1_0/f... File src/drivers/intel/fsp1_0/fastboot_cache.c:
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/intel/fsp1_0/f... PS35, Line 64: if (vboot_measure_fmap(&rdev, "RW_MRC_CACHE")) { nit: could also just do
if (!vboot...) { <stuff right below here> } <fall through to other error case below>
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/mrc_cache/mrc_... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/mrc_cache/mrc_... PS35, Line 316: if (vboot_measure_fmap(rdev, cr->name)) nit: I am not an expert on this code, but I think(?) the MRC_CACHE region is made up of a chain of records, and only the latest record counts (that's what mrc_cache_latest() finds). So are you sure you want to measure the whole region, and not just the stuff you're using?
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/smmstore/store... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/31597/35/src/drivers/smmstore/store... PS35, Line 103: if (vboot_measure_fmap(&store, CONFIG_SMMSTORE_REGION)) What about CONFIG_SMMSTORE_IN_CBFS?
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 55: if (IS_ENABLED(CONFIG_VBOOT_MEASURED_BOOT_IFD)) { Can't you just put this function in soc/intel/common (or maybe cpu/intel) somehow, rather than needing the Kconfig?
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 68: if (fmap_locate_area_as_rdev(ifd_static[i], &fmap) == 0) { Are you okay to silently ignore this if the region isn't found?
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 71: "FMAP: %s", ifd_static[i]); Why not just call vboot_measure_fmap() rather than implement this (twice)?
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 200: if (!whitelist_len || !name_len) nit: check is probably unnecessary now, at least the first part (and then you don't need to strlen() the whitelist)?
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 206: memcmp This should be a strncmp() to make sure you don't overrun the whitelist (because memcmp() is allowed to compare more than one byte at a time).
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 253: if (!vboot_logic_executed()) nit: I'd consider making this an assert() instead. Since this is not a hook but called explicitly, you really never want anything to call it before verstage, and if anything does you'll want to notice.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 35:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/c/coreboot/+/31597/35/src/security/vboot/vboot_c... PS35, Line 67: for (i = 0; i < ARRAY_SIZE(ifd_static); i++) { : if (fmap_locate_area_as_rdev(ifd_static[i], &fmap) == 0) { : char log_string[TCPA_PCR_HASH_NAME]; : snprintf(log_string, TCPA_PCR_HASH_NAME, : "FMAP: %s", ifd_static[i]); : : if (tpm_measure_region(&fmap, TPM_CRTM_PCR, : log_string) != TPM_SUCCESS) : return VB2_ERROR_UNKNOWN; : } : } : : for (i = 0; i < ARRAY_SIZE(ifd_dynamic); i++) { : if (fmap_locate_area_as_rdev(ifd_dynamic[i], &fmap) == 0) { : char log_string[TCPA_PCR_HASH_NAME]; : snprintf(log_string, TCPA_PCR_HASH_NAME, : "FMAP: %s", ifd_dynamic[i]); : : if (tpm_measure_region(&fmap, TPM_RUNTIME_DATA_PCR, : log_string) != TPM_SUCCESS) : return VB2_ERROR_UNKNOWN; : } : } These two for-loops do exactly the same except the PCR where the region is measured to. You could use a struct with region name and PCR pairs, then use just one ifd_... array of this struct and get rid of the second for-loop. Will make the code nicer and smaller in my point of view.
Hello Werner Zeh, Patrick Rudolph, Aaron Durbin, Julius Werner, Huang Jin, Christian Walter, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#37).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/drivers/mrc_cache/mrc_cache.c M src/drivers/smmstore/store.c M src/lib/cbfs.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h M src/soc/intel/apollolake/cse.c 8 files changed, 134 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/37
Hello Werner Zeh, Patrick Rudolph, Aaron Durbin, Julius Werner, Huang Jin, Christian Walter, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#38).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/drivers/mrc_cache/mrc_cache.c M src/drivers/smmstore/store.c M src/lib/cbfs.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h M src/soc/intel/apollolake/cse.c 8 files changed, 134 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/38
Hello Werner Zeh, Patrick Rudolph, Aaron Durbin, Julius Werner, Huang Jin, Christian Walter, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31597
to look at the new patch set (#39).
Change subject: security/vboot: Add fmap measurements ......................................................................
security/vboot: Add fmap measurements
* Hook into fmap location * Add static measurements for IFD to the CRTM
Change-Id: If7e4972805fbc8d19ab55d1f5e506836791c7bf0 Signed-off-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/security/vboot/measured_boot.md M src/drivers/mrc_cache/mrc_cache.c M src/drivers/smmstore/store.c M src/lib/cbfs.c M src/security/vboot/Kconfig M src/security/vboot/vboot_crtm.c M src/security/vboot/vboot_crtm.h M src/soc/intel/apollolake/cse.c 8 files changed, 135 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/31597/39
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31597 )
Change subject: security/vboot: Add fmap measurements ......................................................................
Patch Set 39:
(5 comments)
https://review.coreboot.org/c/coreboot/+/31597/39/src/drivers/smmstore/store... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/31597/39/src/drivers/smmstore/store... PS39, Line 114: int smmstore_read_region(void *buf, ssize_t *bufsize) that's currently only called in SMM
https://review.coreboot.org/c/coreboot/+/31597/39/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/31597/39/src/lib/cbfs.c@99 PS39, Line 99: if (vboot_measure_cbfs_hook(fh, name)) that has nothing to do with fmaps
https://review.coreboot.org/c/coreboot/+/31597/39/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/31597/39/src/security/vboot/Kconfig... PS39, Line 64: This includes the IFD, GbE, ME, EC and Platform Data regions. that are only a few partitions, the IFD has more. Why is EC measured? How does it influence the CRT? Why is GbE measured? How does it influence the CRT?
https://review.coreboot.org/c/coreboot/+/31597/39/src/security/vboot/vboot_c... File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/c/coreboot/+/31597/39/src/security/vboot/vboot_c... PS39, Line 61: "SI_PDR"}; why is PDR static?
https://review.coreboot.org/c/coreboot/+/31597/39/src/security/vboot/vboot_c... PS39, Line 73: if (tpm_measure_region(&fmap, TPM_CRTM_PCR, you need to check if the partition can be read from SPI master "host CPU". The ME region might not be accessible at all.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31597?usp=email )
Change subject: security/vboot: Add fmap measurements ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)
-
Martin L Roth (Code Review)
-
Patrick Rudolph (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Werner Zeh (Code Review)