Fix normal/recovery mode, and RO firmware vs. RW firmware behavior.

Fix normal/recovery mode, and RO firmware vs. RW firmware behavior.

BUG: chrome-os-partner: 1154
TEST: none

[Luigi Semenzato, 2010-06-10 23:39:28]

This change follows a clarification from Randall on how we enter recovery mode
and what information is available.  Essentially, we don't know why we enter
recovery mode.  SetupTPM is called in either mode.  We are hoping that if the
TPM has a hardware failure or is in an unexpected state, the same failure will
occur in recovery mode, and upon detecting it we will not set the bGlobalLock
bit.  This scares me a bit---there is a scenario in which we get a failure in
normal mode and we don't get it in recovery mode and bGlobalLock is set and we
can never fix the original problem---we're bricked.  I don't know if we can do
better though.

[gauravsh, 2010-06-11 08:57:24]

http://codereview.chromium.org/2792009/diff/3001/4004
File vboot_firmware/lib/rollback_index.c (right):

http://codereview.chromium.org/2792009/diff/3001/4004#newcode112
vboot_firmware/lib/rollback_index.c:112: case FIRMWARE_VERSIONS:
 Is the reason for this part of the change -  explicitly asking for the
firmware/kernel version - because it is possible that either one of these
operations might fail depending on what was and wasn't locked before we entered
recovery?

http://codereview.chromium.org/2792009/diff/3001/4004#newcode279
vboot_firmware/lib/rollback_index.c:279:
RETURN_ON_FAILURE(TlclRead(DEVELOPER_MODE_NV_INDEX,
where does this come from? I can't seem to find the definition/description of
DEVELOPER_MODE_NV_INDEX anywhere. 

Also, can you add a comment describing what this code path does - it's not clear
just by looking at the code what the high level rationale is.

[Luigi Semenzato, 2010-06-11 13:49:43]

Sorry---I meant to temporarily withdraw the review request.  I am in the
middle of adding a new feature, clearing the TPM on transitions from normal
to developer mode.  We are also moving, so I updated the CL before shutting
down the machine just in case.

Dictated on a Nexus One.

On Jun 11, 2010 1:57 AM, <gauravsh@chromium.org> wrote:
>
> http://codereview.chromium.org/2792009/diff/3001/4004
> File vboot_firmware/lib/rollback_index.c (right):
>
> http://codereview.chromium.org/2792009/diff/3001/4004#newcode112
> vboot_firmware/lib/rollback_index.c:112: case FIRMWARE_VERSIONS:
> Is the reason for this part of the change - explicitly asking for the
> firmware/kernel version - because it is possible that either one of
> these operations might fail depending on what was and wasn't locked
> before we entered recovery?
>
> http://codereview.chromium.org/2792009/diff/3001/4004#newcode279
> vboot_firmware/lib/rollback_index.c:279:
> RETURN_ON_FAILURE(TlclRead(DEVELOPER_MODE_NV_INDEX,
> where does this come from? I can't seem to find the
> definition/description of DEVELOPER_MODE_NV_INDEX anywhere.
>
> Also, can you add a comment describing what this code path does - it's
> not clear just by looking at the code what the high level rationale is.
>
> http://codereview.chromium.org/2792009/show

[Luigi Semenzato, 2010-06-12 00:45:19]

In addition to the changes described at the beginning of this thread, I have
added code that detects transitions between developer mode and normal mode, and
clears the TPM on transitions into developer mode (but not into normal mode).

Actually, shouldn't we clear the TPM on the opposite transition too?  Otherwise
someone might return to the store a machine with a TPM owned by him, no?

Anyhow: the SetupTPM API changes in this CL, and I fixed the callers so that the
code compiles, but nothing else.  These are the changes:

1. There are two parameters, MODE and DEVELOPER_FLAG.  They must be passed
accordingly to the boot mode.  (I know---not all combinations are meaningful.)

2. If SetupTPM returns TPM_SUCCESS, the caller may proceed.  If it returns
TPM_MUST_REBOOT, the caller must reboot in normal mode.  For all other values,
it must reboot in recovery mode.

[Randall Spangler, 2010-06-12 16:19:35]

On Fri, Jun 11, 2010 at 5:45 PM, <semenzato@chromium.org> wrote:

> In addition to the changes described at the beginning of this thread, I
> have
> added code that detects transitions between developer mode and normal mode,
> and
> clears the TPM on transitions into developer mode (but not into normal
> mode).
>
> Actually, shouldn't we clear the TPM on the opposite transition too?
>  Otherwise
> someone might return to the store a machine with a TPM owned by him, no?
>

Good point.

Anyhow: the SetupTPM API changes in this CL, and I fixed the callers so that
> the
> code compiles, but nothing else.  These are the changes:
>
> 1. There are two parameters, MODE and DEVELOPER_FLAG.  They must be passed
> accordingly to the boot mode.  (I know---not all combinations are
> meaningful.)
>
> 2. If SetupTPM returns TPM_SUCCESS, the caller may proceed.  If it returns
> TPM_MUST_REBOOT, the caller must reboot in normal mode.  For all other
> values,
> it must reboot in recovery mode.


I'll look at what we need to do to support TPM_MUST_REBOOT.  LoadFirmware()
/ LoadKernel() don't have a return code for that yet.

Thanks,
   Randall


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

[Luigi Semenzato, 2010-06-13 00:39:02]

On Sat, Jun 12, 2010 at 9:19 AM, Randall Spangler
<rspangler@chromium.org> wrote:

> I'll look at what we need to do to support TPM_MUST_REBOOT.  LoadFirmware()
> / LoadKernel() don't have a return code for that yet.

Thanks.  Either that, or I need a function pointer to a function that
will reboot in the chosen mode.

[gauravsh, 2010-06-14 10:57:53]

lgtm

http://codereview.chromium.org/2792009/diff/12001/6003
File vboot_firmware/include/rollback_index.h (right):

http://codereview.chromium.org/2792009/diff/12001/6003#newcode55
vboot_firmware/include/rollback_index.h:55: uint32_t SetupTPM(int mode, int
developer_flag);
comment about what the mode parameter, what does it denote, what modes can there
be, etc.

http://codereview.chromium.org/2792009/diff/12001/6007
File vboot_firmware/lib/vboot_firmware.c (right):

http://codereview.chromium.org/2792009/diff/12001/6007#newcode43
vboot_firmware/lib/vboot_firmware.c:43: /* TODO: fix SetupTPM parameter */
fix parameter how?

[Will Drewry, 2010-06-22 15:37:42]

On Fri, Jun 11, 2010 at 7:45 PM,  <semenzato@chromium.org> wrote:
> In addition to the changes described at the beginning of this thread, I have
> added code that detects transitions between developer mode and normal mode,
> and
> clears the TPM on transitions into developer mode (but not into normal
> mode).
>

Thanks for doing this! This might also be the right place to do a
not-in-use RAM wipe, but Randall will probably have a better opinion
on that :)

> Actually, shouldn't we clear the TPM on the opposite transition too?
>
>  Otherwise
> someone might return to the store a machine with a TPM owned by him, no?

Yup - also, if the SRK isn't available for use then we won't be able
to slave keychains off of it, etc and the user will have to go through
recovery.  Having to go through recovery after devmode use probably
won't be too uncommon, but it'd be nice to ensure that the TPM is in a
shiny state when we transition back if we can.

> Anyhow: the SetupTPM API changes in this CL, and I fixed the callers so that
> the
> code compiles, but nothing else.  These are the changes:
>
> 1. There are two parameters, MODE and DEVELOPER_FLAG.  They must be passed
> accordingly to the boot mode.  (I know---not all combinations are
> meaningful.)
>
> 2. If SetupTPM returns TPM_SUCCESS, the caller may proceed.  If it returns
> TPM_MUST_REBOOT, the caller must reboot in normal mode.  For all other
> values,
> it must reboot in recovery mode.
>
> http://codereview.chromium.org/2792009/show
>

[Luigi Semenzato, 2010-06-22 16:26:37]

On Tue, Jun 22, 2010 at 8:37 AM, Will Drewry <wad@chromium.org> wrote:
> On Fri, Jun 11, 2010 at 7:45 PM,  <semenzato@chromium.org> wrote:
>> In addition to the changes described at the beginning of this thread, I have
>> added code that detects transitions between developer mode and normal mode,
>> and
>> clears the TPM on transitions into developer mode (but not into normal
>> mode).
>>
>
> Thanks for doing this! This might also be the right place to do a
> not-in-use RAM wipe, but Randall will probably have a better opinion
> on that :)
>
>> Actually, shouldn't we clear the TPM on the opposite transition too?
>>
>>  Otherwise
>> someone might return to the store a machine with a TPM owned by him, no?
>
> Yup - also, if the SRK isn't available for use then we won't be able
> to slave keychains off of it, etc and the user will have to go through
> recovery.  Having to go through recovery after devmode use probably
> won't be too uncommon, but it'd be nice to ensure that the TPM is in a
> shiny state when we transition back if we can.

Thank you for confirming this.  The current code clears the TPM on
both transitions.

>> Anyhow: the SetupTPM API changes in this CL, and I fixed the callers so that
>> the
>> code compiles, but nothing else.  These are the changes:
>>
>> 1. There are two parameters, MODE and DEVELOPER_FLAG.  They must be passed
>> accordingly to the boot mode.  (I know---not all combinations are
>> meaningful.)
>>
>> 2. If SetupTPM returns TPM_SUCCESS, the caller may proceed.  If it returns
>> TPM_MUST_REBOOT, the caller must reboot in normal mode.  For all other
>> values,
>> it must reboot in recovery mode.
>>
>> http://codereview.chromium.org/2792009/show
>>
>