Update PCR state with the state of the firmware keyblock flags too.

Update PCR state with the state of the firmware keyblock flags too.

I re-factored the extend call into its own module, since it doesn't necessarily
need to be couple with rollbackfirmwaresetup.

BUG=chrome-os-partner:2372
TEST=Tried emerge-x86-generic and emerge-arm-generic vboot_reference.

Change-Id: I0d3115b10b686133e63e304570325ebdbd50ae3a

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=b64faaa

[gauravsh, 2011-03-14 23:53:00]

One thing I was not sure of what to do if the firmware keyblock mode is not
valid. Looking at the flags, they should be either 1 or 2 (dev mode off or dev
mode off), so I use a value of 0 as being invalid in a non-recovery context
while updating the TPM.

In the recovery context (LoadKernel() in recovery), I treat the keyblock mode as
0.

So 0 as the keyblock mode is used to denote both an error (in non-dev mode) or
don't-care (in recovery) while making the extend call. Not sure if that really
suits our purpose.

[Randall Spangler, 2011-03-15 05:18:18]

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/include/tpm_boo...
File firmware/lib/include/tpm_bootmode.h (right):

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/include/tpm_boo...
firmware/lib/include/tpm_bootmode.h:18: */
Returns?

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/tpm_bootmode.c
File firmware/lib/tpm_bootmode.c (right):

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/tpm_bootmode.c#...
firmware/lib/tpm_bootmode.c:21: * Keyblock_Mode could be 0 (invalid or don't
care), 1 (developer switch
Key block flags are defined in vboot_struct.h:

#define KEY_BLOCK_FLAG_DEVELOPER_0  UINT64_C(0x01)  /* Developer switch off */
#define KEY_BLOCK_FLAG_DEVELOPER_1  UINT64_C(0x02)  /* Developer switch on */
#define KEY_BLOCK_FLAG_RECOVERY_0   UINT64_C(0x04)  /* Not recovery mode */
#define KEY_BLOCK_FLAG_RECOVERY_1   UINT64_C(0x08)  /* Recovery mode */

Normal firmware is signed with flags=7 (recovery off, don't care about dev)
Dev firmware is signed with flags=6 (recovery off, dev on)

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/tpm_bootmode.c#...
firmware/lib/tpm_bootmode.c:77: ( ((dev_mode) * 6) + ((rec_mode) * 3) +
(keyblock_mode) )
This runs off the end of your array for keyblock_mode=6 or 7.

I think you need to turn this into a function, so you have space to do:

switch(keyblock_mode):
  case 7: // dev0 | dev1 | rec0, aka normal FW
    index = 1;
    break;
  case 6: // dev1 | rec0, aka dev FW
    index = 2;
    break;
  default:
    index = 0;
}

if(rec_mode)
  index += 3;

if (dev_mode)
  index += 6;

That's safer for dev_mode and rec_mode, too.

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/tpm_bootmode.c#...
firmware/lib/tpm_bootmode.c:83: int digest_index =
GET_BOOTSTATE_INDEX(developer_mode, recovery_mode,
Should range-check digest_index to make sure it's valid.

[gauravsh, 2011-03-15 06:29:10]

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/tpm_bootmode.c
File firmware/lib/tpm_bootmode.c (right):

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/tpm_bootmode.c#...
firmware/lib/tpm_bootmode.c:21: * Keyblock_Mode could be 0 (invalid or don't
care), 1 (developer switch
On 2011/03/15 05:18:18, Randall Spangler wrote:
> Key block flags are defined in vboot_struct.h:
> 
> #define KEY_BLOCK_FLAG_DEVELOPER_0  UINT64_C(0x01)  /* Developer switch off */
> #define KEY_BLOCK_FLAG_DEVELOPER_1  UINT64_C(0x02)  /* Developer switch on */
> #define KEY_BLOCK_FLAG_RECOVERY_0   UINT64_C(0x04)  /* Not recovery mode */
> #define KEY_BLOCK_FLAG_RECOVERY_1   UINT64_C(0x08)  /* Recovery mode */
> 
> Normal firmware is signed with flags=7 (recovery off, don't care about dev)
> Dev firmware is signed with flags=6 (recovery off, dev on)

Ahh ok, I misread this to mean that in the normal case (non-recovery) the flags
would be 0 or 1 depending on the position of the dev mode switch.

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/tpm_bootmode.c#...
firmware/lib/tpm_bootmode.c:77: ( ((dev_mode) * 6) + ((rec_mode) * 3) +
(keyblock_mode) )
On 2011/03/15 05:18:18, Randall Spangler wrote:
> This runs off the end of your array for keyblock_mode=6 or 7.
> 
> I think you need to turn this into a function, so you have space to do:
> 
> switch(keyblock_mode):
>   case 7: // dev0 | dev1 | rec0, aka normal FW
>     index = 1;
>     break;
>   case 6: // dev1 | rec0, aka dev FW
>     index = 2;
>     break;
>   default:
>     index = 0;
> }
> 
> if(rec_mode)
>   index += 3;
> 
> if (dev_mode)
>   index += 6;
> 
> That's safer for dev_mode and rec_mode, too.

Yeah, no point optimizing - I was relying on the (wrong) fact that the passed in
keyblock_mode will always be 0, 1 or 2 (in recovery, it is set to 0 to mean
no-op). I will have a CL to fix this tomorrow.

[gauravsh, 2011-03-15 23:22:49]

PTAL.

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/include/tpm_boo...
File firmware/lib/include/tpm_bootmode.h (right):

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/include/tpm_boo...
firmware/lib/include/tpm_bootmode.h:18: */
On 2011/03/15 05:18:18, Randall Spangler wrote:
> Returns?
Done.

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/tpm_bootmode.c
File firmware/lib/tpm_bootmode.c (right):

http://codereview.chromium.org/6696006/diff/2001/firmware/lib/tpm_bootmode.c#...
firmware/lib/tpm_bootmode.c:83: int digest_index =
GET_BOOTSTATE_INDEX(developer_mode, recovery_mode,
On 2011/03/15 05:18:18, Randall Spangler wrote:
> Should range-check digest_index to make sure it's valid.

I wasn't sure what to return in case the range check fails (that would qualify
as an internal error), and didn't want to further complicate the function by
returning 2 values (overall success + TPM error code, if any) - so I have chosen
to update the PCR with an invalid state digest instead.

[Randall Spangler, 2011-03-15 23:26:11]

LGTM, thanks

http://codereview.chromium.org/6696006/diff/5002/firmware/lib/tpm_bootmode.c
File firmware/lib/tpm_bootmode.c (right):

http://codereview.chromium.org/6696006/diff/5002/firmware/lib/tpm_bootmode.c#...
firmware/lib/tpm_bootmode.c:83: /* Used for PCR extend when passed in boot state
if invalid or
/if/is