New tools to help debug vboot failures.

New tools to help debug vboot failures.

This adds some tools to help us figure out why a particular kernel isn't
booting. Often we suspect it's because it was signed with the wrong keys, or
has flags restricting its use to certain boot modes. This change adds some
tools to extract and display all the keys from the BIOS, and try them on the
various kernels. We also display the sha1sum of all the keys we find, to
make comparing them easier.

Change-Id: I38e447bf95cb6c3a0b87aa949611bb135f2f94b4

BUG=chromeos-partner:888
TEST=manual

To test, obtain a root shell, and run dev_debug_vboot. You should see lots
of useful information go by.

[Randall Spangler, 2010-09-09 00:02:13]

http://codereview.chromium.org/3303018/diff/1/3
File utility/dev_debug_vboot (right):

http://codereview.chromium.org/3303018/diff/1/3#newcode20
utility/dev_debug_vboot:20: dump_fmap -x ${BIOS} || echo "FAILED"
Some extra echo's here to explain what's being dumped/compared would be really
handy.  i.e.

echo "Reading keys from GBB"
echo "Verifying firmware A using root key"
echo "Verifying kernel A on SSD using key from firmware A"

http://codereview.chromium.org/3303018/diff/1/3#newcode28
utility/dev_debug_vboot:28: --fv Firmware_B_Data --kernelkey
kernel_subkey.vbpubk || echo "FAILED"
We only check the kernel below with the kernel subkey from firmware B. 
Shouldn't we check with the kernel subkeys from both A and B, if they differ?

http://codereview.chromium.org/3303018/diff/1/4
File utility/dump_fmap.c (right):

http://codereview.chromium.org/3303018/diff/1/4#newcode23
utility/dump_fmap.c:23: /* FMAP structs */
Add comment that these come from http://code.google.com/p/flashmap/wiki/FmapSpec

(and use the actual C struct from that spec?)

http://codereview.chromium.org/3303018/diff/1/4#newcode66
utility/dump_fmap.c:66: if (opt_x) {
opt_x --> opt_extract  Had to look below to decipher its meaning.

http://codereview.chromium.org/3303018/diff/1/4#newcode71
utility/dump_fmap.c:71: FILE *fp = fopen(buf,"w");
"wb"

http://codereview.chromium.org/3303018/diff/1/5
File utility/vbutil_firmware.c (right):

http://codereview.chromium.org/3303018/diff/1/5#newcode163
utility/vbutil_firmware.c:163: static void PrintDigest(const uint8_t* buf,
uint64_t buflen) {
This appears in at least 3 places.  Move to host/lib/host_misc.c (or similar).

[gauravsh, 2010-09-09 00:26:46]

I am curious, why not just directly use fmap_decode (part of the dhendrix's
flashmap package) and parse its output? Why is dump_fmap.c needed?

[Bill Richardson, 2010-09-09 00:33:44]

I need to extract the pieces. I didn't know about fmap_decode until I'd already
written most of this.

[gauravsh, 2010-09-09 00:43:34]

On Wed, Sep 8, 2010 at 5:33 PM,  <wfrichar@chromium.org> wrote:
> I need to extract the pieces. I didn't know about fmap_decode until I'd
> already
> written most of this.

Ok. Since the upstream code is already sufficient for our needs (from
what I can tell), I'd prefer if we just used that or if you feel
fmap_decode is not sufficient for your needs, get your code upstreamed
to 'flashmap' (David?).

The reason being that apart from maintenance issues, there are many
places where we need to parse the firmware layout like updates,
re-signing - currently we use a combination of hardcoded offsets and
sizes, use of fmap_decode (my signing code) and custom parsing code
such as yours. We should at least be consistent, choose one methods
and then stick to it everywhere.

Anyway, this is just my many cents. I don't have any violent
objections if you want to check this in as-is.

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



-- 
-g

[dhendrix, 2010-09-09 01:24:42]

http://codereview.chromium.org/3303018/diff/1/4
File utility/dump_fmap.c (right):

http://codereview.chromium.org/3303018/diff/1/4#newcode172
utility/dump_fmap.c:172: for (i=0; i<sb.st_size; i++) {
You can increment by 4, if you prefer. It's optional since you're only working
on files, but increasing the stride helps speed a lot for reading from EEPROMs
(it's 4 in the spec, but should probably be 64...).

[dhendrix1, 2010-09-09 01:25:34]

On Wed, Sep 8, 2010 at 5:43 PM, Gaurav Shah <gauravsh@chromium.org> wrote:

> On Wed, Sep 8, 2010 at 5:33 PM,  <wfrichar@chromium.org> wrote:
> > I need to extract the pieces. I didn't know about fmap_decode until I'd
> > already
> > written most of this.
>
> Ok. Since the upstream code is already sufficient for our needs (from
> what I can tell), I'd prefer if we just used that or if you feel
> fmap_decode is not sufficient for your needs, get your code upstreamed
> to 'flashmap' (David?).
>
> The reason being that apart from maintenance issues, there are many
> places where we need to parse the firmware layout like updates,
> re-signing - currently we use a combination of hardcoded offsets and
> sizes, use of fmap_decode (my signing code) and custom parsing code
> such as yours. We should at least be consistent, choose one methods
> and then stick to it everywhere.
>
> Anyway, this is just my many cents. I don't have any violent
> objections if you want to check this in as-is.
>

No violent objections here either. I know we're all in a crunch at the
moment, so perhaps filing a ticket to migrate later on might be the best
option so that Bill doesn't have to re-test everything with a foreign chunk
of code right now.

AFAICT Bill's code doesn't violate the spec in any way. There are some
differences in the output of the program, but they're pretty minor. I'd have
to examine how they're used to really know if we'll break anything by doing
the switchover. FWIW, if the desired output is in list format, perhaps
"mosys -l eeprom map <filename>" might provide most of what is needed here.

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.

[vb, 2010-09-09 13:54:26]

On 2010/09/09 00:43:34, gauravsh wrote:
> On Wed, Sep 8, 2010 at 5:33 PM,  <mailto:wfrichar@chromium.org> wrote:
> > I need to extract the pieces. I didn't know about fmap_decode until I'd
> > already
> > written most of this.
> 
> Ok. Since the upstream code is already sufficient for our needs (from
> what I can tell), I'd prefer if we just used that or if you feel
> fmap_decode is not sufficient for your needs, get your code upstreamed
> to 'flashmap' (David?).
>

Doesn't mosys does all what's needed, especially with the recent addition where
there is no need to specify the target type when working with files?

Dropping this file and using mosys would require some effort now but will save a
lot in the future.

 
> The reason being that apart from maintenance issues, there are many
> places where we need to parse the firmware layout like updates,
> re-signing - currently we use a combination of hardcoded offsets and
> sizes, use of fmap_decode (my signing code) and custom parsing code
> such as yours. We should at least be consistent, choose one methods
> and then stick to it everywhere.
> 
> Anyway, this is just my many cents. I don't have any violent
> objections if you want to check this in as-is.
> 
> >
> >
> > http://codereview.chromium.org/3303018/show
> >
> 
> 
> 
> -- 
> -g
>

[Bill Richardson, 2010-09-09 18:40:33]

New changes uploaded.

I'm going to keep the dump_fmap utility here for now. We can file a new bug to
roll it into the mosys stuff later. AFAIK, mosys doesn't extract the components
from the FMAP, which dump_fmap does. I need the pieces pulled out so I can use
them. Printing the size and offsets is secondary to that.

http://codereview.chromium.org/3303018/diff/1/3
File utility/dev_debug_vboot (right):

http://codereview.chromium.org/3303018/diff/1/3#newcode20
utility/dev_debug_vboot:20: dump_fmap -x ${BIOS} || echo "FAILED"
Good idea.

http://codereview.chromium.org/3303018/diff/1/3#newcode28
utility/dev_debug_vboot:28: --fv Firmware_B_Data --kernelkey
kernel_subkey.vbpubk || echo "FAILED"
Oops. Yes, thanks.

http://codereview.chromium.org/3303018/diff/1/4
File utility/dump_fmap.c (right):

http://codereview.chromium.org/3303018/diff/1/4#newcode23
utility/dump_fmap.c:23: /* FMAP structs */
I didn't know about that. I got them from the BIOS' FMAP.ASM, but your reference
is better.

http://codereview.chromium.org/3303018/diff/1/4#newcode66
utility/dump_fmap.c:66: if (opt_x) {
done

http://codereview.chromium.org/3303018/diff/1/4#newcode71
utility/dump_fmap.c:71: FILE *fp = fopen(buf,"w");
Hmph. Done. "This is strictly for compatibility with C89 and has no effect; the
'b' is ignored on all POSIX conforming systems, including Linux."

http://codereview.chromium.org/3303018/diff/1/4#newcode172
utility/dump_fmap.c:172: for (i=0; i<sb.st_size; i++) {
Ah! I hadn't seen the spec before now. Thanks.

http://codereview.chromium.org/3303018/diff/1/5
File utility/vbutil_firmware.c (right):

http://codereview.chromium.org/3303018/diff/1/5#newcode163
utility/vbutil_firmware.c:163: static void PrintDigest(const uint8_t* buf,
uint64_t buflen) {
Done.

[Randall Spangler, 2010-09-09 21:48:39]

LGTM