Add recovery mode protection to new NVRAM locking scheme.

Add recovery mode protection to new NVRAM locking scheme.
Also fix the TPM initialization.

[Luigi Semenzato, 2010-05-28 00:50:54]

Hi Gaurav, good morning.  I added more thorough TPM first-time initialization
and also the kernel versions protection for entering recovery mode.  As a side
effect I noted that all NV operations were done in pairs, so I combined them to
reduce the number of reads (and writes) and reduce the number of spaces.  Not
sure it's much better one way or the other---I can put it back if you want.

BTW, this compiles (just doing a "make") but I haven't tried emerging it or
running it or anything else.  What do you recommend I do before I check it in? 
Thanks!

[gauravsh, 2010-05-28 07:57:52]

LGTM. Thanks!

As for making sure this won't cause another buildbot failure - try building the
firmware_VbootCrypto autotest for both x86 and ARM boards. If I recall
correctly, you can just build a particular test by passing the test name as an
argument to ./build_autotest.sh (or the equivalent script under src/scripts).

We don't have an ebuild for this yet - once we do, we can just try emerging the
package.

http://codereview.chromium.org/2344002/diff/3001/4003
File src/platform/vboot_reference/vboot_firmware/lib/firmware_image_fw.c
(right):

http://codereview.chromium.org/2344002/diff/3001/4003#newcode285
src/platform/vboot_reference/vboot_firmware/lib/firmware_image_fw.c:285:
(uint16_t) (min_lversion >> 16),
nit: weird extra space :)

http://codereview.chromium.org/2344002/diff/3001/4006
File src/platform/vboot_reference/vboot_firmware/lib/rollback_index.c (right):

http://codereview.chromium.org/2344002/diff/3001/4006#newcode62
src/platform/vboot_reference/vboot_firmware/lib/rollback_index.c:62: * locking
this space earlier or later.
can you elaborate on this TODO? Unclear to me what the issue is.

[Luigi Semenzato, 2010-05-28 16:35:15]

Hi---I removed the TODO and I explained why, or at least I tried.  Since you've
already LGTM'd it, I'll push it shortly.  Thanks!

http://codereview.chromium.org/2344002/diff/3001/4003
File src/platform/vboot_reference/vboot_firmware/lib/firmware_image_fw.c
(right):

http://codereview.chromium.org/2344002/diff/3001/4003#newcode285
src/platform/vboot_reference/vboot_firmware/lib/firmware_image_fw.c:285:
(uint16_t) (min_lversion >> 16),
On 2010/05/28 07:57:52, gauravsh wrote:
> nit: weird extra space :)

:) he he.  Just in case, the extra space is there because Version -> Versions.

http://codereview.chromium.org/2344002/diff/3001/4006
File src/platform/vboot_reference/vboot_firmware/lib/rollback_index.c (right):

http://codereview.chromium.org/2344002/diff/3001/4006#newcode62
src/platform/vboot_reference/vboot_firmware/lib/rollback_index.c:62: * locking
this space earlier or later.
On 2010/05/28 07:57:52, gauravsh wrote:
> can you elaborate on this TODO? Unclear to me what the issue is.

This space is only used to distinguish an error situation from an uninitialized
TPM.  It is created after all other spaces have been created and initialized. 
So, if some NVRAM operations fail, and this space does not exist, the RO
firmware will try to initialize the TPM.  If it exists, it will go right into
recovery mode.  Now: if we lock this space early, it can never be removed---not
even in recovery mode, with PP on.  I think it's OK because we don't really care
to re-execute InitializeSpaces in the RO firmware ever again.  If we really need
to, we can do it at user level in recovery mode.  So I am going to remove the
TODO---it's done.

[Luigi Semenzato, 2010-05-28 16:40:04]

By the way, I tested it with ./build_autotest.sh
--build=firmware_VbootCrypto as you suggested.


On Fri, May 28, 2010 at 9:35 AM,  <semenzato@chromium.org> wrote:
> Hi---I removed the TODO and I explained why, or at least I tried.  Since
> you've
> already LGTM'd it, I'll push it shortly.  Thanks!
>
>
> http://codereview.chromium.org/2344002/diff/3001/4003
> File src/platform/vboot_reference/vboot_firmware/lib/firmware_image_fw.c
> (right):
>
> http://codereview.chromium.org/2344002/diff/3001/4003#newcode285
> src/platform/vboot_reference/vboot_firmware/lib/firmware_image_fw.c:285:
> (uint16_t) (min_lversion >> 16),
> On 2010/05/28 07:57:52, gauravsh wrote:
>>
>> nit: weird extra space :)
>
> :) he he.  Just in case, the extra space is there because Version ->
> Versions.
>
> http://codereview.chromium.org/2344002/diff/3001/4006
> File src/platform/vboot_reference/vboot_firmware/lib/rollback_index.c
> (right):
>
> http://codereview.chromium.org/2344002/diff/3001/4006#newcode62
> src/platform/vboot_reference/vboot_firmware/lib/rollback_index.c:62: *
> locking this space earlier or later.
> On 2010/05/28 07:57:52, gauravsh wrote:
>>
>> can you elaborate on this TODO? Unclear to me what the issue is.
>
> This space is only used to distinguish an error situation from an
> uninitialized TPM.  It is created after all other spaces have been
> created and initialized.  So, if some NVRAM operations fail, and this
> space does not exist, the RO firmware will try to initialize the TPM.
> If it exists, it will go right into recovery mode.  Now: if we lock this
> space early, it can never be removed---not even in recovery mode, with
> PP on.  I think it's OK because we don't really care to re-execute
> InitializeSpaces in the RO firmware ever again.  If we really need to,
> we can do it at user level in recovery mode.  So I am going to remove
> the TODO---it's done.
>
> http://codereview.chromium.org/2344002/show
>

[gauravsh, 2010-05-28 18:39:43]

Cool, thanks Luigi!

On Fri, May 28, 2010 at 9:39 AM, Luigi Semenzato <semenzato@chromium.org> wrote:
> By the way, I tested it with ./build_autotest.sh
> --build=firmware_VbootCrypto as you suggested.
>
>
> On Fri, May 28, 2010 at 9:35 AM,  <semenzato@chromium.org> wrote:
>> Hi---I removed the TODO and I explained why, or at least I tried.  Since
>> you've
>> already LGTM'd it, I'll push it shortly.  Thanks!
>>
>>
>> http://codereview.chromium.org/2344002/diff/3001/4003
>> File src/platform/vboot_reference/vboot_firmware/lib/firmware_image_fw.c
>> (right):
>>
>> http://codereview.chromium.org/2344002/diff/3001/4003#newcode285
>> src/platform/vboot_reference/vboot_firmware/lib/firmware_image_fw.c:285:
>> (uint16_t) (min_lversion >> 16),
>> On 2010/05/28 07:57:52, gauravsh wrote:
>>>
>>> nit: weird extra space :)
>>
>> :) he he.  Just in case, the extra space is there because Version ->
>> Versions.
>>
>> http://codereview.chromium.org/2344002/diff/3001/4006
>> File src/platform/vboot_reference/vboot_firmware/lib/rollback_index.c
>> (right):
>>
>> http://codereview.chromium.org/2344002/diff/3001/4006#newcode62
>> src/platform/vboot_reference/vboot_firmware/lib/rollback_index.c:62: *
>> locking this space earlier or later.
>> On 2010/05/28 07:57:52, gauravsh wrote:
>>>
>>> can you elaborate on this TODO? Unclear to me what the issue is.
>>
>> This space is only used to distinguish an error situation from an
>> uninitialized TPM.  It is created after all other spaces have been
>> created and initialized.  So, if some NVRAM operations fail, and this
>> space does not exist, the RO firmware will try to initialize the TPM.
>> If it exists, it will go right into recovery mode.  Now: if we lock this
>> space early, it can never be removed---not even in recovery mode, with
>> PP on.  I think it's OK because we don't really care to re-execute
>> InitializeSpaces in the RO firmware ever again.  If we really need to,
>> we can do it at user level in recovery mode.  So I am going to remove
>> the TODO---it's done.
>>
>> http://codereview.chromium.org/2344002/show
>>
>



-- 
-g