[coreboot-gerrit] Change in coreboot[master]: security/tpm: Refactor TSS 1.2 and 2.0 implementation

Tue Feb 20 14:06:36 CET 2018

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

Change subject: security/tpm: Refactor TSS 1.2 and 2.0 implementation
......................................................................

Patch Set 32:

(14 comments)

https://review.coreboot.org/#/c/22374/25//COMMIT_MSG
Commit Message:

https://review.coreboot.org/#/c/22374/25//COMMIT_MSG@12
PS25, Line 12: * Fix TPM1 missing kconfig option for boards with TPM 1.2.
> This wasn't missing, it's intentional. […]
okay got it.

https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/Kconfig
File src/drivers/intel/fsp2_0/Kconfig:

https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/Kconfig@117
PS25, Line 117: 	depends on TPM2
> Can also be used with TPM1, I think. […]
Done

https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/memory_init.c
File src/drivers/intel/fsp2_0/memory_init.c:

https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/memory_init.c@38
PS25, Line 38: #if IS_ENABLED(CONFIG_FSP2_0_USES_TPM_MRC_HASH)
> Do not #ifdef out code just because it won't be used. Just do  […]
Done

https://review.coreboot.org/#/c/22374/25/src/security/tpm/Kconfig
File src/security/tpm/Kconfig:

https://review.coreboot.org/#/c/22374/25/src/security/tpm/Kconfig@70
PS25, Line 70: 
> This Kconfig doesn't do anything anymore after your previous patch. Either implement it or drop it.
Done

https://review.coreboot.org/#/c/22374/25/src/security/tpm/Kconfig@78
PS25, Line 78: 	  Power off machine while waiting for CR50 update to take effect.
> ditto
Done

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss.h
File src/security/tpm/tss.h:

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss.h@25
PS25, Line 25: #if (IS_ENABLED(CONFIG_TPM1) || IS_ENABLED(CONFIG_TPM2))
> You should not #ifdef out header definitions unless there's a specific reason. […]
Done

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss.h@86
PS25, Line 86: #endif
> Should be fixed in the previous CL already.
Done

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/cr50/tss.c
File src/security/tpm/tss/cr50/tss.c:

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/cr50/tss.c@1
PS25, Line 1: /*
> I wonder if this is the best name/location for it... […]
good point, let me fix that

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/tcg-1.2/tss_structures.h
File src/security/tpm/tss/tcg-1.2/tss_structures.h:

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/tcg-1.2/tss_structures.h@17
PS25, Line 17: #define TPM_PCR_DIGEST 20
> ... […]
Fixed see secdata_tpm.c comment

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/tcg-2.0/tss.c
File src/security/tpm/tss/tcg-2.0/tss.c:

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/tcg-2.0/tss.c@19
PS25, Line 19: void *tpm_process_command(TPM_CC command, void *command_body);
> Let's put this in a real header somewhere. […]
Done

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss_error_messages.h
File src/security/tpm/tss_error_messages.h:

https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss_error_messages.h@12
PS25, Line 12: TSS_ERROR_MESSAGES_H_
> nit: error_*messages* doesn't really fit for this file anymore. Maybe just tss_errors. […]
Ack

https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_mock.c
File src/security/vboot/secdata_mock.c:

https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_mock.c@75
PS25, Line 75: VB2_SUCCESS
> These changes aren't really correct, most of these functions in the non-mock implementation still us […]
This code belongs to vboot2 IMHO it should return vboot error codes instead of tpm error codes. Especially if we talk about interfaces..

https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_tpm.c
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_tpm.c@56
PS25, Line 56: #define RETURN_ON_FAILURE(tpm_cmd) do {				\
> I think it would be better to just move this to the existing #if...TPM2 block below. […]
Done

https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_tpm.c@104
PS25, Line 104: B2_SUCCESS)
> Why did this get renamed? tlcl_extend() in it's current form can only take digests of exactly this l […]
we have TPM 1.2 and 2.0 support. Both have a min size of 20 bytes because of SHA-1. I moved TPM_PCR_MIN_DIGEST to the common area

-- 
To view, visit https://review.coreboot.org/22374
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I97bbc7b7b025500b49c743b0c303543c33627c88
Gerrit-Change-Number: 22374
Gerrit-PatchSet: 32
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro at das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Randall Spangler <randall at spanglers.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Tue, 20 Feb 2018 13:06:36 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180220/6831bbcc/attachment.html>