<p><a href="https://review.coreboot.org/22374">View Change</a></p><p>14 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22374/25//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25//COMMIT_MSG@12">Patch Set #25, Line 12:</a> <code style="font-family:monospace,monospace">* Fix TPM1 missing kconfig option for boards with TPM 1.2.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This wasn't missing, it's intentional. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">okay got it.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/Kconfig">File src/drivers/intel/fsp2_0/Kconfig:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/Kconfig@117">Patch Set #25, Line 117:</a> <code style="font-family:monospace,monospace"> depends on TPM2</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Can also be used with TPM1, I think. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/memory_init.c">File src/drivers/intel/fsp2_0/memory_init.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/drivers/intel/fsp2_0/memory_init.c@38">Patch Set #25, Line 38:</a> <code style="font-family:monospace,monospace">#if IS_ENABLED(CONFIG_FSP2_0_USES_TPM_MRC_HASH)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Do not #ifdef out code just because it won't be used. Just do  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/Kconfig">File src/security/tpm/Kconfig:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/Kconfig@70">Patch Set #25, Line 70:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This Kconfig doesn't do anything anymore after your previous patch. Either implement it or drop it.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/Kconfig@78">Patch Set #25, Line 78:</a> <code style="font-family:monospace,monospace">    Power off machine while waiting for CR50 update to take effect.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ditto</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss.h">File src/security/tpm/tss.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss.h@25">Patch Set #25, Line 25:</a> <code style="font-family:monospace,monospace">#if (IS_ENABLED(CONFIG_TPM1) || IS_ENABLED(CONFIG_TPM2))</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You should not #ifdef out header definitions unless there's a specific reason. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss.h@86">Patch Set #25, Line 86:</a> <code style="font-family:monospace,monospace">#endif</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should be fixed in the previous CL already.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/cr50/tss.c">File src/security/tpm/tss/cr50/tss.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/cr50/tss.c@1">Patch Set #25, Line 1:</a> <code style="font-family:monospace,monospace">/*</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I wonder if this is the best name/location for it... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">good point, let me fix that</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="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:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/tcg-1.2/tss_structures.h@17">Patch Set #25, Line 17:</a> <code style="font-family:monospace,monospace">#define TPM_PCR_DIGEST 20</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Fixed see secdata_tpm.c comment</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="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:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss/tcg-2.0/tss.c@19">Patch Set #25, Line 19:</a> <code style="font-family:monospace,monospace">void *tpm_process_command(TPM_CC command, void *command_body);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Let's put this in a real header somewhere. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss_error_messages.h">File src/security/tpm/tss_error_messages.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/tpm/tss_error_messages.h@12">Patch Set #25, Line 12:</a> <code style="font-family:monospace,monospace">TSS_ERROR_MESSAGES_H_</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">nit: error_*messages* doesn't really fit for this file anymore. Maybe just tss_errors. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_mock.c">File src/security/vboot/secdata_mock.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_mock.c@75">Patch Set #25, Line 75:</a> <code style="font-family:monospace,monospace">VB2_SUCCESS</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">These changes aren't really correct, most of these functions in the non-mock implementation still us […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This code belongs to vboot2 IMHO it should return vboot error codes instead of tpm error codes. Especially if we talk about interfaces..</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_tpm.c">File src/security/vboot/secdata_tpm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_tpm.c@56">Patch Set #25, Line 56:</a> <code style="font-family:monospace,monospace">#define RETURN_ON_FAILURE(tpm_cmd) do {                               \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think it would be better to just move this to the existing #if...TPM2 block below. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22374/25/src/security/vboot/secdata_tpm.c@104">Patch Set #25, Line 104:</a> <code style="font-family:monospace,monospace">B2_SUCCESS)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why did this get renamed? tlcl_extend() in it's current form can only take digests of exactly this l […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/22374">change 22374</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/22374"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I97bbc7b7b025500b49c743b0c303543c33627c88 </div>
<div style="display:none"> Gerrit-Change-Number: 22374 </div>
<div style="display:none"> Gerrit-PatchSet: 32 </div>
<div style="display:none"> Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> </div>
<div style="display:none"> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> </div>
<div style="display:none"> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> </div>
<div style="display:none"> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> </div>
<div style="display:none"> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Randall Spangler <randall@spanglers.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 20 Feb 2018 13:06:36 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>