Change in coreboot[master]: security/tpm: Include mrc.bin in CRTM if present

[Bill XIE (Code Review)]

Bill XIE has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

security/tpm: Include mrc.bin in CRTM if present

mrc.bin, on platforms where it presents, is code executed on CPU, so
it should be considered a part of CRTM.

cbfs_locate_file_in_region() is hooked to measurement here too, since
mrc.bin is loaded with it, and CBFS_TYPE_MRC (the type of mrc.bin) is
measured to TPM_CRTM_PCR rather than TPM_RUNTIME_DATA_PCR.

TODO: I have heard that SMM is too resource-limited to link with vboot
library, so currently tspi_measure_cbfs_hook() is masked in SMM.
Please correct me if I am wrong.

Change-Id: Ib4c3cf47b919864056baf725001ca8a4aaafa110
Signed-off-by: Bill XIE 
---
M src/lib/cbfs.c
M src/security/tpm/tspi/crtm.c
M src/security/tpm/tspi/crtm.h
3 files changed, 16 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/38858/1

diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 517f59d..f9d9af4 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -58,7 +58,11 @@
    	 * Files can be added to the RO_REGION_ONLY config option to use this feature.
    	 */
    	printk(BIOS_DEBUG, "Fall back to RO region for %s\n", name);
-		ret = cbfs_locate_file_in_region(fh, "COREBOOT", name, type);
+		if (fmap_locate_area_as_rdev("COREBOOT", &rdev))
+			ERROR("RO region not found\n");
+		else
+			ret = cbfs_locate(fh, &rdev, name, type);
+
    }

if (!ret)
@@ -95,7 +99,11 @@
    	return -1;
    }

-	return cbfs_locate(fh, &rdev, name, type);
+	int ret = cbfs_locate(fh, &rdev, name, type);
+	if (!ret)
+		if (tspi_measure_cbfs_hook(fh, name))
+			return -1;
+	return ret;
 }

size_t cbfs_load_and_decompress(const struct region_device *rdev, size_t offset,
diff --git a/src/security/tpm/tspi/crtm.c b/src/security/tpm/tspi/crtm.c
index 89c53a1..1c5438d 100644
--- a/src/security/tpm/tspi/crtm.c
+++ b/src/security/tpm/tspi/crtm.c
@@ -133,10 +133,14 @@
    cbfs_file_data(&rdev, fh);

switch (cbfs_type) {
-	case CBFS_TYPE_MRC:
    case CBFS_TYPE_MRC_CACHE:
    	pcr_index = TPM_RUNTIME_DATA_PCR;
    	break;
+	/*
+	 * mrc.bin is code executed on CPU, so it
+	 * should not be considered runtime data
+	 */
+	case CBFS_TYPE_MRC:
    case CBFS_TYPE_STAGE:
    case CBFS_TYPE_SELF:
    case CBFS_TYPE_FIT:
diff --git a/src/security/tpm/tspi/crtm.h b/src/security/tpm/tspi/crtm.h
index 2c2f18b..8899498 100644
--- a/src/security/tpm/tspi/crtm.h
+++ b/src/security/tpm/tspi/crtm.h
@@ -46,7 +46,7 @@
  */
 uint32_t tspi_init_crtm(void);

-#if CONFIG(TPM_MEASURED_BOOT)
+#if !ENV_SMM && CONFIG(TPM_MEASURED_BOOT)
 /*
  * Measures cbfs data via hook (cbfs)
  * fh is the cbfs file handle to measure

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

Patch Set 1: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

Patch Set 1:

(4 comments)

https://review.coreboot.org/c/coreboot/+/38858/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38858/1//COMMIT_MSG@9 
PS1, Line 9: where it presents
*where it is present* or *where present*

https://review.coreboot.org/c/coreboot/+/38858/1/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/38858/1/src/lib/cbfs.c@61 
PS1, Line 61: 		if (fmap_locate_area_as_rdev("COREBOOT", &rdev))
Why do you use the FMAP method?

https://review.coreboot.org/c/coreboot/+/38858/1/src/lib/cbfs.c@65 
PS1, Line 65: 
Please remove the blank line.

https://review.coreboot.org/c/coreboot/+/38858/1/src/lib/cbfs.c@102 
PS1, Line 102: int ret
I think this should be declared at the top.

[Bill XIE (Code Review)]

Hello Julius Werner, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38858

to look at the new patch set (#2).

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

security/tpm: Include mrc.bin in CRTM if present

mrc.bin, on platforms where it is present, is code executed on CPU, so
it should be considered a part of CRTM.

cbfs_locate_file_in_region() is hooked to measurement here too, since
mrc.bin is loaded with it, and CBFS_TYPE_MRC (the type of mrc.bin) is
measured to TPM_CRTM_PCR rather than TPM_RUNTIME_DATA_PCR.

TODO: I have heard that SMM is too resource-limited to link with vboot
library, so currently tspi_measure_cbfs_hook() is masked in SMM.
Please correct me if I am wrong.

Change-Id: Ib4c3cf47b919864056baf725001ca8a4aaafa110
Signed-off-by: Bill XIE 
---
M src/lib/cbfs.c
M src/security/tpm/tspi/crtm.c
M src/security/tpm/tspi/crtm.h
3 files changed, 16 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/38858/2

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/c/coreboot/+/38858/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38858/1//COMMIT_MSG@9 
PS1, Line 9: where it presents
> *where it is present* or *where present*

Done

https://review.coreboot.org/c/coreboot/+/38858/1/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/38858/1/src/lib/cbfs.c@61 
PS1, Line 61: 		if (fmap_locate_area_as_rdev("COREBOOT", &rdev))
> Why do you use the FMAP method?

It is originally done by cbfs_locate_file_in_region(), whose behavior I just mirrored here.

https://review.coreboot.org/c/coreboot/+/38858/1/src/lib/cbfs.c@65 
PS1, Line 65:
> Please remove the blank line.

Done

https://review.coreboot.org/c/coreboot/+/38858/1/src/lib/cbfs.c@102 
PS1, Line 102: int ret
> I think this should be declared at the top.

Done

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38858/5/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/38858/5/src/security/tpm/tspi/crtm.... 
PS5, Line 206: 	/*
The MRC cache is runtime data because it can change any time between reboot cycles which would break e.g. sealing of credentials

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38858/5/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/38858/5/src/security/tpm/tspi/crtm.... 
PS5, Line 206: 	/*
> The MRC cache is runtime data because it can change any time between reboot cycles which would break […]

I know. I mean while MRC cache is runtime data, the mrc.bin is not, and it should be treated as a part of CRTM.

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

Patch Set 5: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/38858/5/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/38858/5/src/security/tpm/tspi/crtm.... 
PS5, Line 206: 	/*
> I know. I mean while MRC cache is runtime data, the mrc. […]

Ack

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38858/1/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/38858/1/src/lib/cbfs.c@61 
PS1, Line 61: 		if (fmap_locate_area_as_rdev("COREBOOT", &rdev))
> It is originally done by cbfs_locate_file_in_region(), whose behavior I just mirrored here.

Looks good to me. We just hook cbfs_locate afaik

https://review.coreboot.org/c/coreboot/+/38858/5/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/38858/5/src/security/tpm/tspi/crtm.... 
PS5, Line 206: 	/*
> Ack

Done

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

Patch Set 6: Code-Review+2

[Bill XIE (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38858

to look at the new patch set (#8).

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

security/tpm: Include mrc.bin in CRTM if present

mrc.bin, on platforms where it is present, is code executed on CPU, so
it should be considered a part of CRTM.

cbfs_locate_file_in_region() is hooked to measurement here too, since
mrc.bin is loaded with it, and CBFS_TYPE_MRC (the type of mrc.bin) is
measured to TPM_CRTM_PCR rather than TPM_RUNTIME_DATA_PCR.

TODO: I have heard that SMM is too resource-limited to link with vboot
library, so currently tspi_measure_cbfs_hook() is masked in SMM.
Please correct me if I am wrong.

Change-Id: Ib4c3cf47b919864056baf725001ca8a4aaafa110
Signed-off-by: Bill XIE 
---
M src/lib/cbfs.c
M src/security/tpm/tspi/crtm.c
M src/security/tpm/tspi/crtm.h
3 files changed, 16 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/38858/8

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

Patch Set 9: Code-Review+2

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38858 )

Change subject: security/tpm: Include mrc.bin in CRTM if present
......................................................................

security/tpm: Include mrc.bin in CRTM if present

mrc.bin, on platforms where it is present, is code executed on CPU, so
it should be considered a part of CRTM.

cbfs_locate_file_in_region() is hooked to measurement here too, since
mrc.bin is loaded with it, and CBFS_TYPE_MRC (the type of mrc.bin) is
measured to TPM_CRTM_PCR rather than TPM_RUNTIME_DATA_PCR.

TODO: I have heard that SMM is too resource-limited to link with vboot
library, so currently tspi_measure_cbfs_hook() is masked in SMM.
Please correct me if I am wrong.

Change-Id: Ib4c3cf47b919864056baf725001ca8a4aaafa110
Signed-off-by: Bill XIE 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38858
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M src/lib/cbfs.c
M src/security/tpm/tspi/crtm.c
M src/security/tpm/tspi/crtm.h
3 files changed, 16 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 4392ab7..ccd7e6a 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -56,7 +56,10 @@
    	 * Files can be added to the RO_REGION_ONLY config option to use this feature.
    	 */
    	printk(BIOS_DEBUG, "Fall back to RO region for %s\n", name);
-		ret = cbfs_locate_file_in_region(fh, "COREBOOT", name, type);
+		if (fmap_locate_area_as_rdev("COREBOOT", &rdev))
+			ERROR("RO region not found\n");
+		else
+			ret = cbfs_locate(fh, &rdev, name, type);
    }

if (!ret)
@@ -86,14 +89,18 @@
    		       const char *name, uint32_t *type)
 {
    struct region_device rdev;
-
+	int ret = 0;
    if (fmap_locate_area_as_rdev(region_name, &rdev)) {
    	LOG("%s region not found while looking for %s\n",
    	    region_name, name);
    	return -1;
    }

-	return cbfs_locate(fh, &rdev, name, type);
+	ret = cbfs_locate(fh, &rdev, name, type);
+	if (!ret)
+		if (tspi_measure_cbfs_hook(fh, name))
+			return -1;
+	return ret;
 }

size_t cbfs_load_and_decompress(const struct region_device *rdev, size_t offset,
diff --git a/src/security/tpm/tspi/crtm.c b/src/security/tpm/tspi/crtm.c
index dc7d7d2..304cea3 100644
--- a/src/security/tpm/tspi/crtm.c
+++ b/src/security/tpm/tspi/crtm.c
@@ -133,10 +133,14 @@
    cbfs_file_data(&rdev, fh);

switch (cbfs_type) {
-	case CBFS_TYPE_MRC:
    case CBFS_TYPE_MRC_CACHE:
    	pcr_index = TPM_RUNTIME_DATA_PCR;
    	break;
+	/*
+	 * mrc.bin is code executed on CPU, so it
+	 * should not be considered runtime data
+	 */
+	case CBFS_TYPE_MRC:
    case CBFS_TYPE_STAGE:
    case CBFS_TYPE_SELF:
    case CBFS_TYPE_FIT:
diff --git a/src/security/tpm/tspi/crtm.h b/src/security/tpm/tspi/crtm.h
index dfd91e1..eb62495 100644
--- a/src/security/tpm/tspi/crtm.h
+++ b/src/security/tpm/tspi/crtm.h
@@ -50,7 +50,7 @@
  */
 int tspi_measure_cache_to_pcr(void);

-#if CONFIG(TPM_MEASURED_BOOT)
+#if !ENV_SMM && CONFIG(TPM_MEASURED_BOOT)
 /*
  * Measures cbfs data via hook (cbfs)
  * fh is the cbfs file handle to measure