<p><a href="https://review.coreboot.org/22106">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22106/37/src/security/tpm/Makefile.inc">File src/security/tpm/Makefile.inc:</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/22106/37/src/security/tpm/Makefile.inc@7">Patch Set #37, Line 7:</a> <code style="font-family:monospace,monospace">ifeq ($(CONFIG_VBOOT),y)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why not just leave the conditionals out and compile it into all stages? It will be garbage collected […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yeah makes sense.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22106/37/src/security/tpm/tspi/tspi.c">File src/security/tpm/tspi/tspi.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/22106/37/src/security/tpm/tspi/tspi.c@114">Patch Set #37, Line 114:</a> <code style="font-family:monospace,monospace">               if (IS_ENABLED(CONFIG_TPM_DEACTIVATE))</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Note that this won't deactivate an already activated TPM if you bury it down here, because it won't  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">yep gonna fix that</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22106/37/src/security/tpm/tspi/tspi.c@132">Patch Set #37, Line 132:</a> <code style="font-family:monospace,monospace">     result != TPM_SUCCESS) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Can result even == TPM_SUCCESS here?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">leftover. Will fix it</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22106/37/src/security/tpm/tspi/tspi.c@137">Patch Set #37, Line 137:</a> <code style="font-family:monospace,monospace">              hard_reset();</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This will break vboot because it would now also be executed in the recovery path. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">okay</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22106/37/src/security/tpm/tspi/tspi.c@154">Patch Set #37, Line 154:</a> <code style="font-family:monospace,monospace">#if IS_ENABLED(CONFIG_TPM)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">It's really confusing what this is doing here... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I totally agree.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22106/37/src/security/tpm/tspi/tspi.c@173">Patch Set #37, Line 173:</a> <code style="font-family:monospace,monospace">**out_digest</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why does this need to be a double pointer? And why do we need this wrapper at all, it does so little […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Will fix it. IMHO the high level interface should be used instead of accessing the tss directly. Maybe in this case it is not really useful.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22106/37/src/security/tpm/tss/tcg-2.0/tss_structures.h">File src/security/tpm/tss/tcg-2.0/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/22106/37/src/security/tpm/tss/tcg-2.0/tss_structures.h@137">Patch Set #37, Line 137:</a> <code style="font-family:monospace,monospace">#define REC_HASH_NV_INDEX               0x100b</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Note that these are vboot-specific, maybe they should go in a vboot file (like antirollback. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">leftover, sure I will fix it</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/22106/37/src/security/vboot/antirollback.h">File src/security/vboot/antirollback.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/22106/37/src/security/vboot/antirollback.h@60">Patch Set #37, Line 60:</a> <code style="font-family:monospace,monospace">setup_tpm</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">...and while we're at it, this could also be clearer as vboot_setup_tpm().</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This function is used as part of the TPM interface. So it is not vboot specific!</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22106/37/src/security/vboot/antirollback.h@63">Patch Set #37, Line 63:</a> <code style="font-family:monospace,monospace">extend_pcr</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This should probably be namespaced in some way... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">See above</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/22106">change 22106</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/22106"/><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: I883c489801fce88e13952fe24b67315ab6bb1afb </div>
<div style="display:none"> Gerrit-Change-Number: 22106 </div>
<div style="display:none"> Gerrit-PatchSet: 37 </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: Kyösti Mälkki <kyosti.malkki@gmail.com> </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, 23 Jan 2018 02:07:51 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>