[coreboot-gerrit] Change in coreboot[master]: security/tpm: Fix TCPA log feature

[Philipp Deppenwiese (Code Review)]

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

Change subject: security/tpm: Fix TCPA log feature
......................................................................


Patch Set 13:

(13 comments)

https://review.coreboot.org/#/c/29563/13/src/commonlib/include/commonlib/tcpa_log_serialized.h
File src/commonlib/include/commonlib/tcpa_log_serialized.h:

https://review.coreboot.org/#/c/29563/13/src/commonlib/include/commonlib/tcpa_log_serialized.h@24
PS13, Line 24: 128
> This should be (TCPA_DIGEST_MAX_LENGTH * 2 + 1)
Done


https://review.coreboot.org/#/c/29563/13/src/commonlib/include/commonlib/tcpa_log_serialized.h@32
PS13, Line 32: 	char name[TCPA_PCR_HASH_NAME];
> If you are directly exposing this structure how is the tooling supposed to know where the fields end […]
TCPA_FORMAT_HASH_LENGTH and TCPA_LOG_STRING_LENGTH are only used from cbmem tool


https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c
File src/security/tpm/tspi/log.c:

https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@30
PS13, Line 30: 	if (!cbmem_possibly_online() &&
> You should add a comment here about what case you are dealing with.
Done


https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@43
PS13, Line 43: 					 sizeof(struct tcpa_entry));
> You should memset() to 0 because cbmem is not guarantee to be initialized.
Done


https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@73
PS13, Line 73: (char *)
> hash is already char type. No need to cast.
Done


https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@74
PS13, Line 74: 						tce->digest_length,
> This length calculation is wrong. You are overwriting the buffer on the last 2 nibbles.
Done


https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@82
PS13, Line 82: hash
> This string is not NUL terminated. It's exactly 2x max hash size. It is not a valid C string.
Done


https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@102
PS13, Line 102: if (tce)
> This would always evaluate to non-NULL unless we were wrapping arithmetic or entries was first field […]
Done


https://review.coreboot.org/#/c/29563/13/src/security/vboot/Kconfig
File src/security/vboot/Kconfig:

https://review.coreboot.org/#/c/29563/13/src/security/vboot/Kconfig@258
PS13, Line 258: default y
> If it's default y how is anyone supposed to unselect it? If it's driven by VBOOT_MEASURED_BOOT then  […]
Done


https://review.coreboot.org/#/c/29563/13/src/security/vboot/vboot_crtm.c
File src/security/vboot/vboot_crtm.c:

https://review.coreboot.org/#/c/29563/13/src/security/vboot/vboot_crtm.c@165
PS13, Line 165: tcpa_log_dump()
> We can just call this when we're exiting BS_LOAD_PAYLOAD state. […]
Done


https://review.coreboot.org/#/c/29563/13/src/security/vboot/vboot_crtm.c@170
PS13, Line 170: static void recover_tcpa_log(int is_recovery)
> Why isn't this function just in the other file where you handled the rest of the log stuff?
It's only needed for measurements in the vboot case


https://review.coreboot.org/#/c/29563/13/src/security/vboot/vboot_crtm.c@173
PS13, Line 173: if (tclt)
> When would this be NULL?
Done


https://review.coreboot.org/#/c/29563/13/src/security/vboot/vboot_crtm.c@188
PS13, Line 188: ROMSTAGE_CBMEM_INIT_HOOK(recover_tcpa_log);
> Do you want to handle other conditions of cbmem coming online later? If so you need the other CBMEM_ […]
mhh I guess you are right. Only ramstage and postcar are leftovers ?



-- 
To view, visit https://review.coreboot.org/29563
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: Ic93133531b84318f48940d34bded48cbae739c44
Gerrit-Change-Number: 29563
Gerrit-PatchSet: 13
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks at gmail.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi at google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph at 9elements.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh at siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Patrick Rudolph (1001864)
Gerrit-CC: Patrick Rudolph <siro at das-labor.org>
Gerrit-Comment-Date: Mon, 19 Nov 2018 15:48:45 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181119/bb514047/attachment.html>