VBoot Reference: Add version checking to for preventing rollbacks.

VBoot Reference: Add version checking to for preventing rollbacks.

This CL adds a new function VerifyFirmwareDriver_f() means to be a part of the RO firmware which determine which copy of the firmware to boot from. It is meant to ensure that a particular firmware is only booted if 1) it verifies successfully, 2) its version is newer or equal to current stored version. In addition, the driver function also updates the stored version if needed. 

Currently I am using the TLCL API with stub calls, (in fact, most of the TPM interaction is done in rollback_index.c which implements the actual version query/update API) used by the firmware.

[gauravsh, 2010-03-23 23:41:48]

Sending this out now to get some initial feedback. 

Randall: have a look at VerifyFirmwareDriver_f() and see if the boot path logic
matches what we discussed earlier.

[gauravsh, 2010-03-23 23:43:48]

To be clear, you should review it Luigi. Randall should look at the
VerifyFirmwareDriver_f() function.

[Luigi Semenzato, 2010-03-24 00:20:25]

Thanks for the clarification!

On Tue, Mar 23, 2010 at 4:43 PM,  <gauravsh@chromium.org> wrote:
> To be clear, you should review it Luigi. Randall should look at the
> VerifyFirmwareDriver_f() function.
>
> http://codereview.chromium.org/1241002
>

[Luigi Semenzato, 2010-03-24 03:34:47]

Gaurav, looks good.  Most comments are nit.  There are a couple of slightly more
substantive comments, but mostly for discussion---nothing really needs to
change.

http://codereview.chromium.org/1241002/diff/1/3
File src/platform/vboot_reference/common/tlcl_stub.c (right):

http://codereview.chromium.org/1241002/diff/1/3#newcode14
src/platform/vboot_reference/common/tlcl_stub.c:14: uint32_t TlclWrite(uint32_t
index, uint8_t *data, uint32_t length) { return 0; }
Perhaps the comment in tlcl.h is misleading---you should probably return
TPM_SUCCESS for Write and Read, not 0.

http://codereview.chromium.org/1241002/diff/1/6
File src/platform/vboot_reference/include/utility.h (right):

http://codereview.chromium.org/1241002/diff/1/6#newcode16
src/platform/vboot_reference/include/utility.h:16: /* Combine [msw] and [lsw]
uint16s to a uint32_t with it's [msw] and
it's -> its

http://codereview.chromium.org/1241002/diff/1/6#newcode22
src/platform/vboot_reference/include/utility.h:22: ((uint32_t) ((lsw) & 0xFF)))
Since the byte order doesn't change, you could do this more concisely ((msw <<
16) && (lsw && 0xffff))

http://codereview.chromium.org/1241002/diff/1/8
File src/platform/vboot_reference/utils/Makefile (right):

http://codereview.chromium.org/1241002/diff/1/8#newcode23
src/platform/vboot_reference/utils/Makefile:23: 
A space leaked in here.  Maybe I shouldn't even mention it.

http://codereview.chromium.org/1241002/diff/1/8#newcode44
src/platform/vboot_reference/utils/Makefile:44: $(CC) $(CFLAGS) $(INCLUDES) -c
$< -o $@
Almost time to add a default .c.o rule.  I guess you would still special case
the -ansi.

http://codereview.chromium.org/1241002/diff/1/9
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/1241002/diff/1/9#newcode25
src/platform/vboot_reference/utils/firmware_image.c:25: 
Did you mean to add an extra line here?

http://codereview.chromium.org/1241002/diff/1/9#newcode322
src/platform/vboot_reference/utils/firmware_image.c:322: "Firmware Version
Rollback."
The comment in the .h file suggests that VERIFY_FIRMWARE_MAX is a catch-all. 
Does it mean you use it as a generic error?  In that case it should have a
string here.  Or maybe the comment means something else?

http://codereview.chromium.org/1241002/diff/1/9#newcode689
src/platform/vboot_reference/utils/firmware_image.c:689: firmwareA_is_verified =
1;
Does google style allow this, or should braces always be used?

http://codereview.chromium.org/1241002/diff/1/9#newcode698
src/platform/vboot_reference/utils/firmware_image.c:698: (uint16_t)
(min_lversion >> 16));
If you just shift by 16 here instead of combining bytes, then it seems that the
CombineVersions macro should also shift by 16.

http://codereview.chromium.org/1241002/diff/1/9#newcode699
src/platform/vboot_reference/utils/firmware_image.c:699:
WriteStoredVersion(FIRMWARE_VERSION,
0xffff?

http://codereview.chromium.org/1241002/diff/1/9#newcode700
src/platform/vboot_reference/utils/firmware_image.c:700: (uint16_t)
(min_lversion & 0x00FF));
0xffff?

http://codereview.chromium.org/1241002/diff/1/9#newcode708
src/platform/vboot_reference/utils/firmware_image.c:708:
LockStoredVersion(FIRMWARE_VERSION);
Since the keys get locked at the same time, and you're using them both
separately and in the 32-bit combined form, I am wondering if we'd be better off
storing them in the 32-bit form.  I wish we already knew whether we should try
to minimize the number of locking ops---that's really the only reason to worry
about this.  Perhaps we should be optimistic now and change it later if we need
to.

http://codereview.chromium.org/1241002/diff/1/9#newcode726
src/platform/vboot_reference/utils/firmware_image.c:726: }
join lines?

http://codereview.chromium.org/1241002/diff/1/10
File src/platform/vboot_reference/utils/rollback_index.c (right):

http://codereview.chromium.org/1241002/diff/1/10#newcode21
src/platform/vboot_reference/utils/rollback_index.c:21: /* From TLCL
testsuite/readonly.c */
Thank you for the acknowledgement, but perhaps this comment is not needed.

http://codereview.chromium.org/1241002/diff/1/10#newcode71
src/platform/vboot_reference/utils/rollback_index.c:71: TlclSelftestfull();  /*
TODO(gauravsh): This should probably be deferred. */
TlclStartup should be called before the firmware initializes the memory
controller, so the selftest can run in parallel with that.  Here we should just
call TlclSelftestFull to make sure the self test has completed---unless we want
to rely on the NVRAM operations being available before the selftest completes. 
Maybe we should add this information to the comment?

http://codereview.chromium.org/1241002/diff/1/10#newcode74
src/platform/vboot_reference/utils/rollback_index.c:74: fprintf(stderr, "Ho Ho
Ho! We must jump to recovery.");
output to stderr, but InitializeSpaces outputs to stdout.  Make consistent?  Or
remove the printf in InitializeSpaces?

http://codereview.chromium.org/1241002/diff/1/10#newcode125
src/platform/vboot_reference/utils/rollback_index.c:125: switch (type) {
How much error checking do we want to do?  If the WriteLock fails (cannot
happen!) we might want to enter recovery mode.  I don't even think tlcl returns
an error here, but it certainly can.

[Luigi Semenzato, 2010-03-24 05:01:54]

I forgot to mention: another reason to minimize the number of locking
operations is that they take a long time.  I have measured 29ms per
locking operation.  It's a coarse measurement, but not too far off---I
am guessing +/-5ms.  Sorry I don't have more precise numbers yet.

On Tue, Mar 23, 2010 at 8:34 PM,  <semenzato@chromium.org> wrote:
> Gaurav, looks good.  Most comments are nit.  There are a couple of slightly
> more
> substantive comments, but mostly for discussion---nothing really needs to
> change.
>
>
> http://codereview.chromium.org/1241002/diff/1/3
> File src/platform/vboot_reference/common/tlcl_stub.c (right):
>
> http://codereview.chromium.org/1241002/diff/1/3#newcode14
> src/platform/vboot_reference/common/tlcl_stub.c:14: uint32_t
> TlclWrite(uint32_t index, uint8_t *data, uint32_t length) { return 0; }
> Perhaps the comment in tlcl.h is misleading---you should probably return
> TPM_SUCCESS for Write and Read, not 0.
>
> http://codereview.chromium.org/1241002/diff/1/6
> File src/platform/vboot_reference/include/utility.h (right):
>
> http://codereview.chromium.org/1241002/diff/1/6#newcode16
> src/platform/vboot_reference/include/utility.h:16: /* Combine [msw] and
> [lsw] uint16s to a uint32_t with it's [msw] and
> it's -> its
>
> http://codereview.chromium.org/1241002/diff/1/6#newcode22
> src/platform/vboot_reference/include/utility.h:22: ((uint32_t) ((lsw) &
> 0xFF)))
> Since the byte order doesn't change, you could do this more concisely
> ((msw << 16) && (lsw && 0xffff))
>
> http://codereview.chromium.org/1241002/diff/1/8
> File src/platform/vboot_reference/utils/Makefile (right):
>
> http://codereview.chromium.org/1241002/diff/1/8#newcode23
> src/platform/vboot_reference/utils/Makefile:23:
> A space leaked in here.  Maybe I shouldn't even mention it.
>
> http://codereview.chromium.org/1241002/diff/1/8#newcode44
> src/platform/vboot_reference/utils/Makefile:44: $(CC) $(CFLAGS)
> $(INCLUDES) -c $< -o $@
> Almost time to add a default .c.o rule.  I guess you would still special
> case the -ansi.
>
> http://codereview.chromium.org/1241002/diff/1/9
> File src/platform/vboot_reference/utils/firmware_image.c (right):
>
> http://codereview.chromium.org/1241002/diff/1/9#newcode25
> src/platform/vboot_reference/utils/firmware_image.c:25:
> Did you mean to add an extra line here?
>
> http://codereview.chromium.org/1241002/diff/1/9#newcode322
> src/platform/vboot_reference/utils/firmware_image.c:322: "Firmware
> Version Rollback."
> The comment in the .h file suggests that VERIFY_FIRMWARE_MAX is a
> catch-all.  Does it mean you use it as a generic error?  In that case it
> should have a string here.  Or maybe the comment means something else?
>
> http://codereview.chromium.org/1241002/diff/1/9#newcode689
> src/platform/vboot_reference/utils/firmware_image.c:689:
> firmwareA_is_verified = 1;
> Does google style allow this, or should braces always be used?
>
> http://codereview.chromium.org/1241002/diff/1/9#newcode698
> src/platform/vboot_reference/utils/firmware_image.c:698: (uint16_t)
> (min_lversion >> 16));
> If you just shift by 16 here instead of combining bytes, then it seems
> that the CombineVersions macro should also shift by 16.
>
> http://codereview.chromium.org/1241002/diff/1/9#newcode699
> src/platform/vboot_reference/utils/firmware_image.c:699:
> WriteStoredVersion(FIRMWARE_VERSION,
> 0xffff?
>
> http://codereview.chromium.org/1241002/diff/1/9#newcode700
> src/platform/vboot_reference/utils/firmware_image.c:700: (uint16_t)
> (min_lversion & 0x00FF));
> 0xffff?
>
> http://codereview.chromium.org/1241002/diff/1/9#newcode708
> src/platform/vboot_reference/utils/firmware_image.c:708:
> LockStoredVersion(FIRMWARE_VERSION);
> Since the keys get locked at the same time, and you're using them both
> separately and in the 32-bit combined form, I am wondering if we'd be
> better off storing them in the 32-bit form.  I wish we already knew
> whether we should try to minimize the number of locking ops---that's
> really the only reason to worry about this.  Perhaps we should be
> optimistic now and change it later if we need to.
>
> http://codereview.chromium.org/1241002/diff/1/9#newcode726
> src/platform/vboot_reference/utils/firmware_image.c:726: }
> join lines?
>
> http://codereview.chromium.org/1241002/diff/1/10
> File src/platform/vboot_reference/utils/rollback_index.c (right):
>
> http://codereview.chromium.org/1241002/diff/1/10#newcode21
> src/platform/vboot_reference/utils/rollback_index.c:21: /* From TLCL
> testsuite/readonly.c */
> Thank you for the acknowledgement, but perhaps this comment is not
> needed.
>
> http://codereview.chromium.org/1241002/diff/1/10#newcode71
> src/platform/vboot_reference/utils/rollback_index.c:71:
> TlclSelftestfull();  /* TODO(gauravsh): This should probably be
> deferred. */
> TlclStartup should be called before the firmware initializes the memory
> controller, so the selftest can run in parallel with that.  Here we
> should just call TlclSelftestFull to make sure the self test has
> completed---unless we want to rely on the NVRAM operations being
> available before the selftest completes.  Maybe we should add this
> information to the comment?
>
> http://codereview.chromium.org/1241002/diff/1/10#newcode74
> src/platform/vboot_reference/utils/rollback_index.c:74: fprintf(stderr,
> "Ho Ho Ho! We must jump to recovery.");
> output to stderr, but InitializeSpaces outputs to stdout.  Make
> consistent?  Or remove the printf in InitializeSpaces?
>
> http://codereview.chromium.org/1241002/diff/1/10#newcode125
> src/platform/vboot_reference/utils/rollback_index.c:125: switch (type) {
> How much error checking do we want to do?  If the WriteLock fails
> (cannot happen!) we might want to enter recovery mode.  I don't even
> think tlcl returns an error here, but it certainly can.
>
> http://codereview.chromium.org/1241002
>

[gauravsh, 2010-03-24 05:04:32]

I have addressed all your comments so far. I also uploaded a new patch set which
adds tests to check rollback prevention functionality. It's still incomplete -
but I'd like to hear feedback on the kind of additional things that can be
tested.

http://codereview.chromium.org/1241002/diff/1/3
File src/platform/vboot_reference/common/tlcl_stub.c (right):

http://codereview.chromium.org/1241002/diff/1/3#newcode14
src/platform/vboot_reference/common/tlcl_stub.c:14: uint32_t TlclWrite(uint32_t
index, uint8_t *data, uint32_t length) { return 0; }
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> Perhaps the comment in tlcl.h is misleading---you should probably return
> TPM_SUCCESS for Write and Read, not 0.

Done.

http://codereview.chromium.org/1241002/diff/1/6
File src/platform/vboot_reference/include/utility.h (right):

http://codereview.chromium.org/1241002/diff/1/6#newcode16
src/platform/vboot_reference/include/utility.h:16: /* Combine [msw] and [lsw]
uint16s to a uint32_t with it's [msw] and
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> it's -> its

Oops. Done.

http://codereview.chromium.org/1241002/diff/1/6#newcode22
src/platform/vboot_reference/include/utility.h:22: ((uint32_t) ((lsw) & 0xFF)))
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> Since the byte order doesn't change, you could do this more concisely ((msw <<
> 16) && (lsw && 0xffff))

Done.

http://codereview.chromium.org/1241002/diff/1/8
File src/platform/vboot_reference/utils/Makefile (right):

http://codereview.chromium.org/1241002/diff/1/8#newcode23
src/platform/vboot_reference/utils/Makefile:23: 
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> A space leaked in here.  Maybe I shouldn't even mention it.

Done.

http://codereview.chromium.org/1241002/diff/1/8#newcode44
src/platform/vboot_reference/utils/Makefile:44: $(CC) $(CFLAGS) $(INCLUDES) -c
$< -o $@
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> Almost time to add a default .c.o rule.  I guess you would still special case
> the -ansi.

Done.

http://codereview.chromium.org/1241002/diff/1/9
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/1241002/diff/1/9#newcode25
src/platform/vboot_reference/utils/firmware_image.c:25: 
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> Did you mean to add an extra line here?
No, fixed.

http://codereview.chromium.org/1241002/diff/1/9#newcode322
src/platform/vboot_reference/utils/firmware_image.c:322: "Firmware Version
Rollback."
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> The comment in the .h file suggests that VERIFY_FIRMWARE_MAX is a catch-all. 
> Does it mean you use it as a generic error?  In that case it should have a
> string here.  Or maybe the comment means something else?

Well, it does act as a generic catch-all although currently unused. But mostly
it keeps track of the total number of errors (see its use above as the size of
the array). Would you suggest a different comment?

http://codereview.chromium.org/1241002/diff/1/9#newcode689
src/platform/vboot_reference/utils/firmware_image.c:689: firmwareA_is_verified =
1;
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> Does google style allow this, or should braces always be used?
It allows this. Braces must be used if any one of the if-else-if- clauses uses
it.

http://codereview.chromium.org/1241002/diff/1/9#newcode698
src/platform/vboot_reference/utils/firmware_image.c:698: (uint16_t)
(min_lversion >> 16));
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> If you just shift by 16 here instead of combining bytes, then it seems that
the
> CombineVersions macro should also shift by 16.
I don't understand this comment. Will this not work?

http://codereview.chromium.org/1241002/diff/1/9#newcode699
src/platform/vboot_reference/utils/firmware_image.c:699:
WriteStoredVersion(FIRMWARE_VERSION,
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> 0xffff?
Aren't those two equivalent since the MSW is discarded?

http://codereview.chromium.org/1241002/diff/1/9#newcode708
src/platform/vboot_reference/utils/firmware_image.c:708:
LockStoredVersion(FIRMWARE_VERSION);
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> Since the keys get locked at the same time, and you're using them both
> separately and in the 32-bit combined form, I am wondering if we'd be better
off
> storing them in the 32-bit form.  I wish we already knew whether we should try
> to minimize the number of locking ops---that's really the only reason to worry
> about this.  Perhaps we should be optimistic now and change it later if we
need
> to.

Sounds good. I will add a TODO to that effect.

http://codereview.chromium.org/1241002/diff/1/9#newcode726
src/platform/vboot_reference/utils/firmware_image.c:726: }
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> join lines?

Done.

http://codereview.chromium.org/1241002/diff/1/10
File src/platform/vboot_reference/utils/rollback_index.c (right):

http://codereview.chromium.org/1241002/diff/1/10#newcode21
src/platform/vboot_reference/utils/rollback_index.c:21: /* From TLCL
testsuite/readonly.c */
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> Thank you for the acknowledgement, but perhaps this comment is not needed.
Ok. :)

http://codereview.chromium.org/1241002/diff/1/10#newcode71
src/platform/vboot_reference/utils/rollback_index.c:71: TlclSelftestfull();  /*
TODO(gauravsh): This should probably be deferred. */
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> TlclStartup should be called before the firmware initializes the memory
> controller, so the selftest can run in parallel with that.  Here we should
just
> call TlclSelftestFull to make sure the self test has completed---unless we
want
> to rely on the NVRAM operations being available before the selftest completes.

> Maybe we should add this information to the comment?

I have added the above statement verbatim as a comment. :)

http://codereview.chromium.org/1241002/diff/1/10#newcode74
src/platform/vboot_reference/utils/rollback_index.c:74: fprintf(stderr, "Ho Ho
Ho! We must jump to recovery.");
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> output to stderr, but InitializeSpaces outputs to stdout.  Make consistent? 
Or
> remove the printf in InitializeSpaces?

If it's an error, it must be output to stderr. However I don't think this really
matters, as all this will be done in firmware land.

I will most likely remove all fprintf's from the code and make it call something
like your debug() or warn() macro instead. Easy to remove it using a compile
time flag then.

http://codereview.chromium.org/1241002/diff/1/10#newcode125
src/platform/vboot_reference/utils/rollback_index.c:125: switch (type) {
On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> How much error checking do we want to do?  If the WriteLock fails (cannot
> happen!) we might want to enter recovery mode.  I don't even think tlcl
returns
> an error here, but it certainly can.

TlclWriteLock doesn't tell whether an error occured. I have added this as a TODO
for now.

[gauravsh, 2010-03-24 05:07:21]

On Tue, Mar 23, 2010 at 10:01 PM, Luigi Semenzato
<semenzato@chromium.org> wrote:
> I forgot to mention: another reason to minimize the number of locking
> operations is that they take a long time.  I have measured 29ms per
> locking operation.  It's a coarse measurement, but not too far off---I
> am guessing +/-5ms.  Sorry I don't have more precise numbers yet.

I think the way we are using the indices, we might just be able to get
away with using a single 32-bit space. In that case, we can combine
them together. That will sure save us all the bit gymnastics of
concatenating two 16-bit ints to get the logical version!

>
> On Tue, Mar 23, 2010 at 8:34 PM,  <semenzato@chromium.org> wrote:
>> Gaurav, looks good.  Most comments are nit.  There are a couple of slightly
>> more
>> substantive comments, but mostly for discussion---nothing really needs to
>> change.
>>
>>
>> http://codereview.chromium.org/1241002/diff/1/3
>> File src/platform/vboot_reference/common/tlcl_stub.c (right):
>>
>> http://codereview.chromium.org/1241002/diff/1/3#newcode14
>> src/platform/vboot_reference/common/tlcl_stub.c:14: uint32_t
>> TlclWrite(uint32_t index, uint8_t *data, uint32_t length) { return 0; }
>> Perhaps the comment in tlcl.h is misleading---you should probably return
>> TPM_SUCCESS for Write and Read, not 0.
>>
>> http://codereview.chromium.org/1241002/diff/1/6
>> File src/platform/vboot_reference/include/utility.h (right):
>>
>> http://codereview.chromium.org/1241002/diff/1/6#newcode16
>> src/platform/vboot_reference/include/utility.h:16: /* Combine [msw] and
>> [lsw] uint16s to a uint32_t with it's [msw] and
>> it's -> its
>>
>> http://codereview.chromium.org/1241002/diff/1/6#newcode22
>> src/platform/vboot_reference/include/utility.h:22: ((uint32_t) ((lsw) &
>> 0xFF)))
>> Since the byte order doesn't change, you could do this more concisely
>> ((msw << 16) && (lsw && 0xffff))
>>
>> http://codereview.chromium.org/1241002/diff/1/8
>> File src/platform/vboot_reference/utils/Makefile (right):
>>
>> http://codereview.chromium.org/1241002/diff/1/8#newcode23
>> src/platform/vboot_reference/utils/Makefile:23:
>> A space leaked in here.  Maybe I shouldn't even mention it.
>>
>> http://codereview.chromium.org/1241002/diff/1/8#newcode44
>> src/platform/vboot_reference/utils/Makefile:44: $(CC) $(CFLAGS)
>> $(INCLUDES) -c $< -o $@
>> Almost time to add a default .c.o rule.  I guess you would still special
>> case the -ansi.
>>
>> http://codereview.chromium.org/1241002/diff/1/9
>> File src/platform/vboot_reference/utils/firmware_image.c (right):
>>
>> http://codereview.chromium.org/1241002/diff/1/9#newcode25
>> src/platform/vboot_reference/utils/firmware_image.c:25:
>> Did you mean to add an extra line here?
>>
>> http://codereview.chromium.org/1241002/diff/1/9#newcode322
>> src/platform/vboot_reference/utils/firmware_image.c:322: "Firmware
>> Version Rollback."
>> The comment in the .h file suggests that VERIFY_FIRMWARE_MAX is a
>> catch-all.  Does it mean you use it as a generic error?  In that case it
>> should have a string here.  Or maybe the comment means something else?
>>
>> http://codereview.chromium.org/1241002/diff/1/9#newcode689
>> src/platform/vboot_reference/utils/firmware_image.c:689:
>> firmwareA_is_verified = 1;
>> Does google style allow this, or should braces always be used?
>>
>> http://codereview.chromium.org/1241002/diff/1/9#newcode698
>> src/platform/vboot_reference/utils/firmware_image.c:698: (uint16_t)
>> (min_lversion >> 16));
>> If you just shift by 16 here instead of combining bytes, then it seems
>> that the CombineVersions macro should also shift by 16.
>>
>> http://codereview.chromium.org/1241002/diff/1/9#newcode699
>> src/platform/vboot_reference/utils/firmware_image.c:699:
>> WriteStoredVersion(FIRMWARE_VERSION,
>> 0xffff?
>>
>> http://codereview.chromium.org/1241002/diff/1/9#newcode700
>> src/platform/vboot_reference/utils/firmware_image.c:700: (uint16_t)
>> (min_lversion & 0x00FF));
>> 0xffff?
>>
>> http://codereview.chromium.org/1241002/diff/1/9#newcode708
>> src/platform/vboot_reference/utils/firmware_image.c:708:
>> LockStoredVersion(FIRMWARE_VERSION);
>> Since the keys get locked at the same time, and you're using them both
>> separately and in the 32-bit combined form, I am wondering if we'd be
>> better off storing them in the 32-bit form.  I wish we already knew
>> whether we should try to minimize the number of locking ops---that's
>> really the only reason to worry about this.  Perhaps we should be
>> optimistic now and change it later if we need to.
>>
>> http://codereview.chromium.org/1241002/diff/1/9#newcode726
>> src/platform/vboot_reference/utils/firmware_image.c:726: }
>> join lines?
>>
>> http://codereview.chromium.org/1241002/diff/1/10
>> File src/platform/vboot_reference/utils/rollback_index.c (right):
>>
>> http://codereview.chromium.org/1241002/diff/1/10#newcode21
>> src/platform/vboot_reference/utils/rollback_index.c:21: /* From TLCL
>> testsuite/readonly.c */
>> Thank you for the acknowledgement, but perhaps this comment is not
>> needed.
>>
>> http://codereview.chromium.org/1241002/diff/1/10#newcode71
>> src/platform/vboot_reference/utils/rollback_index.c:71:
>> TlclSelftestfull();  /* TODO(gauravsh): This should probably be
>> deferred. */
>> TlclStartup should be called before the firmware initializes the memory
>> controller, so the selftest can run in parallel with that.  Here we
>> should just call TlclSelftestFull to make sure the self test has
>> completed---unless we want to rely on the NVRAM operations being
>> available before the selftest completes.  Maybe we should add this
>> information to the comment?
>>
>> http://codereview.chromium.org/1241002/diff/1/10#newcode74
>> src/platform/vboot_reference/utils/rollback_index.c:74: fprintf(stderr,
>> "Ho Ho Ho! We must jump to recovery.");
>> output to stderr, but InitializeSpaces outputs to stdout.  Make
>> consistent?  Or remove the printf in InitializeSpaces?
>>
>> http://codereview.chromium.org/1241002/diff/1/10#newcode125
>> src/platform/vboot_reference/utils/rollback_index.c:125: switch (type) {
>> How much error checking do we want to do?  If the WriteLock fails
>> (cannot happen!) we might want to enter recovery mode.  I don't even
>> think tlcl returns an error here, but it certainly can.
>>
>> http://codereview.chromium.org/1241002
>>
>



-- 
-g

[Luigi Semenzato, 2010-03-24 06:01:52]

LGTM after you address these issues, pretty please and with sugar on top (quote
from what movie?)

1. remove NUM_FIRMWARE_COPIES or explain why you need it
2. remove the "catch all" comment from VERIFY_FIRMWARE_MAX or add a string for
that index in the array
3. we reach an agreement on 0xff vs. 0xffff.

http://codereview.chromium.org/1241002/diff/1/9
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/1241002/diff/1/9#newcode29
src/platform/vboot_reference/utils/firmware_image.c:29: #define
NUM_FIRMWARE_COPIES 2
This isn't used anywhere, is it?

http://codereview.chromium.org/1241002/diff/1/9#newcode322
src/platform/vboot_reference/utils/firmware_image.c:322: "Firmware Version
Rollback."
On 2010/03/24 05:04:34, gauravsh wrote:
> On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> > The comment in the .h file suggests that VERIFY_FIRMWARE_MAX is a catch-all.

> > Does it mean you use it as a generic error?  In that case it should have a
> > string here.  Or maybe the comment means something else?
> 
> Well, it does act as a generic catch-all although currently unused. But mostly
> it keeps track of the total number of errors (see its use above as the size of
> the array). Would you suggest a different comment?

It's really nit, but someone who reads the comment in the .h file might use
VERIFY_FIRMWARE_MAX as an error code without realizing that there is no
corresponding error string (because the string array is in a different file),
and you'll get a segv when the error occurs.  I would use a different number for
the catch-all, 1 for instance, or I would not have one.

http://codereview.chromium.org/1241002/diff/1/9#newcode699
src/platform/vboot_reference/utils/firmware_image.c:699:
WriteStoredVersion(FIRMWARE_VERSION,
On 2010/03/24 05:04:34, gauravsh wrote:
> On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> > 0xffff?
> Aren't those two equivalent since the MSW is discarded?
> 

Are we off by a factor of 2?  0xff is 8 bits, 0xffff is 16 bits.

http://codereview.chromium.org/1241002/diff/1/10
File src/platform/vboot_reference/utils/rollback_index.c (right):

http://codereview.chromium.org/1241002/diff/1/10#newcode71
src/platform/vboot_reference/utils/rollback_index.c:71: TlclSelftestfull();  /*
TODO(gauravsh): This should probably be deferred. */
On 2010/03/24 05:04:34, gauravsh wrote:
> On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> > TlclStartup should be called before the firmware initializes the memory
> > controller, so the selftest can run in parallel with that.  Here we should
> just
> > call TlclSelftestFull to make sure the self test has completed---unless we
> want
> > to rely on the NVRAM operations being available before the selftest
completes.
> 
> > Maybe we should add this information to the comment?
> 
> I have added the above statement verbatim as a comment. :)
> 

You are too kind :)

http://codereview.chromium.org/1241002/diff/1/10#newcode74
src/platform/vboot_reference/utils/rollback_index.c:74: fprintf(stderr, "Ho Ho
Ho! We must jump to recovery.");
On 2010/03/24 05:04:34, gauravsh wrote:
> On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> > output to stderr, but InitializeSpaces outputs to stdout.  Make consistent? 
> Or
> > remove the printf in InitializeSpaces?
> 
> If it's an error, it must be output to stderr. However I don't think this
really
> matters, as all this will be done in firmware land.
> 
> I will most likely remove all fprintf's from the code and make it call
something
> like your debug() or warn() macro instead. Easy to remove it using a compile
> time flag then. 
> 

Sounds good.

http://codereview.chromium.org/1241002/diff/1/10#newcode125
src/platform/vboot_reference/utils/rollback_index.c:125: switch (type) {
On 2010/03/24 05:04:34, gauravsh wrote:
> On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> > How much error checking do we want to do?  If the WriteLock fails (cannot
> > happen!) we might want to enter recovery mode.  I don't even think tlcl
> returns
> > an error here, but it certainly can.
> 
> TlclWriteLock doesn't tell whether an error occured. I have added this as a
TODO
> for now.
> 

Sounds good.

http://codereview.chromium.org/1241002/diff/13001/14011
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/1241002/diff/13001/14011#newcode28
src/platform/vboot_reference/utils/firmware_image.c:28: #define
NUM_FIRMWARE_COPIES 2
Is this used anywhere?

[Randall Spangler, 2010-03-24 17:33:28]

lgtm

[gauravsh, 2010-03-24 19:20:33]

BTW, did you also review the tests that I added with my last patchset?

http://codereview.chromium.org/1241002/diff/1/9
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/1241002/diff/1/9#newcode29
src/platform/vboot_reference/utils/firmware_image.c:29: #define
NUM_FIRMWARE_COPIES 2
On 2010/03/24 06:01:52, Luigi Semenzato wrote:
> This isn't used anywhere, is it?
Nope, removed.

http://codereview.chromium.org/1241002/diff/1/9#newcode322
src/platform/vboot_reference/utils/firmware_image.c:322: "Firmware Version
Rollback."
On 2010/03/24 06:01:52, Luigi Semenzato wrote:
> On 2010/03/24 05:04:34, gauravsh wrote:
> > On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> > > The comment in the .h file suggests that VERIFY_FIRMWARE_MAX is a
catch-all.
> 
> > > Does it mean you use it as a generic error?  In that case it should have a
> > > string here.  Or maybe the comment means something else?
> > 
> > Well, it does act as a generic catch-all although currently unused. But
mostly
> > it keeps track of the total number of errors (see its use above as the size
of
> > the array). Would you suggest a different comment?
> 
> It's really nit, but someone who reads the comment in the .h file might use
> VERIFY_FIRMWARE_MAX as an error code without realizing that there is no
> corresponding error string (because the string array is in a different file),
> and you'll get a segv when the error occurs.  I would use a different number
for
> the catch-all, 1 for instance, or I would not have one.
> 
> 

I changed the comment to say it refers to the total number of errors
VerifyFirmware can return.

http://codereview.chromium.org/1241002/diff/1/9#newcode699
src/platform/vboot_reference/utils/firmware_image.c:699:
WriteStoredVersion(FIRMWARE_VERSION,
On 2010/03/24 06:01:52, Luigi Semenzato wrote:
> On 2010/03/24 05:04:34, gauravsh wrote:
> > On 2010/03/24 03:34:47, Luigi Semenzato wrote:
> > > 0xffff?
> > Aren't those two equivalent since the MSW is discarded?
> > 
> 
> Are we off by a factor of 2?  0xff is 8 bits, 0xffff is 16 bits.

Oops, my bad. Yes, you are right.

[Luigi Semenzato, 2010-03-24 20:41:59]

Thank you for pointing out I missed a couple of files.

LGTM after fixing some nit as needed.

http://codereview.chromium.org/1241002/diff/22001/23007
File src/platform/vboot_reference/tests/firmware_image_tests.c (right):

http://codereview.chromium.org/1241002/diff/22001/23007#newcode19
src/platform/vboot_reference/tests/firmware_image_tests.c:19: #define COL_RED
"\e[0;31m]"
This looks a bit strange: RED ends with a bracket and GREEN doesn't?

http://codereview.chromium.org/1241002/diff/22001/23007#newcode22
src/platform/vboot_reference/tests/firmware_image_tests.c:22: int TEST_EQ(int
result, int expected_result, char* testname) {
Do google conventions allow this function name?

http://codereview.chromium.org/1241002/diff/22001/23007#newcode24
src/platform/vboot_reference/tests/firmware_image_tests.c:24: fprintf(stderr,
"%s Test " COL_GREEN " PASSED\n" COL_STOP, testname);
Is it OK to change color after the newline?

http://codereview.chromium.org/1241002/diff/22001/23007#newcode26
src/platform/vboot_reference/tests/firmware_image_tests.c:26: }
Do google conventions allow breaking the "else" this way?

http://codereview.chromium.org/1241002/diff/22001/23007#newcode78
src/platform/vboot_reference/tests/firmware_image_tests.c:78: 
Delete blank line?

http://codereview.chromium.org/1241002/diff/22001/23007#newcode158
src/platform/vboot_reference/tests/firmware_image_tests.c:158: 
Delete two blank lines?

http://codereview.chromium.org/1241002/diff/22001/23008
File src/platform/vboot_reference/tests/firmware_rollback_tests.c (right):

http://codereview.chromium.org/1241002/diff/22001/23008#newcode20
src/platform/vboot_reference/tests/firmware_rollback_tests.c:20: * then the
image has a invalid signatures and will fail verification. */
"an invalid signature"

http://codereview.chromium.org/1241002/diff/22001/23008#newcode112
src/platform/vboot_reference/tests/firmware_rollback_tests.c:112: "Firmware A
(Corrupt with current version), FirmwareB (Valid with current version)");
Line too long.  Also below.

[gauravsh, 2010-03-24 20:49:01]

Fixed and submitted.

http://codereview.chromium.org/1241002/diff/22001/23007
File src/platform/vboot_reference/tests/firmware_image_tests.c (right):

http://codereview.chromium.org/1241002/diff/22001/23007#newcode19
src/platform/vboot_reference/tests/firmware_image_tests.c:19: #define COL_RED
"\e[0;31m]"
On 2010/03/24 20:42:00, Luigi Semenzato wrote:
> This looks a bit strange: RED ends with a bracket and GREEN doesn't?
Ahh, fixed.

http://codereview.chromium.org/1241002/diff/22001/23007#newcode22
src/platform/vboot_reference/tests/firmware_image_tests.c:22: int TEST_EQ(int
result, int expected_result, char* testname) {
On 2010/03/24 20:42:00, Luigi Semenzato wrote:
> Do google conventions allow this function name?
Well, since we are not using gtest, this is fine. Ideally, this would be
implemented as a macro.

http://codereview.chromium.org/1241002/diff/22001/23007#newcode24
src/platform/vboot_reference/tests/firmware_image_tests.c:24: fprintf(stderr,
"%s Test " COL_GREEN " PASSED\n" COL_STOP, testname);
On 2010/03/24 20:42:00, Luigi Semenzato wrote:
> Is it OK to change color after the newline?
Yes, doesn't make a difference.

http://codereview.chromium.org/1241002/diff/22001/23007#newcode26
src/platform/vboot_reference/tests/firmware_image_tests.c:26: }
On 2010/03/24 20:42:00, Luigi Semenzato wrote:
> Do google conventions allow breaking the "else" this way?

I think it's fine as long as it's consistent. But I have move it up.

http://codereview.chromium.org/1241002/diff/22001/23007#newcode78
src/platform/vboot_reference/tests/firmware_image_tests.c:78: 
On 2010/03/24 20:42:00, Luigi Semenzato wrote:
> Delete blank line?

Done.

http://codereview.chromium.org/1241002/diff/22001/23008
File src/platform/vboot_reference/tests/firmware_rollback_tests.c (right):

http://codereview.chromium.org/1241002/diff/22001/23008#newcode20
src/platform/vboot_reference/tests/firmware_rollback_tests.c:20: * then the
image has a invalid signatures and will fail verification. */
On 2010/03/24 20:42:00, Luigi Semenzato wrote:
> "an invalid signature"
Done.

http://codereview.chromium.org/1241002/diff/22001/23008#newcode112
src/platform/vboot_reference/tests/firmware_rollback_tests.c:112: "Firmware A
(Corrupt with current version), FirmwareB (Valid with current version)");
On 2010/03/24 20:42:00, Luigi Semenzato wrote:
> Line too long.  Also below.

Done.

[Luigi Semenzato, 2010-03-24 20:50:47]

There were other places where you used COL_RED and COL_GREEN, you may
want to check those too if you haven't already.  Sorry I didn't
mention it earlier.

On Wed, Mar 24, 2010 at 1:49 PM,  <gauravsh@chromium.org> wrote:
> Fixed and submitted.
>
>
> http://codereview.chromium.org/1241002/diff/22001/23007
> File src/platform/vboot_reference/tests/firmware_image_tests.c (right):
>
> http://codereview.chromium.org/1241002/diff/22001/23007#newcode19
> src/platform/vboot_reference/tests/firmware_image_tests.c:19: #define
> COL_RED "\e[0;31m]"
> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>
>> This looks a bit strange: RED ends with a bracket and GREEN doesn't?
>
> Ahh, fixed.
>
> http://codereview.chromium.org/1241002/diff/22001/23007#newcode22
> src/platform/vboot_reference/tests/firmware_image_tests.c:22: int
> TEST_EQ(int result, int expected_result, char* testname) {
> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>
>> Do google conventions allow this function name?
>
> Well, since we are not using gtest, this is fine. Ideally, this would be
> implemented as a macro.
>
> http://codereview.chromium.org/1241002/diff/22001/23007#newcode24
> src/platform/vboot_reference/tests/firmware_image_tests.c:24:
> fprintf(stderr, "%s Test " COL_GREEN " PASSED\n" COL_STOP, testname);
> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>
>> Is it OK to change color after the newline?
>
> Yes, doesn't make a difference.
>
> http://codereview.chromium.org/1241002/diff/22001/23007#newcode26
> src/platform/vboot_reference/tests/firmware_image_tests.c:26: }
> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>
>> Do google conventions allow breaking the "else" this way?
>
> I think it's fine as long as it's consistent. But I have move it up.
>
> http://codereview.chromium.org/1241002/diff/22001/23007#newcode78
> src/platform/vboot_reference/tests/firmware_image_tests.c:78:
> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>
>> Delete blank line?
>
> Done.
>
> http://codereview.chromium.org/1241002/diff/22001/23008
> File src/platform/vboot_reference/tests/firmware_rollback_tests.c
> (right):
>
> http://codereview.chromium.org/1241002/diff/22001/23008#newcode20
> src/platform/vboot_reference/tests/firmware_rollback_tests.c:20: * then
> the image has a invalid signatures and will fail verification. */
> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>
>> "an invalid signature"
>
> Done.
>
> http://codereview.chromium.org/1241002/diff/22001/23008#newcode112
> src/platform/vboot_reference/tests/firmware_rollback_tests.c:112:
> "Firmware A (Corrupt with current version), FirmwareB (Valid with
> current version)");
> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>
>> Line too long.  Also below.
>
> Done.
>
> http://codereview.chromium.org/1241002
>

[gauravsh, 2010-03-24 20:51:47]

On Wed, Mar 24, 2010 at 1:50 PM, Luigi Semenzato <semenzato@chromium.org> wrote:
> There were other places where you used COL_RED and COL_GREEN, you may
> want to check those too if you haven't already.  Sorry I didn't
> mention it earlier.
I am moving a lot of common functionality to test_common.c. So await a
new CL in a few.

>
> On Wed, Mar 24, 2010 at 1:49 PM,  <gauravsh@chromium.org> wrote:
>> Fixed and submitted.
>>
>>
>> http://codereview.chromium.org/1241002/diff/22001/23007
>> File src/platform/vboot_reference/tests/firmware_image_tests.c (right):
>>
>> http://codereview.chromium.org/1241002/diff/22001/23007#newcode19
>> src/platform/vboot_reference/tests/firmware_image_tests.c:19: #define
>> COL_RED "\e[0;31m]"
>> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>>
>>> This looks a bit strange: RED ends with a bracket and GREEN doesn't?
>>
>> Ahh, fixed.
>>
>> http://codereview.chromium.org/1241002/diff/22001/23007#newcode22
>> src/platform/vboot_reference/tests/firmware_image_tests.c:22: int
>> TEST_EQ(int result, int expected_result, char* testname) {
>> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>>
>>> Do google conventions allow this function name?
>>
>> Well, since we are not using gtest, this is fine. Ideally, this would be
>> implemented as a macro.
>>
>> http://codereview.chromium.org/1241002/diff/22001/23007#newcode24
>> src/platform/vboot_reference/tests/firmware_image_tests.c:24:
>> fprintf(stderr, "%s Test " COL_GREEN " PASSED\n" COL_STOP, testname);
>> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>>
>>> Is it OK to change color after the newline?
>>
>> Yes, doesn't make a difference.
>>
>> http://codereview.chromium.org/1241002/diff/22001/23007#newcode26
>> src/platform/vboot_reference/tests/firmware_image_tests.c:26: }
>> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>>
>>> Do google conventions allow breaking the "else" this way?
>>
>> I think it's fine as long as it's consistent. But I have move it up.
>>
>> http://codereview.chromium.org/1241002/diff/22001/23007#newcode78
>> src/platform/vboot_reference/tests/firmware_image_tests.c:78:
>> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>>
>>> Delete blank line?
>>
>> Done.
>>
>> http://codereview.chromium.org/1241002/diff/22001/23008
>> File src/platform/vboot_reference/tests/firmware_rollback_tests.c
>> (right):
>>
>> http://codereview.chromium.org/1241002/diff/22001/23008#newcode20
>> src/platform/vboot_reference/tests/firmware_rollback_tests.c:20: * then
>> the image has a invalid signatures and will fail verification. */
>> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>>
>>> "an invalid signature"
>>
>> Done.
>>
>> http://codereview.chromium.org/1241002/diff/22001/23008#newcode112
>> src/platform/vboot_reference/tests/firmware_rollback_tests.c:112:
>> "Firmware A (Corrupt with current version), FirmwareB (Valid with
>> current version)");
>> On 2010/03/24 20:42:00, Luigi Semenzato wrote:
>>>
>>> Line too long.  Also below.
>>
>> Done.
>>
>> http://codereview.chromium.org/1241002
>>
>



-- 
-g