Christian Walter has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: src/security/tpm/tss: Add Support for PTT ......................................................................
src/security/tpm/tss: Add Support for PTT
When we use Intel Platform Trust Technologies, we need to verify that the enable bit is set before we use the integrated TPM
Change-Id: I3b262a5d5253648fb96fb1fd9ba3995f92755bb1 Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/security/tpm/tss/tcg-2.0/tss.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34381/1
diff --git a/src/security/tpm/tss/tcg-2.0/tss.c b/src/security/tpm/tss/tcg-2.0/tss.c index c4b5538..5cba43d 100644 --- a/src/security/tpm/tss/tcg-2.0/tss.c +++ b/src/security/tpm/tss/tcg-2.0/tss.c @@ -11,6 +11,7 @@ #include <vb2_api.h> #include <security/tpm/tis.h> #include <security/tpm/tss.h> +#include <drivers/ptt/ptt.h>
#include "tss_structures.h" #include "tss_marshaling.h" @@ -190,6 +191,13 @@ printk(BIOS_ERR, "%s: tis_open returned error\n", __func__); return VB2_ERROR_UNKNOWN; } + if (CONFIG(INTEL_PTT)) { + if (ptt_active()) { + printk(BIOS_ERR, "%s: Intel PTT is not active.\n", __func__); + return VB2_ERROR_UNKNOWN; + } + printk(BIOS_SPEW, "%s: Intel PTT is active.\n", __func__); + }
car_set_var(tlcl_init_done, 1);
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: src/security/tpm/tss: Add Support for PTT ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34381/3/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/34381/3/src/security/tpm/tss/tcg-2.... PS3, Line 195: if (ptt_active()) { !ptt_active() ?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: src/security/tpm/tss: Add Support for PTT ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34381/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34381/3//COMMIT_MSG@7 PS3, Line 7: Support support
https://review.coreboot.org/c/coreboot/+/34381/3//COMMIT_MSG@9 PS3, Line 9: When we use Intel Platform Trust Technologies, we need to verify : that the enable bit is set before we use the integrated TPM Please add a dot/period at the end of a sentence.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34381
to look at the new patch set (#4).
Change subject: src/security/tpm/tss: Add support for PTT ......................................................................
src/security/tpm/tss: Add support for PTT
When we use Intel Platform Trust Technologies, we need to verify that the enable bit is set before we use the integrated TPM.
Change-Id: I3b262a5d5253648fb96fb1fd9ba3995f92755bb1 Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/security/tpm/tss/tcg-2.0/tss.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34381/4
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: src/security/tpm/tss: Add support for PTT ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34381/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34381/3//COMMIT_MSG@7 PS3, Line 7: Support
support
Ack
https://review.coreboot.org/c/coreboot/+/34381/3//COMMIT_MSG@9 PS3, Line 9: When we use Intel Platform Trust Technologies, we need to verify : that the enable bit is set before we use the integrated TPM
Please add a dot/period at the end of a sentence.
Ack
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34381
to look at the new patch set (#5).
Change subject: src/security/tpm/tss: Add support for PTT ......................................................................
src/security/tpm/tss: Add support for PTT
When we use Intel Platform Trust Technologies, we need to verify that the enable bit is set before we use the integrated TPM.
Change-Id: I3b262a5d5253648fb96fb1fd9ba3995f92755bb1 Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/security/tpm/tss/tcg-2.0/tss.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34381/5
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34381
to look at the new patch set (#6).
Change subject: src/security/tpm/tss: Add support for PTT ......................................................................
src/security/tpm/tss: Add support for PTT
When we use Intel Platform Trust Technologies, we need to verify that the enable bit is set before we use the integrated TPM.
Change-Id: I3b262a5d5253648fb96fb1fd9ba3995f92755bb1 Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/security/tpm/tss/tcg-2.0/tss.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34381/6
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: src/security/tpm/tss: Add support for PTT ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34381/3/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/34381/3/src/security/tpm/tss/tcg-2.... PS3, Line 195: if (ptt_active()) {
!ptt_active() ?
Ack
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: src/security/tpm/tss: Add support for PTT ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... PS12, Line 195: if (CONFIG(HAVE_INTEL_PTT)) { I'm confused how this works and why this has to be here. Where is the code that implements tis_init() and tis_open() for this TPM? I don't see it anywhere in this patch train.
(If this is one of those weird Intel things where the hardware emulates a legacy interface and you're using e.g. src/drivers/pc80/tpm to access this, this config option and code should be in there, not here. It's an implementation detail of the TPM implementation driver, not the protocol stack.)
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: src/security/tpm/tss: Add support for PTT ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... PS12, Line 195: if (CONFIG(HAVE_INTEL_PTT)) {
I'm confused how this works and why this has to be here. […]
It implements the CRB interface, added here https://review.coreboot.org/c/coreboot/+/34106/
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: src/security/tpm/tss: Add support for PTT ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... PS12, Line 195: if (CONFIG(HAVE_INTEL_PTT)) {
It implements the CRB interface, added here https://review.coreboot. […]
That's right. The bit (according to the specs) has to be checked after the tis_open and before we are processing a command for the first time. We could also move it into tis_open, but then it would be interface specific. Maybe at some point in time, Intel decides to add another interface to their iTPM Specs and then we would need to reimplement it again for the other interface.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34381
to look at the new patch set (#14).
Change subject: security/tpm/tss: Add support for PTT ......................................................................
security/tpm/tss: Add support for PTT
When we use Intel Platform Trust Technologies, we need to verify that the enable bit is set before we use the integrated TPM.
Change-Id: I3b262a5d5253648fb96fb1fd9ba3995f92755bb1 Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/security/tpm/tss/tcg-2.0/tss.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34381/14
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: security/tpm/tss: Add support for PTT ......................................................................
Patch Set 14: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: security/tpm/tss: Add support for PTT ......................................................................
Patch Set 15: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... PS12, Line 195: if (CONFIG(HAVE_INTEL_PTT)) {
That's right. […]
Sorry, but I really disagree. The stack goes down from the high-level protocol stuff to the low-level platform implementation. This seems like a low-level platform detail. It doesn't belong in here. I mean, a big open-coded if (INTEL_STUFF) {...} block doesn't belong in here anyway... but if you encapsulated it properly, you'd have to create a function like platform_tpm_initialized() and call it here, and I just don't really see how that would make sense for any other platform than this one. All normal TPMs are connected through an external peripheral interface where it doesn't really make sense for the SoC to have some magic internal knowledge about the state of the TPM. So I think the CRB interface is really the only one for which something like this makes sense, and you should put it there.
Also, why are you calling this *after* tis_init() and tis_open() anyway? Wouldn't those fail already when the PTT is not active? Does the CRB interface not have an in-band mechanism to tell if the thing is active? That would really be a way better way to check this anyway.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: security/tpm/tss: Add support for PTT ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... PS12, Line 195: if (CONFIG(HAVE_INTEL_PTT)) {
Sorry, but I really disagree. […]
[not resolved]
Philipp Deppenwiese has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: security/tpm/tss: Add support for PTT ......................................................................
Removed Code-Review+2 by Philipp Deppenwiese zaolin.daisuki@gmail.com
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: security/tpm/tss: Add support for PTT ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... PS12, Line 195: if (CONFIG(HAVE_INTEL_PTT)) {
[not resolved]
I'm not sure about whether tis_open or tis_init would fail or not. The Intel PTT BIOS Writers Guide states that this should be done prior sending a TPM Command. But anyways - we can move it from the TSS Layer to the TIS Layer if that is fine with everyone.
My point was, that if Intel decides to support more interface (because logically the ME sits in the SB, which is also connected via LPC - so LPC would also be an option here), we have to integrate this into more interfaces.
Hello Julius Werner, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34381
to look at the new patch set (#17).
Change subject: drivers/crb: Add support for PTT ......................................................................
drivers/crb: Add support for PTT
When we use Intel Platform Trust Technologies, we need to verify that the enable bit is set before we use the integrated TPM.
Change-Id: I3b262a5d5253648fb96fb1fd9ba3995f92755bb1 Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/drivers/crb/tis.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34381/17
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: drivers/crb: Add support for PTT ......................................................................
Patch Set 17: Code-Review+1
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: drivers/crb: Add support for PTT ......................................................................
Patch Set 27:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/34381/12/src/security/tpm/tss/tcg-2... PS12, Line 195: if (CONFIG(HAVE_INTEL_PTT)) {
I'm not sure about whether tis_open or tis_init would fail or not. […]
Ack
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: drivers/crb: Add support for PTT ......................................................................
Patch Set 28: Code-Review+2
Philipp Deppenwiese has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34381 )
Change subject: drivers/crb: Add support for PTT ......................................................................
drivers/crb: Add support for PTT
When we use Intel Platform Trust Technologies, we need to verify that the enable bit is set before we use the integrated TPM.
Change-Id: I3b262a5d5253648fb96fb1fd9ba3995f92755bb1 Signed-off-by: Christian Walter christian.walter@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34381 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com Reviewed-by: Julius Werner jwerner@chromium.org --- M src/drivers/crb/tis.c 1 file changed, 9 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved Julius Werner: Looks good to me, but someone else must approve
diff --git a/src/drivers/crb/tis.c b/src/drivers/crb/tis.c index c110151..94bfb9e 100644 --- a/src/drivers/crb/tis.c +++ b/src/drivers/crb/tis.c @@ -16,6 +16,7 @@ #include <security/tpm/tis.h> #include <arch/acpigen.h> #include <device/device.h> +#include <drivers/intel/ptt/ptt.h>
#include "tpm.h" #include "chip.h" @@ -49,6 +50,14 @@ return -1; }
+ if (CONFIG(HAVE_INTEL_PTT)) { + if (!ptt_active()) { + printk(BIOS_ERR, "%s: Intel PTT is not active.\n", __func__); + return -1; + } + printk(BIOS_DEBUG, "%s: Intel PTT is active.\n", __func__); + } + return 0; }
-
Christian Walter (Code Review) -
Julius Werner (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)