Modifying the kernel_utility tool to create our magic blob.

Modifying the kernel_utility tool to create our magic blob.

For the --generate operation, the --in <file> option is gone and there are
three new required options:

  --vmlinuz <file>         Embedded kernel image
  --config <file>          Embedded kernel command-line parameters
  --bootloader <file>      Embedded bootloader stub

This takes the specified kernel, extracts the 32-bit component, and combines
that with the configuration file (essentially just the kernel cmdline
string) and the bootstub image . The resulting blob is signed and ready to
put in a kernel partition.

There's also an optional --padding parameter, to specify how much extra
(unsigned) space to leave between the signature header and the kernel blob.
The default is 0x4000, which is about four times as much needed when using
the largest signature size we currently support.

[gauravsh, 2010-05-27 10:05:38]

http://codereview.chromium.org/2283005/diff/1/5
File src/platform/vboot_reference/vboot_firmware/include/utility.h (right):

http://codereview.chromium.org/2283005/diff/1/5#newcode90
src/platform/vboot_reference/vboot_firmware/include/utility.h:90: * On success,
return a meaningless but non-NULL pointer and update [state].
updates [state]

http://codereview.chromium.org/2283005/diff/1/7
File src/platform/vboot_reference/vkernel/include/kernel_blob.h (right):

http://codereview.chromium.org/2283005/diff/1/7#newcode50
src/platform/vboot_reference/vkernel/include/kernel_blob.h:50: #endif //
VBOOT_REFERENCE_KERNEL_BLOB_H_
nit: 2 spaces between #endif and start of comment.

http://codereview.chromium.org/2283005/diff/1/9
File src/platform/vboot_reference/vkernel/kernel_image.c (right):

http://codereview.chromium.org/2283005/diff/1/9#newcode10
src/platform/vboot_reference/vkernel/kernel_image.c:10: #include "kernel_blob.h"
nit: this #include goes in the section below standard C includes (#include
"cryptolib.h", etc. below)

http://codereview.chromium.org/2283005/diff/1/9#newcode160
src/platform/vboot_reference/vkernel/kernel_image.c:160: /* Read config and
kernel signatures. */
s/config/preamble/

http://codereview.chromium.org/2283005/diff/1/9#newcode164
src/platform/vboot_reference/vkernel/kernel_image.c:164: StatefulMemcpy(&st,
image->kernel_signature, kernel_signature_len);
these 2 calls to StatefulMemcpy() should be reversed - since now the
kernel_signature is part of the preamble (sorry for missing this earlier)

http://codereview.chromium.org/2283005/diff/1/9#newcode588
src/platform/vboot_reference/vkernel/kernel_image.c:588: static uint64_t
roundup(uint64_t val, uint64_t alignment) {
function comment? a better name maybe?

http://codereview.chromium.org/2283005/diff/1/9#newcode620
src/platform/vboot_reference/vkernel/kernel_image.c:620: uint64_t*
bootstub_size) {
are the above 2 the same fields that are part of struct KernelImage
(bootloader_offset and bootloader_size). If so, please use the same name to
avoid potential confusion about them meaning different things.

http://codereview.chromium.org/2283005/diff/1/9#newcode621
src/platform/vboot_reference/vkernel/kernel_image.c:621: uint8_t *kernel_buf,
*config_buf, *bootloader_buf, *blob = 0;
nit: please follow the * alignment convention in this file (* attaches to the
type, not the var name).

http://codereview.chromium.org/2283005/diff/1/9#newcode634
src/platform/vboot_reference/vkernel/kernel_image.c:634: goto done0;
So, goto-s is not allowed in Google code, and I am sure some other reviewer will
frown upon this usage given its use of multiple labels. I know there are gotos
peppered here and there in the vboot_reference code but I am trying to slowly
refactor them out.

At the very least,  avoid using goto for userland implementations (in
firmware-land, goto might be efficient and prudent choice). 

If it's too much pain, you don't have to refactor the gotos with this CL but at
least add a TODO.

http://codereview.chromium.org/2283005/diff/1/9#newcode639
src/platform/vboot_reference/vkernel/kernel_image.c:639: assert(config_size <
CROS_CONFIG_SIZE); // need room for trailing '\0'
is assert() really necessary here? shouldn't the error be returned to the
caller?

http://codereview.chromium.org/2283005/diff/1/9#newcode659
src/platform/vboot_reference/vkernel/kernel_image.c:659: CROS_CONFIG_SIZE +
I can't check for sure if this is entirely bug free. But you might want to add a
test with a known good kernel image to make sure this works as intended.

http://codereview.chromium.org/2283005/diff/1/9#newcode665
src/platform/vboot_reference/vkernel/kernel_image.c:665: memset(blob, 0,
blob_size);
Memset

http://codereview.chromium.org/2283005/diff/1/9#newcode670
src/platform/vboot_reference/vkernel/kernel_image.c:670: memcpy(blob + now,
kernel_buf + kernel32_start, kernel32_size);
Memcpy

http://codereview.chromium.org/2283005/diff/1/9#newcode679
src/platform/vboot_reference/vkernel/kernel_image.c:679: memcpy(blob + now,
config_buf, config_size);
Memcpy

http://codereview.chromium.org/2283005/diff/1/9#newcode687
src/platform/vboot_reference/vkernel/kernel_image.c:687:
memcpy(&(params->setup_sects), &(lh->setup_sects),
Memcpy (In general, prefer the wrapper versions instead of the libc version for
all invocations in this file) - this is mostly for consistency and also makes
porting to the firmware easier

[Bill Richardson, 2010-05-27 17:55:46]

Try this.

http://codereview.chromium.org/2283005/diff/1/9
File src/platform/vboot_reference/vkernel/kernel_image.c (right):

http://codereview.chromium.org/2283005/diff/1/9#newcode164
src/platform/vboot_reference/vkernel/kernel_image.c:164: StatefulMemcpy(&st,
image->kernel_signature, kernel_signature_len);
On 2010/05/27 10:05:38, gauravsh wrote:
> these 2 calls to StatefulMemcpy() should be reversed - since now the
> kernel_signature is part of the preamble (sorry for missing this earlier)

Whoops, you're right! tests/run_image_verification_tests.sh passed anyway. Is
there another test that I'm missing? Can you update tests/Makefile with a target
to regenerate and rerun all possible tests?

http://codereview.chromium.org/2283005/diff/1/9#newcode588
src/platform/vboot_reference/vkernel/kernel_image.c:588: static uint64_t
roundup(uint64_t val, uint64_t alignment) {
On 2010/05/27 10:05:38, gauravsh wrote:
> function comment? a better name maybe? 

ok

http://codereview.chromium.org/2283005/diff/1/9#newcode634
src/platform/vboot_reference/vkernel/kernel_image.c:634: goto done0;
The Google standard is only for C++, not C.
http://wiki.corp.google.com/twiki/bin/view/Main/GdhProgrammingStyleGuides says
"For C, linux kernel and other open source packages, use whichever coding
standard applies to the package you are contributing to." The linux kernel style
guide explictly suggests this use of goto for cleanup.
http://lxr.linux.no/linux/Documentation/CodingStyle (Chapter 7).

http://codereview.chromium.org/2283005/diff/1/9#newcode639
src/platform/vboot_reference/vkernel/kernel_image.c:639: assert(config_size <
CROS_CONFIG_SIZE); // need room for trailing '\0'
On 2010/05/27 10:05:38, gauravsh wrote:
> is assert() really necessary here? shouldn't the error be returned to the
> caller? 

Okay. We don't actually have any way to convey the reason why something didn't
work - we just return NULL and let things fall apart in the caller. I figured
the assert() would be quicker to debug when/if someone eventually provides too
large a config file.

http://codereview.chromium.org/2283005/diff/1/9#newcode659
src/platform/vboot_reference/vkernel/kernel_image.c:659: CROS_CONFIG_SIZE +
On 2010/05/27 10:05:38, gauravsh wrote:
> I can't check for sure if this is entirely bug free. But you might want to add
a
> test with a known good kernel image to make sure this works as intended.

Okay.

http://codereview.chromium.org/2283005/diff/1/9#newcode687
src/platform/vboot_reference/vkernel/kernel_image.c:687:
memcpy(&(params->setup_sects), &(lh->setup_sects),
On 2010/05/27 10:05:38, gauravsh wrote:
> Memcpy (In general, prefer the wrapper versions instead of the libc version
for
> all invocations in this file) - this is mostly for consistency and also makes
> porting to the firmware easier

This was just an oversight. Would it be useful to put something in our headers
so that the non-wrapper versions cause a compile-time error?

[gauravsh, 2010-05-27 18:06:04]

lgtm

http://codereview.chromium.org/2283005/diff/1/9
File src/platform/vboot_reference/vkernel/kernel_image.c (right):

http://codereview.chromium.org/2283005/diff/1/9#newcode164
src/platform/vboot_reference/vkernel/kernel_image.c:164: StatefulMemcpy(&st,
image->kernel_signature, kernel_signature_len);
On 2010/05/27 17:55:47, Bill Richardson wrote:
> On 2010/05/27 10:05:38, gauravsh wrote:
> > these 2 calls to StatefulMemcpy() should be reversed - since now the
> > kernel_signature is part of the preamble (sorry for missing this earlier)
> 
> Whoops, you're right! tests/run_image_verification_tests.sh passed anyway. Is
> there another test that I'm missing? Can you update tests/Makefile with a
target
> to regenerate and rerun all possible tests?
> 
> 

I think this function is not tested as part of the image verification tests.
It's only used by the kernel signing utility (so a generated signed image would
have most likely failed verification).

http://codereview.chromium.org/2283005/diff/1/9#newcode634
src/platform/vboot_reference/vkernel/kernel_image.c:634: goto done0;
On 2010/05/27 17:55:47, Bill Richardson wrote:
> The Google standard is only for C++, not C.
> http://wiki.corp.google.com/twiki/bin/view/Main/GdhProgrammingStyleGuides says
> "For C, linux kernel and other open source packages, use whichever coding
> standard applies to the package you are contributing to." The linux kernel
style
> guide explictly suggests this use of goto for cleanup.
> http://lxr.linux.no/linux/Documentation/CodingStyle (Chapter 7).
> 

Ok, although we don't really follow linux kernel style for this file. I am fine
with the gotos. They are much cleaner for cleanup.

http://codereview.chromium.org/2283005/diff/1/9#newcode687
src/platform/vboot_reference/vkernel/kernel_image.c:687:
memcpy(&(params->setup_sects), &(lh->setup_sects),
On 2010/05/27 17:55:47, Bill Richardson wrote:
> On 2010/05/27 10:05:38, gauravsh wrote:
> > Memcpy (In general, prefer the wrapper versions instead of the libc version
> for
> > all invocations in this file) - this is mostly for consistency and also
makes
> > porting to the firmware easier
> 
> This was just an oversight. Would it be useful to put something in our headers
> so that the non-wrapper versions cause a compile-time error?
> 

Sure. I don't remember the necessary magic to do this off the top of my head.
(will #undef-ing the symbols work?)

http://codereview.chromium.org/2283005/diff/7001/8008#newcode18
src/platform/vboot_reference/vkernel/kernel_image.c:18: 
oops, I meant only #include kernel_blob.h should go here. #inclde kernel_image.h
(header corres. to this .c file) should come first.

[Randall Spangler, 2010-05-27 20:30:16]

lgtm