Change VerifyFirmware() to take separate pointers to firmware verification header and firmware data.

Change VerifyFirmware() to take separate pointers to firmware verification header and firmware data.

The firmware verification code no longer assumes that verification data and firmware data are contiguous and follow each other. Needed for EFI where the actual firmware must be stored in its own firmware volume.

BUG=1704
TEST=modified existing tests for the new API, and they still pass

[Chris Masone, 2010-04-18 17:33:41]

http://codereview.chromium.org/1578035/diff/1/8
File src/platform/vboot_reference/tests/test_common.h (right):

http://codereview.chromium.org/1578035/diff/1/8#newcode58
src/platform/vboot_reference/tests/test_common.h:58: * assume to be 1 bytes, and
containing { 'F' }.
s/assume/assumed/

also, is the assumption that -- if it does not satisfy either of those criteria,
does that mean that it's corrupt?

http://codereview.chromium.org/1578035/diff/1/9
File src/platform/vboot_reference/tests/verify_firmware_fuzz_driver.c (right):

http://codereview.chromium.org/1578035/diff/1/9#newcode29
src/platform/vboot_reference/tests/verify_firmware_fuzz_driver.c:29:
fprintf(stderr, "Couldn't read firmware image.\n");
none of these error msgs say anything about why the reads failed...putting that
info in might be nice, no?

http://codereview.chromium.org/1578035/diff/1/9#newcode40
src/platform/vboot_reference/tests/verify_firmware_fuzz_driver.c:40:
firmware_blob))) {
I'm not sure it matters, but it's odd that VerifySignedFirmware and
VerifyFirmware take args that represent the same stuff (one is file names, one
is file contents) but in a different order.  I feel like, if I was using these
utils, I'd always have to look up which one takes with args in which order.

http://codereview.chromium.org/1578035/diff/1/10
File src/platform/vboot_reference/vfirmware/firmware_image_fw.c (right):

http://codereview.chromium.org/1578035/diff/1/10#newcode122
src/platform/vboot_reference/vfirmware/firmware_image_fw.c:122: int preamble_len
= (FIELD_LEN(firmware_version) +
you can just refer directly to these fields, which are defined inside the
FirmwareImage struct?

http://codereview.chromium.org/1578035/diff/1/11
File src/platform/vboot_reference/vfirmware/include/firmware_image_fw.h (right):

http://codereview.chromium.org/1578035/diff/1/11#newcode106
src/platform/vboot_reference/vfirmware/include/firmware_image_fw.h:106: * key
[root_key] and verification header [verification_header_blob]..
extra .

http://codereview.chromium.org/1578035/diff/1/11#newcode138
src/platform/vboot_reference/vfirmware/include/firmware_image_fw.h:138: o */
um...o?  What?

[gauravsh, 2010-04-18 21:57:52]

http://codereview.chromium.org/1578035/diff/1/8
File src/platform/vboot_reference/tests/test_common.h (right):

http://codereview.chromium.org/1578035/diff/1/8#newcode58
src/platform/vboot_reference/tests/test_common.h:58: * assume to be 1 bytes, and
containing { 'F' }.
On 2010/04/18 17:33:41, cmasone wrote:
> s/assume/assumed/
> 
> also, is the assumption that -- if it does not satisfy either of those
criteria,
> does that mean that it's corrupt?

Done.

The verification block is only valid for a firmware with a single byte of 'F'.
Anything else would an invalidate the verification block, and yes, corrupt (from
the test's perspective).

http://codereview.chromium.org/1578035/diff/1/9
File src/platform/vboot_reference/tests/verify_firmware_fuzz_driver.c (right):

http://codereview.chromium.org/1578035/diff/1/9#newcode29
src/platform/vboot_reference/tests/verify_firmware_fuzz_driver.c:29:
fprintf(stderr, "Couldn't read firmware image.\n");
On 2010/04/18 17:33:41, cmasone wrote:
> none of these error msgs say anything about why the reads failed...putting
that
> info in might be nice, no?

BufferFromFile() doesn't return the type of error that occurred to the caller.
However, it will spew out error messages about the problem. I can make that more
verbose, but I'd like to leave BufferFromFile() as it is.

http://codereview.chromium.org/1578035/diff/1/9#newcode40
src/platform/vboot_reference/tests/verify_firmware_fuzz_driver.c:40:
firmware_blob))) {
On 2010/04/18 17:33:41, cmasone wrote:
> I'm not sure it matters, but it's odd that VerifySignedFirmware and
> VerifyFirmware take args that represent the same stuff (one is file names, one
> is file contents) but in a different order.  I feel like, if I was using these
> utils, I'd always have to look up which one takes with args in which order. 

Good point. That irks me too. Fixed.

http://codereview.chromium.org/1578035/diff/1/10
File src/platform/vboot_reference/vfirmware/firmware_image_fw.c (right):

http://codereview.chromium.org/1578035/diff/1/10#newcode122
src/platform/vboot_reference/vfirmware/firmware_image_fw.c:122: int preamble_len
= (FIELD_LEN(firmware_version) +
On 2010/04/18 17:33:41, cmasone wrote:
> you can just refer directly to these fields, which are defined inside the
> FirmwareImage struct?
 Yes, through the magic of macros. Look at the definition of the FIELD_LEN macro
at the beginning of the file. 

Since I am only interested in the size information which is static and can be
determined at compile time,  this helps me not having to hardcode the field
lengths and not requiring an existing structure variable for calling sizeof() on
the fields.

http://codereview.chromium.org/1578035/diff/1/11
File src/platform/vboot_reference/vfirmware/include/firmware_image_fw.h (right):

http://codereview.chromium.org/1578035/diff/1/11#newcode106
src/platform/vboot_reference/vfirmware/include/firmware_image_fw.h:106: * key
[root_key] and verification header [verification_header_blob]..
On 2010/04/18 17:33:41, cmasone wrote:
> extra .
done.

http://codereview.chromium.org/1578035/diff/1/11#newcode138
src/platform/vboot_reference/vfirmware/include/firmware_image_fw.h:138: o */
On 2010/04/18 17:33:41, cmasone wrote:
> um...o?  What?

oooo! Fixed.

[Chris Masone, 2010-04-18 22:11:48]

LGTM

On 2010/04/18 21:57:52, gauravsh wrote:
> http://codereview.chromium.org/1578035/diff/1/8
> File src/platform/vboot_reference/tests/test_common.h (right):
> 
> http://codereview.chromium.org/1578035/diff/1/8#newcode58
> src/platform/vboot_reference/tests/test_common.h:58: * assume to be 1 bytes,
and
> containing { 'F' }.
> On 2010/04/18 17:33:41, cmasone wrote:
> > s/assume/assumed/
> > 
> > also, is the assumption that -- if it does not satisfy either of those
> criteria,
> > does that mean that it's corrupt?
> 
> Done.
> 
> The verification block is only valid for a firmware with a single byte of 'F'.
> Anything else would an invalidate the verification block, and yes, corrupt
(from
> the test's perspective).
> 
> http://codereview.chromium.org/1578035/diff/1/9
> File src/platform/vboot_reference/tests/verify_firmware_fuzz_driver.c (right):
> 
> http://codereview.chromium.org/1578035/diff/1/9#newcode29
> src/platform/vboot_reference/tests/verify_firmware_fuzz_driver.c:29:
> fprintf(stderr, "Couldn't read firmware image.\n");
> On 2010/04/18 17:33:41, cmasone wrote:
> > none of these error msgs say anything about why the reads failed...putting
> that
> > info in might be nice, no?
> 
> BufferFromFile() doesn't return the type of error that occurred to the caller.
> However, it will spew out error messages about the problem. I can make that
more
> verbose, but I'd like to leave BufferFromFile() as it is.
> 
> http://codereview.chromium.org/1578035/diff/1/9#newcode40
> src/platform/vboot_reference/tests/verify_firmware_fuzz_driver.c:40:
> firmware_blob))) {
> On 2010/04/18 17:33:41, cmasone wrote:
> > I'm not sure it matters, but it's odd that VerifySignedFirmware and
> > VerifyFirmware take args that represent the same stuff (one is file names,
one
> > is file contents) but in a different order.  I feel like, if I was using
these
> > utils, I'd always have to look up which one takes with args in which order. 
> 
> Good point. That irks me too. Fixed.
> 
> http://codereview.chromium.org/1578035/diff/1/10
> File src/platform/vboot_reference/vfirmware/firmware_image_fw.c (right):
> 
> http://codereview.chromium.org/1578035/diff/1/10#newcode122
> src/platform/vboot_reference/vfirmware/firmware_image_fw.c:122: int
preamble_len
> = (FIELD_LEN(firmware_version) +
> On 2010/04/18 17:33:41, cmasone wrote:
> > you can just refer directly to these fields, which are defined inside the
> > FirmwareImage struct?
>  Yes, through the magic of macros. Look at the definition of the FIELD_LEN
macro
> at the beginning of the file. 
> 
> Since I am only interested in the size information which is static and can be
> determined at compile time,  this helps me not having to hardcode the field
> lengths and not requiring an existing structure variable for calling sizeof()
on
> the fields.
> 
> http://codereview.chromium.org/1578035/diff/1/11
> File src/platform/vboot_reference/vfirmware/include/firmware_image_fw.h
(right):
> 
> http://codereview.chromium.org/1578035/diff/1/11#newcode106
> src/platform/vboot_reference/vfirmware/include/firmware_image_fw.h:106: * key
> [root_key] and verification header [verification_header_blob]..
> On 2010/04/18 17:33:41, cmasone wrote:
> > extra .
> done.
> 
> http://codereview.chromium.org/1578035/diff/1/11#newcode138
> src/platform/vboot_reference/vfirmware/include/firmware_image_fw.h:138: o */
> On 2010/04/18 17:33:41, cmasone wrote:
> > um...o?  What?
> 
> oooo! Fixed.