Import recent tpm_lite changes that make all TPM commands return a status.

Import recent tpm_lite changes that make all TPM commands return a status.

[Luigi Semenzato, 2010-06-08 20:18:24]

Here we go.  TEST: it compiles.

[Luigi Semenzato, 2010-06-08 23:35:47]

I made the changes and they compile.  I still have the nagging suspicion that I
switched the 1/0 convention for success/failure for some function and forgot to
change the order of some IF clause that called that function.  I will look for
that too.

[gauravsh, 2010-06-09 12:25:50]

On Tue, Jun 8, 2010 at 4:35 PM,  <semenzato@chromium.org> wrote:
> I made the changes and they compile.  I still have the nagging suspicion
> that I
> switched the 1/0 convention for success/failure for some function and forgot
> to
> change the order of some IF clause that called that function.  I will look
> for
> that too.
>
> http://codereview.chromium.org/2711006/show
>

Is this ready for a review or are you in the process of making changes?

-- 
-g

[Randall Spangler, 2010-06-09 15:32:41]

http://codereview.chromium.org/2711006/diff/10001/11003
File vboot_firmware/include/rollback_index.h (right):

http://codereview.chromium.org/2711006/diff/10001/11003#newcode32
vboot_firmware/include/rollback_index.h:32: int GetStoredVersions(int type,
uint16_t* key_version, uint16_t* version);
Since these pass through TPM return codes, shouldn't they also return uint32_t?

http://codereview.chromium.org/2711006/diff/10001/11006
File vboot_firmware/lib/rollback_index.c (right):

http://codereview.chromium.org/2711006/diff/10001/11006#newcode258
vboot_firmware/lib/rollback_index.c:258: RETURN_ON_FAILURE(TlclForceClear());
Isn't this the normal code path?  Neither write failed, so why are we forcing
clear and rebooting?

[Luigi Semenzato, 2010-06-09 15:47:07]

On Wed, Jun 9, 2010 at 8:32 AM,  <rspangler@chromium.org> wrote:
>
> http://codereview.chromium.org/2711006/diff/10001/11003
> File vboot_firmware/include/rollback_index.h (right):
>
> http://codereview.chromium.org/2711006/diff/10001/11003#newcode32
> vboot_firmware/include/rollback_index.h:32: int GetStoredVersions(int
> type, uint16_t* key_version, uint16_t* version);
> Since these pass through TPM return codes, shouldn't they also return
> uint32_t?

Yes.  I will fix these, thanks.

>
> http://codereview.chromium.org/2711006/diff/10001/11006
> File vboot_firmware/lib/rollback_index.c (right):
>
> http://codereview.chromium.org/2711006/diff/10001/11006#newcode258
> vboot_firmware/lib/rollback_index.c:258:
> RETURN_ON_FAILURE(TlclForceClear());
> Isn't this the normal code path?  Neither write failed, so why are we
> forcing clear and rebooting?

I was confused, sorry.  We need to first check if there is an owner,
and ForceClear only if there isn't one.  We do that to prevent running
against the 64-write limit without an owner.

> http://codereview.chromium.org/2711006/show
>

[Luigi Semenzato, 2010-06-09 18:22:44]

PTAL

[gauravsh, 2010-06-09 18:30:09]

http://codereview.chromium.org/2711006/diff/19001/20001
File tests/rollback_index_mock.c (right):

http://codereview.chromium.org/2711006/diff/19001/20001#newcode25
tests/rollback_index_mock.c:25: int GetStoredVersions(int type, uint16_t*
key_version, uint16_t* version) {
also modify return types (uint32_t) for all the mocked functions

http://codereview.chromium.org/2711006/diff/19001/20005
File vboot_firmware/lib/firmware_image_fw.c (right):

http://codereview.chromium.org/2711006/diff/19001/20005#newcode313
vboot_firmware/lib/firmware_image_fw.c:313: if (0 != LockFirmwareVersions()) {
instead of 0, can you use TSS_SUCCESS (assuming there's a trousers equivalent of
a TPM function succeeding)

http://codereview.chromium.org/2711006/diff/19001/20005#newcode314
vboot_firmware/lib/firmware_image_fw.c:314: return
VERIFY_FIRMWARE_INTERNAL_ERROR;
would VERIFY_FIRMWARE_TPM_ERROR be a more accurate/informative description of
this error message?

http://codereview.chromium.org/2711006/diff/19001/20008
File vboot_firmware/stub/tlcl.c (right):

http://codereview.chromium.org/2711006/diff/19001/20008#newcode28
vboot_firmware/stub/tlcl.c:28: int TlclIsOwned(void) { return TPM_SUCCESS; }
can this return an error? If so, should the return type be uint32_t?

[Luigi Semenzato, 2010-06-09 19:36:01]

PTAL.  Changed as suggested, except for the change to TlclIsOwned which requires
a parallel change to tpm_lite.  (Maybe it's time to move tpm_lite under
vboot_reference?)

http://codereview.chromium.org/2711006/diff/19001/20001
File tests/rollback_index_mock.c (right):

http://codereview.chromium.org/2711006/diff/19001/20001#newcode25
tests/rollback_index_mock.c:25: int GetStoredVersions(int type, uint16_t*
key_version, uint16_t* version) {
On 2010/06/09 18:30:10, gauravsh wrote:
> also modify return types (uint32_t) for all the mocked functions

Yes, sure.  I wonder why the compiler didn't catch this?

http://codereview.chromium.org/2711006/diff/19001/20005
File vboot_firmware/lib/firmware_image_fw.c (right):

http://codereview.chromium.org/2711006/diff/19001/20005#newcode314
vboot_firmware/lib/firmware_image_fw.c:314: return
VERIFY_FIRMWARE_INTERNAL_ERROR;
On 2010/06/09 18:30:10, gauravsh wrote:
> would VERIFY_FIRMWARE_TPM_ERROR be a more accurate/informative description of
> this error message?

Sure!

http://codereview.chromium.org/2711006/diff/19001/20008
File vboot_firmware/stub/tlcl.c (right):

http://codereview.chromium.org/2711006/diff/19001/20008#newcode28
vboot_firmware/stub/tlcl.c:28: int TlclIsOwned(void) { return TPM_SUCCESS; }
On 2010/06/09 18:30:10, gauravsh wrote:
> can this return an error? If so, should the return type be uint32_t?

No, it doesn't return an error and in fact return TPM_SUCCESS is wrong---it
should be return 1 or 0.  It may be nicer if it returned an error code and a
value.  Maybe I'll do that, although in practice it won't matter much.

[gauravsh, 2010-06-09 19:44:36]

LGTM

And I agree, it's time we moved tpm_lite to under vboot_reference.

On 2010/06/09 19:36:01, Luigi Semenzato wrote:
> PTAL.  Changed as suggested, except for the change to TlclIsOwned which
requires
> a parallel change to tpm_lite.  (Maybe it's time to move tpm_lite under
> vboot_reference?)
> 
> http://codereview.chromium.org/2711006/diff/19001/20001
> File tests/rollback_index_mock.c (right):
> 
> http://codereview.chromium.org/2711006/diff/19001/20001#newcode25
> tests/rollback_index_mock.c:25: int GetStoredVersions(int type, uint16_t*
> key_version, uint16_t* version) {
> On 2010/06/09 18:30:10, gauravsh wrote:
> > also modify return types (uint32_t) for all the mocked functions
> 
> Yes, sure.  I wonder why the compiler didn't catch this?
> 
> http://codereview.chromium.org/2711006/diff/19001/20005
> File vboot_firmware/lib/firmware_image_fw.c (right):
> 
> http://codereview.chromium.org/2711006/diff/19001/20005#newcode314
> vboot_firmware/lib/firmware_image_fw.c:314: return
> VERIFY_FIRMWARE_INTERNAL_ERROR;
> On 2010/06/09 18:30:10, gauravsh wrote:
> > would VERIFY_FIRMWARE_TPM_ERROR be a more accurate/informative description
of
> > this error message?
> 
> Sure!
> 
> http://codereview.chromium.org/2711006/diff/19001/20008
> File vboot_firmware/stub/tlcl.c (right):
> 
> http://codereview.chromium.org/2711006/diff/19001/20008#newcode28
> vboot_firmware/stub/tlcl.c:28: int TlclIsOwned(void) { return TPM_SUCCESS; }
> On 2010/06/09 18:30:10, gauravsh wrote:
> > can this return an error? If so, should the return type be uint32_t?
> 
> No, it doesn't return an error and in fact return TPM_SUCCESS is wrong---it
> should be return 1 or 0.  It may be nicer if it returned an error code and a
> value.  Maybe I'll do that, although in practice it won't matter much.

[Randall Spangler, 2010-06-10 15:40:45]

LGTM