Change in ...coreboot[master]: drivers/tpm: remove initialization call from Intel FSP2.0

[Joel Kitching (Code Review)]

Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31837

Change subject: drivers/tpm: remove initialization call from Intel FSP2.0
......................................................................

drivers/tpm: remove initialization call from Intel FSP2.0

Remove extraneous call to tpm_setup from Intel FSP memory
initialization.

* For CONFIG_VBOOT=n devices, src/drivers/tpm/tpm.c takes care of
  initializing TPM (see Kconfig option TPM_INIT).
* For CONFIG_VBOOT=y devices, TPM will be initialized whenever
  verstage is executed, depending on how the device is configured
  (VBOOT_STARTS_IN_BOOTBLOCK or VBOOT_STARTS_IN_ROMSTAGE).

See bug for more information:
https://bugs.chromium.org/p/chromium/issues/detail?id=940377

BUG=chromium:940377
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: I4ba91c275c33245be61041cb592e52f861dbafe6
Signed-off-by: Joel Kitching 
---
M src/drivers/intel/fsp2_0/memory_init.c
1 file changed, 0 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/31837/1

diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index 2002c11..04af4a0 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -92,14 +92,6 @@

/* Create romstage handof information */
    romstage_handoff_init(s3wake);
-
-	/*
-	 * Initialize the TPM, unless the TPM was already initialized
-	 * in verstage and used to verify romstage.
-	 */
-	if ((IS_ENABLED(CONFIG_TPM1) || IS_ENABLED(CONFIG_TPM2)) &&
-	    !IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK))
-		tpm_setup(s3wake);
 }

static void fsp_fill_mrc_cache(FSPM_ARCH_UPD *arch_upd, uint32_t fsp_version)

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/tpm: remove initialization call from Intel FSP2.0
......................................................................

Patch Set 1: Code-Review+1

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/tpm: remove initialization call from Intel FSP2.0
......................................................................

Patch Set 1:

Great catch!. I missed that

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/tpm: remove initialization call from Intel FSP2.0
......................................................................

Patch Set 1: Code-Review+2

[Julius Werner (Code Review)]

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

Change subject: drivers/tpm: remove initialization call from Intel FSP2.0
......................................................................

Patch Set 1: Code-Review+2

[Joel Kitching (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: drivers/tpm: remove initialization call from Intel FSP2.0
......................................................................

drivers/tpm: remove initialization call from Intel FSP2.0

Remove extraneous call to tpm_setup from Intel FSP memory
initialization.

* For CONFIG_VBOOT=n devices, src/drivers/tpm/tpm.c takes care of
  initializing TPM (see Kconfig option TPM_INIT).
* For CONFIG_VBOOT=y devices, TPM will be initialized whenever
  verstage is executed, depending on how the device is configured
  (VBOOT_STARTS_IN_BOOTBLOCK or VBOOT_STARTS_IN_ROMSTAGE).

See bug for more information:
https://bugs.chromium.org/p/chromium/issues/detail?id=940377

BUG=chromium:940377
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: I4ba91c275c33245be61041cb592e52f861dbafe6
Signed-off-by: Joel Kitching 
---
M src/drivers/intel/fsp2_0/memory_init.c
1 file changed, 0 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/31837/2

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/tpm: remove initialization call from Intel FSP2.0
......................................................................

Patch Set 2: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/tpm: remove initialization call from Intel FSP2.0
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG@12 
PS2, Line 12: src/drivers/tpm/tpm.c takes care of
            :   initializing TPM (see Kconfig option TPM_INIT).
So, were the devices getting initialized twice currently if VBOOT is not selected -- i.e. with TPM_INIT tpm_setup was done in romstage as well as in ramstage?

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG@14 
PS2, Line 14:  TPM will be initialized whenever
            :   verstage is executed, depending on how the device is configured
There is actually one case which would fail with or without this change. When using VBOOT and RECOVERY_MRC_CACHE, tpm read/write occurs before tpm_setup with VBOOT_STARTS_IN_ROMSTAGE. So, that would fail since tpm_setup is not yet called.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/tpm: remove initialization call from Intel FSP2.0
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG@14 
PS2, Line 14:  TPM will be initialized whenever
            :   verstage is executed, depending on how the device is configured
> There is actually one case which would fail with or without this change. […]

I am okay if you want to just add a condition saying that RECOVERY_MRC_CACHE hash is maintained in TPM only if VBOOT_STARTS_IN_BOOTBLOCK. I am not sure if there any actual users of recovery mrc cache and VBOOT_STARTS_IN_ROMSTAGE.

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/tpm: remove initialization call from Intel FSP2.0
......................................................................

Patch Set 2: Code-Review-1

First consider Furquan's comments.

Change in ...coreboot[master]: drivers/tpm: update TPM initialization logic for Intel FSP2.0

[Joel Kitching (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: drivers/tpm: update TPM initialization logic for Intel FSP2.0
......................................................................

drivers/tpm: update TPM initialization logic for Intel FSP2.0

Intel FSP memory initialization needs to access the TPM when
HAS_RECOVERY_MRC_CACHE is enabled.

* Update its tpm_setup call to include HAS_RECOVERY_MRC_CACHE
  selection as a condition.
* Update vboot's TPM initialization to bypass tpm_setup
  when HAS_RECOVERY_MRC_CACHE is selected.

See bug for more information:
https://bugs.chromium.org/p/chromium/issues/detail?id=940377

BUG=chromium:940377
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: I4ba91c275c33245be61041cb592e52f861dbafe6
Signed-off-by: Joel Kitching 
---
M src/drivers/intel/fsp2_0/memory_init.c
M src/security/vboot/secdata_tpm.c
2 files changed, 12 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/31837/3

Change in ...coreboot[master]: drivers/tpm: update TPM initialization logic for Intel FSP2.0

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/tpm: update TPM initialization logic for Intel FSP2.0
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG@12 
PS2, Line 12: src/drivers/tpm/tpm.c takes care of
            :   initializing TPM (see Kconfig option TPM_INIT).
> So, were the devices getting initialized twice currently if VBOOT is not selected -- i.e. […]

I guess we are going to run into more problems with !VBOOT if we leave this code in here...
What about the suggestion in the comment below about a new Kconfig flag?

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG@14 
PS2, Line 14:  TPM will be initialized whenever
            :   verstage is executed, depending on how the device is configured
> I am okay if you want to just add a condition saying that RECOVERY_MRC_CACHE hash is maintained in T […]

Thanks for letting me know about this -- I had no idea that RECOVERY_MRC_CACHE triggered TPM read/writes.

Since I'd like to keep some devices working with STARTS_IN_BOOTBLOCK for test purposes, how about fixing it to work?  (See latest patchset.)

Alternatively, we could also introduce a more general Kconfig option... I was thinking TPM_EARLY_INIT might be a good pair for TPM_INIT.  We can specify that they are mutually exclusive.

* TPM_EARLY_INIT: Something earlier than the ramstage TPM init code starts up the TPM.
* TPM_INIT: Ramstage TPM init code starts up the TPM.
* [Neither, but with VBOOT]: Vboot takes care of it when neither of these flags are enabled.
* [None]: TPM is not enabled.

Change in ...coreboot[master]: drivers/tpm: update TPM initialization logic for Intel FSP2.0

[Patrick Rudolph (Code Review)]

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

Change subject: drivers/tpm: update TPM initialization logic for Intel FSP2.0
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c@453 
PS3, Line 453: 	    CONFIG(VBOOT_STARTS_IN_ROMSTAGE))
there are 0 platforms that have VBOOT_STARTS_IN_ROMSTAGE and FSP2.0

Change in ...coreboot[master]: drivers/tpm: update TPM initialization logic for Intel FSP2.0

[Julius Werner (Code Review)]

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

Change subject: drivers/tpm: update TPM initialization logic for Intel FSP2.0
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG@14 
PS2, Line 14:  TPM will be initialized whenever
            :   verstage is executed, depending on how the device is configured
> Thanks for letting me know about this -- I had no idea that RECOVERY_MRC_CACHE triggered TPM read/wr […]

I think I would prefer what Furquan said -- just don't support RECOVERY_MRC_CACHE with STARTS_IN_ROMSTAGE. It would add a bunch of complexity and we have no real use case for it. For the purposes of testing STARTS_IN_ROMSTAGE, you can just deactivate RECOVERY_MRC_CACHE on those boards... they should still boot just fine, they'll just take a little longer to do it in recovery mode.

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c@453 
PS3, Line 453: 	    CONFIG(VBOOT_STARTS_IN_ROMSTAGE))
> there are 0 platforms that have VBOOT_STARTS_IN_ROMSTAGE and FSP2. […]

True but I'm not sure that needs to be cemented as a hard requirement? I think one of Joel's goals here is to be able to change a newer board to STARTS_IN_ROMSTAGE for development, because we're running out of easily accessible old boards that actually use it and so it never gets tested anymore.

I guess the other option would be to kill the STARTS_IN_ROMSTAGE option entirely, but I'm not sure if we're willing to do that yet.

Change in ...coreboot[master]: drivers/tpm: update TPM initialization logic for Intel FSP2.0

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/tpm: update TPM initialization logic for Intel FSP2.0
......................................................................

Patch Set 3:

Just a short question. Why is the MRC cache bound to the TPM?? Is there any specific security mechanism involved. The first time I looked at the code I got the impression we can do better storing stuff in the spi flash

Change in ...coreboot[master]: drivers/tpm: update TPM initialization logic for Intel FSP2.0

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/tpm: update TPM initialization logic for Intel FSP2.0
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c@453 
PS3, Line 453: 	    CONFIG(VBOOT_STARTS_IN_ROMSTAGE))
> True but I'm not sure that needs to be cemented as a hard requirement? I think one of Joel's goals h […]

I would recommend to kill the support which is used by multiple platforms atm

Change in ...coreboot[master]: drivers/tpm: update TPM initialization logic for Intel FSP2.0

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/tpm: update TPM initialization logic for Intel FSP2.0
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c@453 
PS3, Line 453: 	    CONFIG(VBOOT_STARTS_IN_ROMSTAGE))
> I would recommend to kill the support which is used by multiple platforms atm

I wouldn't recommend killing the support which is used by multiple platforms btw

Change in ...coreboot[master]: drivers/tpm: update TPM initialization logic for Intel FSP2.0

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/tpm: update TPM initialization logic for Intel FSP2.0
......................................................................

Patch Set 3: Code-Review-2

Let's find a better solution by removing the TPM dependency

Change in ...coreboot[master]: drivers/tpm: update TPM initialization logic for Intel FSP2.0

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/tpm: update TPM initialization logic for Intel FSP2.0
......................................................................

Patch Set 3: Code-Review-1

Change in ...coreboot[master]: drivers/tpm: update TPM initialization logic for Intel FSP2.0

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/tpm: update TPM initialization logic for Intel FSP2.0
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG@14 
PS2, Line 14:  TPM will be initialized whenever
            :   verstage is executed, depending on how the device is configured
> I think I would prefer what Furquan said -- just don't support RECOVERY_MRC_CACHE with STARTS_IN_ROM […]

Correct. Even without any change, currently, it would do the memory retraining in recovery mode. You would see some extra error messages indicating failure to read hash, but probably not too bad.

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Joel Kitching (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

When VBOOT_STARTS_IN_BOOTBLOCK is selected, the tpm_setup call
in memory_init.c is not used.

When VBOOT_STARTS_IN_ROMSTAGE is selected, the tpm_setup call
in memory_init.c is triggered.  However, when verstage runs,
tpm_setup is called yet again, and an error is triggered from
the multiple initialization calls.

Since there are currently no boards using
VBOOT_STARTS_IN_ROMSTAGE + FSP2_0_USES_TPM_MRC_HASH, disable
this combination via Kconfig, and remove the tpm_setup call
from Intel FSP memory initializion code.

* VBOOT=y VBOOT_STARTS_IN_BOOTBLOCK=y
  vboot is enabled, and TPM is setup prior to Intel FSP memory
  initialization.  Allow FSP2_0_USES_TPM_MRC_HASH option.

* VBOOT=y VBOOT_STARTS_IN_BOOTBLOCK=n
  vboot is enabled, but TPM is setup in romstage, after Intel
  FSP memory initialization.  Disallow FSP2_0_USES_TPM_MRC_HASH
  option.

* VBOOT=n
  vboot is disabled.  Disallow FSP2_0_USES_TPM_MRC_HASH option.

See bug for more information:
https://bugs.chromium.org/p/chromium/issues/detail?id=940377

BUG=chromium:940377
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: I4ba91c275c33245be61041cb592e52f861dbafe6
Signed-off-by: Joel Kitching 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/memory_init.c
2 files changed, 10 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/31837/4

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

Patch Set 4:

(2 comments)

We actually have two options here:

* When VBOOT_STARTS_IN_ROMSTAGE is enabled, disallow USE_RECOVERY_MRC_CACHE.
  AFAIK this would regenerate the recovery MRC training data on each recovery mode boot.
  Pro: slow.  Con: safe.

* When VBOOT_STARTS_IN_ROMSTAGE is enabled, disallow FSP2_0_USES_TPM_MRC_HASH.
  This would use the recovery MRC cache as normal, without the save-hash-in-TPM functionality.
  Pro: fast.  Con: less safe.

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31837/2//COMMIT_MSG@14 
PS2, Line 14:  TPM will be initialized whenever
            :   verstage is executed, depending on how the device is configured
> Correct. Even without any change, currently, it would do the memory retraining in recovery mode. […]

OK, let's just disable the combination of Kconfig options as suggested.

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c@453 
PS3, Line 453: 	    CONFIG(VBOOT_STARTS_IN_ROMSTAGE))
> I wouldn't recommend killing the support which is used by multiple platforms btw

I think we still need to consider killing VBOOT_STARTS_IN_ROMSTAGE.  It causes many edge cases that are a hassle to handle, given that this option is not used for any of our modern boards.

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Joel Kitching (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

When VBOOT_STARTS_IN_BOOTBLOCK is selected, the tpm_setup call
in memory_init.c is not used.

When VBOOT_STARTS_IN_ROMSTAGE is selected, the tpm_setup call
in memory_init.c is triggered.  However, when verstage runs,
tpm_setup is called yet again, and an error is triggered from
the multiple initialization calls.

Since there are currently no boards using
VBOOT_STARTS_IN_ROMSTAGE + FSP2_0_USES_TPM_MRC_HASH, disable
this combination via Kconfig, and remove the tpm_setup call
from Intel FSP memory initializion code.

* VBOOT=y VBOOT_STARTS_IN_BOOTBLOCK=y
  vboot is enabled, and TPM is setup prior to Intel FSP memory
  initialization.  Allow FSP2_0_USES_TPM_MRC_HASH option.

* VBOOT=y VBOOT_STARTS_IN_BOOTBLOCK=n
  vboot is enabled, but TPM is setup in romstage, after Intel
  FSP memory initialization.  Disallow FSP2_0_USES_TPM_MRC_HASH
  option.

* VBOOT=n
  vboot is disabled.  Disallow FSP2_0_USES_TPM_MRC_HASH option.

See bug for more information:
https://bugs.chromium.org/p/chromium/issues/detail?id=940377

BUG=chromium:940377
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: I4ba91c275c33245be61041cb592e52f861dbafe6
Signed-off-by: Joel Kitching 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/memory_init.c
2 files changed, 10 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/31837/5

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

Patch Set 5: Code-Review+1

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/#/c/31837/3/src/security/vboot/secdata_tpm.c@453 
PS3, Line 453: 	    CONFIG(VBOOT_STARTS_IN_ROMSTAGE))
> I think we still need to consider killing VBOOT_STARTS_IN_ROMSTAGE. […]

As I told you. May work for Google but not for everyone else. I am fine with it, if Google takes the effort porting all old platforms from FSP 1.0/1.1 to VBOOT_STARTS_IN_BOOTBLOCK

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

Patch Set 5:
> Patch Set 4:
>
> (2 comments)
>
> We actually have two options here:
>
> When VBOOT_STARTS_IN_ROMSTAGE is enabled, disallow USE_RECOVERY_MRC_CACHE.
> AFAIK this would regenerate the recovery MRC training data on each recovery mode boot.
> Pro: slow.  Con: safe.
>
> When VBOOT_STARTS_IN_ROMSTAGE is enabled, disallow FSP2_0_USES_TPM_MRC_HASH.
> This would use the recovery MRC cache as normal, without the save-hash-in-TPM functionality.
> Pro: fast.  Con: less safe.

I am fine with the second option.

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Philipp Deppenwiese (Code Review)]

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

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

Patch Set 5: Code-Review+2

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

Patch Set 5: Code-Review+2

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Julius Werner (Code Review)]

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

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

Patch Set 5: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/31837/5/src/drivers/intel/fsp2_0/Kconfig 
File src/drivers/intel/fsp2_0/Kconfig:

https://review.coreboot.org/#/c/31837/5/src/drivers/intel/fsp2_0/Kconfig@162 
PS5, Line 162: VBOOT_STARTS_IN_BOOTBLOCK
nit: depends on has no teeth for things that are 'select'ed by other Kconfig options. Maybe add a _Static_assert() somewhere to be sure?

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Joel Kitching (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

When VBOOT_STARTS_IN_BOOTBLOCK is selected, the tpm_setup call
in memory_init.c is not used.

When VBOOT_STARTS_IN_ROMSTAGE is selected, the tpm_setup call
in memory_init.c is triggered.  However, when verstage runs,
tpm_setup is called yet again, and an error is triggered from
the multiple initialization calls.

Since there are currently no boards using
VBOOT_STARTS_IN_ROMSTAGE + FSP2_0_USES_TPM_MRC_HASH, disable
this combination via Kconfig, and remove the tpm_setup call
from Intel FSP memory initializion code.

* VBOOT=y VBOOT_STARTS_IN_BOOTBLOCK=y
  vboot is enabled, and TPM is setup prior to Intel FSP memory
  initialization.  Allow FSP2_0_USES_TPM_MRC_HASH option.

* VBOOT=y VBOOT_STARTS_IN_BOOTBLOCK=n
  vboot is enabled, but TPM is setup in romstage, after Intel
  FSP memory initialization.  Disallow FSP2_0_USES_TPM_MRC_HASH
  option.

* VBOOT=n
  vboot is disabled.  Disallow FSP2_0_USES_TPM_MRC_HASH option.

See bug for more information:
https://bugs.chromium.org/p/chromium/issues/detail?id=940377

BUG=chromium:940377
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: I4ba91c275c33245be61041cb592e52f861dbafe6
Signed-off-by: Joel Kitching 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/memory_init.c
2 files changed, 15 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/31837/6

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/31837/5/src/drivers/intel/fsp2_0/Kconfig 
File src/drivers/intel/fsp2_0/Kconfig:

https://review.coreboot.org/#/c/31837/5/src/drivers/intel/fsp2_0/Kconfig@162 
PS5, Line 162: VBOOT_STARTS_IN_BOOTBLOCK
> nit: depends on has no teeth for things that are 'select'ed by other Kconfig options. […]

Sure.  Please double-check my boolean logic...

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

Patch Set 6: Code-Review+2

Change in ...coreboot[master]: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

[Julius Werner (Code Review)]

Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31837 )

Change subject: drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic
......................................................................

drivers/intel/fsp2_0: fix TPM setup and MRC cache hash logic

When VBOOT_STARTS_IN_BOOTBLOCK is selected, the tpm_setup call
in memory_init.c is not used.

When VBOOT_STARTS_IN_ROMSTAGE is selected, the tpm_setup call
in memory_init.c is triggered.  However, when verstage runs,
tpm_setup is called yet again, and an error is triggered from
the multiple initialization calls.

Since there are currently no boards using
VBOOT_STARTS_IN_ROMSTAGE + FSP2_0_USES_TPM_MRC_HASH, disable
this combination via Kconfig, and remove the tpm_setup call
from Intel FSP memory initializion code.

* VBOOT=y VBOOT_STARTS_IN_BOOTBLOCK=y
  vboot is enabled, and TPM is setup prior to Intel FSP memory
  initialization.  Allow FSP2_0_USES_TPM_MRC_HASH option.

* VBOOT=y VBOOT_STARTS_IN_BOOTBLOCK=n
  vboot is enabled, but TPM is setup in romstage, after Intel
  FSP memory initialization.  Disallow FSP2_0_USES_TPM_MRC_HASH
  option.

* VBOOT=n
  vboot is disabled.  Disallow FSP2_0_USES_TPM_MRC_HASH option.

See bug for more information:
https://bugs.chromium.org/p/chromium/issues/detail?id=940377

BUG=chromium:940377
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: I4ba91c275c33245be61041cb592e52f861dbafe6
Signed-off-by: Joel Kitching 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31837
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/memory_init.c
2 files changed, 15 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Joel Kitching: Looks good to me, but someone else must approve

diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig
index 3951e9a..2b98542 100644
--- a/src/drivers/intel/fsp2_0/Kconfig
+++ b/src/drivers/intel/fsp2_0/Kconfig
@@ -159,10 +159,19 @@
 config FSP2_0_USES_TPM_MRC_HASH
    bool
    depends on TPM1 || TPM2
-	depends on VBOOT
+	depends on VBOOT && VBOOT_STARTS_IN_BOOTBLOCK
    default y if HAS_RECOVERY_MRC_CACHE
    default n
    select VBOOT_HAS_REC_HASH_SPACE
+	help
+	  Store hash of trained recovery MRC cache in NVRAM space in TPM.
+	  Use the hash to validate recovery MRC cache before using it.
+	  This hash needs to be updated every time recovery mode training
+	  is recomputed, or if the hash does not match recovery MRC cache.
+	  Selecting this option requires that TPM already be setup by this
+	  point in time.  Thus it is only compatible when the option
+	  VBOOT_STARTS_IN_BOOTBLOCK is selected, which causes verstage and
+	  TPM setup to occur prior to memory initialization.

config FSP_PLATFORM_MEMORY_SETTINGS_VERSIONS
    bool
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index 3dafcf8..f7cf0dd 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -33,6 +33,11 @@
 #include <vb2_api.h>
 #include <fsp/memory_init.h>

+/* TPM MRC hash functionality depends on vboot starting before memory init. */
+_Static_assert(!CONFIG(FSP2_0_USES_TPM_MRC_HASH) ||
+	       CONFIG(VBOOT_STARTS_IN_BOOTBLOCK),
+	       "for TPM MRC hash functionality, vboot must start in bootblock");
+
 static void save_memory_training_data(bool s3wake, uint32_t fsp_version)
 {
    size_t  mrc_data_size;
@@ -92,14 +97,6 @@

/* Create romstage handof information */
    romstage_handoff_init(s3wake);
-
-	/*
-	 * Initialize the TPM, unless the TPM was already initialized
-	 * in verstage and used to verify romstage.
-	 */
-	if ((CONFIG(TPM1) || CONFIG(TPM2)) &&
-	    !CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
-		tpm_setup(s3wake);
 }

static void fsp_fill_mrc_cache(FSPM_ARCH_UPD *arch_upd, uint32_t fsp_version)