Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default to satisfy measured boot

[Bill XIE (Code Review)]

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

Change subject: drivers/pc80/rtc: Always load cmos.default to satisfy measured boot
......................................................................

drivers/pc80/rtc: Always load cmos.default to satisfy measured boot

cmos.default used to be loaded only when cmos is needed to be reset,
but conditional loading of CBFS files may break measured boot if
measurement is hooked on each loading.

In order to resolve this, loadings should be made unconditional, but
the use of loaded data remains conditional, so cmos.default should
always be loaded (with cbfs_boot_map_with_leak() which is further
hooked with measurement), but cmos resetting remains conditional.

Change-Id: If6ea0d1cbaa7d96f7dea7e77b7548ca2b30efe9e
Signed-off-by: Bill XIE 
---
M src/drivers/pc80/rtc/option.c
1 file changed, 7 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39906/1

diff --git a/src/drivers/pc80/rtc/option.c b/src/drivers/pc80/rtc/option.c
index bb697df..5bf2fc9 100644
--- a/src/drivers/pc80/rtc/option.c
+++ b/src/drivers/pc80/rtc/option.c
@@ -239,7 +239,7 @@
    return cmos_checksum_valid(LB_CKS_RANGE_START, LB_CKS_RANGE_END, LB_CKS_LOC);
 }

-static void cmos_load_defaults(void)
+void sanitize_cmos(void)
 {
    size_t length = 128;
    size_t i;
@@ -250,14 +250,10 @@
    if (!cmos_default)
    	return;

-	u8 control_state = cmos_disable_rtc();
-	for (i = 14; i < MIN(128, length); i++)
-		cmos_write_inner(cmos_default[i], i);
-	cmos_restore_rtc(control_state);
-}
-
-void sanitize_cmos(void)
-{
-	if (cmos_error() || !cmos_lb_cks_valid() || CONFIG(STATIC_OPTION_TABLE))
-		cmos_load_defaults();
+	if (cmos_error() || !cmos_lb_cks_valid() || CONFIG(STATIC_OPTION_TABLE)) {
+		u8 control_state = cmos_disable_rtc();
+		for (i = 14; i < MIN(128, length); i++)
+			cmos_write_inner(cmos_default[i], i);
+		cmos_restore_rtc(control_state);
+	}
 }

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39906 )

Change subject: drivers/pc80/rtc: Always load cmos.default to satisfy measured boot
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@7 
PS1, Line 7: Always load cmos.default to satisfy measured boot
If this is only a requirement of measured boot, why does this change make the loading of cmos.default unconditional?

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39906 )

Change subject: drivers/pc80/rtc: Always load cmos.default to satisfy measured boot
......................................................................

Patch Set 1:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@10 
PS1, Line 10: break
it doesn't break measured boot, just change the calculated PCRs

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@13 
PS1, Line 13: unconditional
that seems to be a good solution.
However this not only affects cmos.bin, but also other files like spd.bin for example.
This should be written down somewhere to make developers aware of the issue, so it wouldn't be changed back in the future.

Change in coreboot[master]: drivers/pc80/rtc: Change the logic to use cmos.default to keep PCRs

[Bill XIE (Code Review)]

Hello build bot (Jenkins), Angel Pons,

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

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

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

Change subject: drivers/pc80/rtc: Change the logic to use cmos.default to keep PCRs
......................................................................

drivers/pc80/rtc: Change the logic to use cmos.default to keep PCRs

cmos.default used to be loaded only when cmos is needed to be reset,
but conditional loading of CBFS files may change the calculated PCRs
if measurement is hooked on each loading.

In order to resolve this, loadings should be made less conditional,
(if a file might be used, it should be loaded and measured) but the
use of loaded data remains conditional.

Change-Id: If6ea0d1cbaa7d96f7dea7e77b7548ca2b30efe9e
Signed-off-by: Bill XIE 
---
M src/drivers/pc80/rtc/option.c
1 file changed, 7 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39906/2

Change in coreboot[master]: drivers/pc80/rtc: Change the logic to use cmos.default to keep PCRs

[Bill XIE (Code Review)]

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

Change subject: drivers/pc80/rtc: Change the logic to use cmos.default to keep PCRs
......................................................................

Patch Set 2:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@7 
PS1, Line 7: Always load cmos.default to satisfy measured boot
> If this is only a requirement of measured boot, why does this change make the loading of cmos. […]

Title changed. Only the logic about how to use the file is changed. When and whether to use remains the same.

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@10 
PS1, Line 10: break
> it doesn't break measured boot, just change the calculated PCRs

Done

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@13 
PS1, Line 13: unconditional
> that seems to be a good solution. […]

Where had I better to write it down?

Change in coreboot[master]: drivers/pc80/rtc: Change the logic to use cmos.default to keep PCRs

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39906 )

Change subject: drivers/pc80/rtc: Change the logic to use cmos.default to keep PCRs
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@7 
PS1, Line 7: Always load cmos.default to satisfy measured boot
> Title changed. Only the logic about how to use the file is changed. […]

The problem still remains... If this is only required for measured boot to work, please use checks for CONFIG(TPM_MEASURED_BOOT).

Change in coreboot[master]: drivers/pc80/rtc: Change the logic to use cmos.default to keep PCRs

[Bill XIE (Code Review)]

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

Change subject: drivers/pc80/rtc: Change the logic to use cmos.default to keep PCRs
......................................................................

Patch Set 4:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@7 
PS1, Line 7: Always load cmos.default to satisfy measured boot
> The problem still remains... […]

Please review the actual change.
Should I keep two logically identical implementations and switch between them with conditional preprocessing just to save a file loading?

Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

[Bill XIE (Code Review)]

Hello build bot (Jenkins), Angel Pons,

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

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

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

Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled
......................................................................

drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

cmos.default used to be loaded only when cmos is needed to be reset,
but conditional loading of CBFS files may change the calculated PCRs
if measurement is hooked on each loading.

In order to resolve this, loadings should be made less conditional,
(if a file might be used, it should be loaded and measured) but the
use of loaded data remains conditional.

Change-Id: If6ea0d1cbaa7d96f7dea7e77b7548ca2b30efe9e
Signed-off-by: Bill XIE 
---
M src/drivers/pc80/rtc/option.c
1 file changed, 16 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39906/5

Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

[Bill XIE (Code Review)]

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

Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39906/5/src/drivers/pc80/rtc/option... 
File src/drivers/pc80/rtc/option.c:

https://review.coreboot.org/c/coreboot/+/39906/5/src/drivers/pc80/rtc/option... 
PS5, Line 246: 	
Done as you wish, but I believe we do not need such complexity just to save a file loading which in coreboot is just getting a pointer to it.

Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

[Bill XIE (Code Review)]

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

Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled
......................................................................

Patch Set 5:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@7 
PS1, Line 7: Always load cmos.default to satisfy measured boot
> Please review the actual change. […]

Done

Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39906 )

Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@13 
PS1, Line 13: unconditional
> Where had I better to write it down?

The if(CONFIG(MEASURED_BOOT) ... Seems a good starting point.
For PCR precalculations it must be specified what is measured and when, but that's out of scope of this commit.

Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39906 )

Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled
......................................................................

Patch Set 5: Code-Review+1

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@7 
PS1, Line 7: Always load cmos.default to satisfy measured boot
> Done

Thank you for understanding.

Reading back, I see now that I didn't make my point clear enough. So, given what happened with CB:35077, I suppose that my -1 score looked like the beginning of a similar story and was thus not well-received. However, it was never my intention to hold this change hostage!

I only gave a -1 (which does not block submit) to express dissatisfaction with the unconditional loading, as it leaves no indication in the code that measured boot relies on always loading that file. By placing a check in the code, it provides a clue for any future readers so that they don't think it's a bug and "fix" it.

https://review.coreboot.org/c/coreboot/+/39906/5/src/drivers/pc80/rtc/option... 
File src/drivers/pc80/rtc/option.c:

https://review.coreboot.org/c/coreboot/+/39906/5/src/drivers/pc80/rtc/option... 
PS5, Line 246:
> Done as you wish, but I believe we do not need such complexity just to save a file loading which in  […]

I'd say the complexity is more or less the same as before. However, having that check provides any future readers with a hint that something needs the file to be loaded even when not used at all.

In any case, I've tried to make it simple as possible and came up with this:

void sanitize_cmos(void)
 {
    const unsigned char *cmos_default;
    const bool cmos_need_reset =
    	CONFIG(STATIC_OPTION_TABLE) || cmos_error() || !cmos_lb_cks_valid();

size_t length = 128;
    size_t i;

if (CONFIG(TPM_MEASURED_BOOT) || cmos_need_reset) {
    	cmos_default = cbfs_boot_map_with_leak("cmos.default",
    				CBFS_COMPONENT_CMOS_DEFAULT, &length);

if (!cmos_default || !cmos_need_reset)
    		return;

u8 control_state = cmos_disable_rtc();
    	for (i = 14; i < MIN(128, length); i++)
    		cmos_write_inner(cmos_default[i], i);
    	cmos_restore_rtc(control_state);
    }
 }

Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

[Bill XIE (Code Review)]

Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,

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

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

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

Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled
......................................................................

drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

cmos.default used to be loaded only when cmos is needed to be reset,
but conditional loading of CBFS files may change the calculated PCRs
if measurement is hooked on each loading.

In order to resolve this, loadings should be made less conditional,
(if a file might be used, it should be loaded and measured) but the
use of loaded data remains conditional.

Change-Id: If6ea0d1cbaa7d96f7dea7e77b7548ca2b30efe9e
Signed-off-by: Bill XIE 
---
M src/drivers/pc80/rtc/option.c
1 file changed, 18 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39906/6

Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

[Bill XIE (Code Review)]

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

Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39906/5/src/drivers/pc80/rtc/option... 
File src/drivers/pc80/rtc/option.c:

https://review.coreboot.org/c/coreboot/+/39906/5/src/drivers/pc80/rtc/option... 
PS5, Line 246:
> I'd say the complexity is more or less the same as before. […]

Thanks for refactoring the code.

Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39906 )

Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled
......................................................................

Patch Set 7: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/39906/5/src/drivers/pc80/rtc/option... 
File src/drivers/pc80/rtc/option.c:

https://review.coreboot.org/c/coreboot/+/39906/5/src/drivers/pc80/rtc/option... 
PS5, Line 246:
> Thanks for refactoring the code.

Thanks to you as well! 😄

Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39906 )

Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled
......................................................................

drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

cmos.default used to be loaded only when cmos is needed to be reset,
but conditional loading of CBFS files may change the calculated PCRs
if measurement is hooked on each loading.

In order to resolve this, loadings should be made less conditional,
(if a file might be used, it should be loaded and measured) but the
use of loaded data remains conditional.

Change-Id: If6ea0d1cbaa7d96f7dea7e77b7548ca2b30efe9e
Signed-off-by: Bill XIE 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39906
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/drivers/pc80/rtc/option.c
1 file changed, 18 insertions(+), 18 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/drivers/pc80/rtc/option.c b/src/drivers/pc80/rtc/option.c
index bb697df..dc78dbb 100644
--- a/src/drivers/pc80/rtc/option.c
+++ b/src/drivers/pc80/rtc/option.c
@@ -239,25 +239,25 @@
    return cmos_checksum_valid(LB_CKS_RANGE_START, LB_CKS_RANGE_END, LB_CKS_LOC);
 }

-static void cmos_load_defaults(void)
-{
-	size_t length = 128;
-	size_t i;
-
-	const unsigned char *cmos_default =
-		cbfs_boot_map_with_leak("cmos.default",
-				CBFS_COMPONENT_CMOS_DEFAULT, &length);
-	if (!cmos_default)
-		return;
-
-	u8 control_state = cmos_disable_rtc();
-	for (i = 14; i < MIN(128, length); i++)
-		cmos_write_inner(cmos_default[i], i);
-	cmos_restore_rtc(control_state);
-}

void sanitize_cmos(void)
 {
-	if (cmos_error() || !cmos_lb_cks_valid() || CONFIG(STATIC_OPTION_TABLE))
-		cmos_load_defaults();
+	const unsigned char *cmos_default;
+	const bool cmos_need_reset =
+		CONFIG(STATIC_OPTION_TABLE) || cmos_error() || !cmos_lb_cks_valid();
+	size_t length = 128;
+	size_t i;
+
+	if (CONFIG(TPM_MEASURED_BOOT) || cmos_need_reset) {
+		cmos_default = cbfs_boot_map_with_leak("cmos.default",
+			CBFS_COMPONENT_CMOS_DEFAULT, &length);
+
+		if (!cmos_default || !cmos_need_reset)
+			return;
+
+		u8 control_state = cmos_disable_rtc();
+		for (i = 14; i < MIN(128, length); i++)
+			cmos_write_inner(cmos_default[i], i);
+		cmos_restore_rtc(control_state);
+	}
 }

Change in coreboot[master]: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39906 )

Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled
......................................................................

Patch Set 8:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1972
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1971
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1970

Please note: This test is under development and might not be accurate at all!