Major refactoring of structures, with unit tests.

Major refactoring of structures, with unit tests.  This matches the doc I sent out earlier.

Firmware-side code for LoadKernel() is in place now.  LoadFirmware() replacement coming soon.

The new functions are implemented in parallel to the existing ones (i.e., everything that used to work still does).

[gauravsh, 2010-06-10 14:44:13]

Overall, I really like this change but given the size of the CL, I am still not
done going through everything. 

Moreover, since you have extensive unit tests, I haven't looked closely for bugs
in any API functions which are essentially rehashes of already existing ones but
use the new style structures.

I still need to have a closer look at LoadKernel2() and LoadFirmware2() but go
ahead and check this in after taking into account my comments below if my final
LGTM is blocking further progress.

Some comments follow.

http://codereview.chromium.org/2745007/diff/9001/10007
File host/lib/host_key.c (right):

http://codereview.chromium.org/2745007/diff/9001/10007#newcode83
host/lib/host_key.c:83: /* Allocate a new public key with space for a [key_size]
byte key. */
nit: there's a possibility of confusion with what we mean by [key_size] byte
key. In this case, we are referring to the size of stored key on  disk. But this
might be also be interpreted to mean the strength of the key. I had to check the
source to make sure I was interpreting it right. 

I noticed this elsewhere too. It would be useful to clarify that we mean the
storage size of the key.

http://codereview.chromium.org/2745007/diff/9001/10007#newcode102
host/lib/host_key.c:102: if (dest->key_size < src->key_size)
shouldn't we always return an error if dest key_size and src key_size are
different?

http://codereview.chromium.org/2745007/diff/9001/10020
File vboot_firmware/include/load_kernel_fw.h (right):

http://codereview.chromium.org/2745007/diff/9001/10020#newcode23
vboot_firmware/include/load_kernel_fw.h:23: #define BOOT_FLAG_DEVELOPER 0x01ULL 
/* Developer switch is on */
unsigned long long is not guaranteed to be uint64_t on all platform. would using
the UINT64_C() macro be more portable here?

http://codereview.chromium.org/2745007/diff/9001/10024
File vboot_firmware/lib/include/vboot_firmware.h (right):

http://codereview.chromium.org/2745007/diff/9001/10024#newcode23
vboot_firmware/lib/include/vboot_firmware.h:23: 
nit: extra new line

http://codereview.chromium.org/2745007/diff/9001/10025
File vboot_firmware/lib/include/vboot_kernel.h (right):

http://codereview.chromium.org/2745007/diff/9001/10025#newcode16
vboot_firmware/lib/include/vboot_kernel.h:16: #include "vboot_common.h"
nit: alphas order for includes

http://codereview.chromium.org/2745007/diff/9001/10025#newcode35
vboot_firmware/lib/include/vboot_kernel.h:35: 
nit: extra new line

http://codereview.chromium.org/2745007/diff/9001/10026
File vboot_firmware/lib/include/vboot_struct.h (right):

http://codereview.chromium.org/2745007/diff/9001/10026#newcode41
vboot_firmware/lib/include/vboot_struct.h:41: #define KEY_BLOCK_FLAG_DEVELOPER_0
 0x01ULL  /* Developer switch off */
UINT64_C(0x01) instead?

http://codereview.chromium.org/2745007/diff/9001/10028
File vboot_firmware/lib/vboot_common.c (right):

http://codereview.chromium.org/2745007/diff/9001/10028#newcode27
vboot_firmware/lib/vboot_common.c:27: uint64_t OffsetOf(const void *base, const
void *ptr) {
since this function assumes 64-bit pointers, would it be useful to have a
preprocessor check to ensure we are not compiling this code on a 32-bit system?

http://codereview.chromium.org/2745007/diff/9001/10029
File vboot_firmware/lib/vboot_firmware.c (right):

http://codereview.chromium.org/2745007/diff/9001/10029#newcode60
vboot_firmware/lib/vboot_firmware.c:60: if (index) {
nit/suggestion: since this is not a boolean check, it would be more clearer if
the check was if (0 == index) with the corresponding if-else blocks
interchanged.

http://codereview.chromium.org/2745007/diff/9001/10030
File vboot_firmware/lib/vboot_kernel.c (right):

http://codereview.chromium.org/2745007/diff/9001/10030#newcode1
vboot_firmware/lib/vboot_kernel.c:1: /* Copyright (c) 2010 The Chromium OS
Authors. All rights reserved.
TODO REview

http://codereview.chromium.org/2745007/diff/9001/10030#newcode14
vboot_firmware/lib/vboot_kernel.c:14: #include "vboot_kernel.h"
nit: this include should go first.

[gauravsh, 2010-06-10 16:04:26]

High level question about this. Are you also aiming to ensure that if the
verified boot structures change (say, we add a new field to the key header),
older versions of VerifyFirmware/Kernel() should still be able to verify a
verified boot image with this new structure?

One proposal I had was to leave some unused space (initialized to 0) in both the
key header and preamble blocks - if in the future, we decide to add new fields,
we just take bytes of storage out of that space.

On 2010/06/10 14:44:13, gauravsh wrote:
> Overall, I really like this change but given the size of the CL, I am still
not
> done going through everything. 
> 
> Moreover, since you have extensive unit tests, I haven't looked closely for
bugs
> in any API functions which are essentially rehashes of already existing ones
but
> use the new style structures.
> 
> I still need to have a closer look at LoadKernel2() and LoadFirmware2() but go
> ahead and check this in after taking into account my comments below if my
final
> LGTM is blocking further progress.
> 
> Some comments follow.
> 
> http://codereview.chromium.org/2745007/diff/9001/10007
> File host/lib/host_key.c (right):
> 
> http://codereview.chromium.org/2745007/diff/9001/10007#newcode83
> host/lib/host_key.c:83: /* Allocate a new public key with space for a
[key_size]
> byte key. */
> nit: there's a possibility of confusion with what we mean by [key_size] byte
> key. In this case, we are referring to the size of stored key on  disk. But
this
> might be also be interpreted to mean the strength of the key. I had to check
the
> source to make sure I was interpreting it right. 
> 
> I noticed this elsewhere too. It would be useful to clarify that we mean the
> storage size of the key.
> 
> http://codereview.chromium.org/2745007/diff/9001/10007#newcode102
> host/lib/host_key.c:102: if (dest->key_size < src->key_size)
> shouldn't we always return an error if dest key_size and src key_size are
> different?
> 
> http://codereview.chromium.org/2745007/diff/9001/10020
> File vboot_firmware/include/load_kernel_fw.h (right):
> 
> http://codereview.chromium.org/2745007/diff/9001/10020#newcode23
> vboot_firmware/include/load_kernel_fw.h:23: #define BOOT_FLAG_DEVELOPER
0x01ULL 
> /* Developer switch is on */
> unsigned long long is not guaranteed to be uint64_t on all platform. would
using
> the UINT64_C() macro be more portable here?
> 
> http://codereview.chromium.org/2745007/diff/9001/10024
> File vboot_firmware/lib/include/vboot_firmware.h (right):
> 
> http://codereview.chromium.org/2745007/diff/9001/10024#newcode23
> vboot_firmware/lib/include/vboot_firmware.h:23: 
> nit: extra new line
> 
> http://codereview.chromium.org/2745007/diff/9001/10025
> File vboot_firmware/lib/include/vboot_kernel.h (right):
> 
> http://codereview.chromium.org/2745007/diff/9001/10025#newcode16
> vboot_firmware/lib/include/vboot_kernel.h:16: #include "vboot_common.h"
> nit: alphas order for includes
> 
> http://codereview.chromium.org/2745007/diff/9001/10025#newcode35
> vboot_firmware/lib/include/vboot_kernel.h:35: 
> nit: extra new line
> 
> http://codereview.chromium.org/2745007/diff/9001/10026
> File vboot_firmware/lib/include/vboot_struct.h (right):
> 
> http://codereview.chromium.org/2745007/diff/9001/10026#newcode41
> vboot_firmware/lib/include/vboot_struct.h:41: #define
KEY_BLOCK_FLAG_DEVELOPER_0
>  0x01ULL  /* Developer switch off */
> UINT64_C(0x01) instead?
> 
> http://codereview.chromium.org/2745007/diff/9001/10028
> File vboot_firmware/lib/vboot_common.c (right):
> 
> http://codereview.chromium.org/2745007/diff/9001/10028#newcode27
> vboot_firmware/lib/vboot_common.c:27: uint64_t OffsetOf(const void *base,
const
> void *ptr) {
> since this function assumes 64-bit pointers, would it be useful to have a
> preprocessor check to ensure we are not compiling this code on a 32-bit
system?
> 
> http://codereview.chromium.org/2745007/diff/9001/10029
> File vboot_firmware/lib/vboot_firmware.c (right):
> 
> http://codereview.chromium.org/2745007/diff/9001/10029#newcode60
> vboot_firmware/lib/vboot_firmware.c:60: if (index) {
> nit/suggestion: since this is not a boolean check, it would be more clearer if
> the check was if (0 == index) with the corresponding if-else blocks
> interchanged.
> 
> http://codereview.chromium.org/2745007/diff/9001/10030
> File vboot_firmware/lib/vboot_kernel.c (right):
> 
> http://codereview.chromium.org/2745007/diff/9001/10030#newcode1
> vboot_firmware/lib/vboot_kernel.c:1: /* Copyright (c) 2010 The Chromium OS
> Authors. All rights reserved.
> TODO REview
> 
> http://codereview.chromium.org/2745007/diff/9001/10030#newcode14
> vboot_firmware/lib/vboot_kernel.c:14: #include "vboot_kernel.h"
> nit: this include should go first.

[Randall Spangler, 2010-06-10 16:20:55]

On Thu, Jun 10, 2010 at 9:04 AM, <gauravsh@chromium.org> wrote:

> High level question about this. Are you also aiming to ensure that if the
> verified boot structures change (say, we add a new field to the key
> header),
> older versions of VerifyFirmware/Kernel() should still be able to verify a
> verified boot image with this new structure?
>

Yes.

An older parser only knows about its struct size, so ignores new fields.

A new parser can see that header_minor_version is less than expected, and
knows what fields were added since that version so it can pick reasonable
defaults (which don't have to be 0).  This also works even if the new fields
are variable-length (such as adding the kernel subkey to the firmware
preamble), since all the variable-length data is explicitly pointed to by
the offsets in the VbSignature and VbPublicKey sub-structs.

Thanks,
    Randall


>
> One proposal I had was to leave some unused space (initialized to 0) in
> both the
> key header and preamble blocks - if in the future, we decide to add new
> fields,
> we just take bytes of storage out of that space.
>
>
> On 2010/06/10 14:44:13, gauravsh wrote:
>
>> Overall, I really like this change but given the size of the CL, I am
>> still
>>
> not
>
>> done going through everything.
>>
>
>  Moreover, since you have extensive unit tests, I haven't looked closely
>> for
>>
> bugs
>
>> in any API functions which are essentially rehashes of already existing
>> ones
>>
> but
>
>> use the new style structures.
>>
>
>  I still need to have a closer look at LoadKernel2() and LoadFirmware2()
>> but go
>> ahead and check this in after taking into account my comments below if my
>>
> final
>
>> LGTM is blocking further progress.
>>
>
>  Some comments follow.
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10007
>> File host/lib/host_key.c (right):
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10007#newcode83
>> host/lib/host_key.c:83: /* Allocate a new public key with space for a
>>
> [key_size]
>
>> byte key. */
>> nit: there's a possibility of confusion with what we mean by [key_size]
>> byte
>> key. In this case, we are referring to the size of stored key on  disk.
>> But
>>
> this
>
>> might be also be interpreted to mean the strength of the key. I had to
>> check
>>
> the
>
>> source to make sure I was interpreting it right.
>>
>
>  I noticed this elsewhere too. It would be useful to clarify that we mean
>> the
>> storage size of the key.
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10007#newcode102
>> host/lib/host_key.c:102: if (dest->key_size < src->key_size)
>> shouldn't we always return an error if dest key_size and src key_size are
>> different?
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10020
>> File vboot_firmware/include/load_kernel_fw.h (right):
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10020#newcode23
>> vboot_firmware/include/load_kernel_fw.h:23: #define BOOT_FLAG_DEVELOPER
>>
> 0x01ULL
>
>> /* Developer switch is on */
>> unsigned long long is not guaranteed to be uint64_t on all platform. would
>>
> using
>
>> the UINT64_C() macro be more portable here?
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10024
>> File vboot_firmware/lib/include/vboot_firmware.h (right):
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10024#newcode23
>> vboot_firmware/lib/include/vboot_firmware.h:23:
>> nit: extra new line
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10025
>> File vboot_firmware/lib/include/vboot_kernel.h (right):
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10025#newcode16
>> vboot_firmware/lib/include/vboot_kernel.h:16: #include "vboot_common.h"
>> nit: alphas order for includes
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10025#newcode35
>> vboot_firmware/lib/include/vboot_kernel.h:35:
>> nit: extra new line
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10026
>> File vboot_firmware/lib/include/vboot_struct.h (right):
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10026#newcode41
>> vboot_firmware/lib/include/vboot_struct.h:41: #define
>>
> KEY_BLOCK_FLAG_DEVELOPER_0
>
>>  0x01ULL  /* Developer switch off */
>> UINT64_C(0x01) instead?
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10028
>> File vboot_firmware/lib/vboot_common.c (right):
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10028#newcode27
>> vboot_firmware/lib/vboot_common.c:27: uint64_t OffsetOf(const void *base,
>>
> const
>
>> void *ptr) {
>> since this function assumes 64-bit pointers, would it be useful to have a
>> preprocessor check to ensure we are not compiling this code on a 32-bit
>>
> system?
>
>  http://codereview.chromium.org/2745007/diff/9001/10029
>> File vboot_firmware/lib/vboot_firmware.c (right):
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10029#newcode60
>> vboot_firmware/lib/vboot_firmware.c:60: if (index) {
>> nit/suggestion: since this is not a boolean check, it would be more
>> clearer if
>> the check was if (0 == index) with the corresponding if-else blocks
>> interchanged.
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10030
>> File vboot_firmware/lib/vboot_kernel.c (right):
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10030#newcode1
>> vboot_firmware/lib/vboot_kernel.c:1: /* Copyright (c) 2010 The Chromium OS
>> Authors. All rights reserved.
>> TODO REview
>>
>
>  http://codereview.chromium.org/2745007/diff/9001/10030#newcode14
>> vboot_firmware/lib/vboot_kernel.c:14: #include "vboot_kernel.h"
>> nit: this include should go first.
>>
>
>
>
> http://codereview.chromium.org/2745007/show
>

[vb, 2010-06-10 17:37:48]

I just had a quick look at the common structs, a few comments/suggestions FYI.

http://codereview.chromium.org/2745007/diff/9001/10026
File vboot_firmware/lib/include/vboot_struct.h (right):

http://codereview.chromium.org/2745007/diff/9001/10026#newcode28
vboot_firmware/lib/include/vboot_struct.h:28: uint64_t sig_size;    /* Size of
signature data from start of this struct */
Shouldn't this be just 'size of signature data'?

Also, why do you need to use 8 byte fields? It might be required for offsets
(albeit I don't see files larger than 2^32 in size practical), but all other
fields could be 4 bytes and some of them - 2? Or is this to not worry about
packing?

Generally, structures shared between compilers, are better to be explicitly
packed and accompanied by some compile time checks verifying the layout.

http://codereview.chromium.org/2745007/diff/9001/10026#newcode34
vboot_firmware/lib/include/vboot_struct.h:34: #define KEY_BLOCK_MAGIC_SIZE 8
would be more robust defined as (sizeof(KEY_BLOCK_MAGIC) - 1)