Add structs for TPM NV simplification

Add structs for TPM NV simplification.  Now uses only 2 NV spaces, one for firmware and one for kernel.

Changed TlclRead / TlclWrite to take void* / const void* to reduce typecasts.

Much restructuring of rollback_index.c.

Fixed a version-packing bug in rollback_index.c (& --> |)

BUG:chrome-os-partner:304
TEST:manual testing of all code flows on CRB

[Luigi Semenzato, 2010-08-09 20:58:16]

Good idea.  

Do we have to worry that the MSVC and GNU compilers will generate the same
structure offsets?  Also, is it possible to pack individual structures with GNU
cc?  I know there is a global flag (I've used it) but that could be a problem
when compiling for user-level for library calls that pass structure arguments.

Is your plan to do the writes to in-memory copies of the structure, and then
flush the structures to the TPM (if needed) before locking?  Then there is the
extra pain of passing the struct around in the RO firmware (no globals).

[Luigi Semenzato, 2010-08-10 15:15:50]

LGTM if you want to commit this separately.

On 2010/08/09 20:58:16, Luigi Semenzato wrote:
> Good idea.  
> 
> Do we have to worry that the MSVC and GNU compilers will generate the same
> structure offsets?  Also, is it possible to pack individual structures with
GNU
> cc?  I know there is a global flag (I've used it) but that could be a problem
> when compiling for user-level for library calls that pass structure arguments.
> 
> Is your plan to do the writes to in-memory copies of the structure, and then
> flush the structures to the TPM (if needed) before locking?  Then there is the
> extra pain of passing the struct around in the RO firmware (no globals).

[Randall Spangler, 2010-08-10 15:41:31]

Thanks.  I think I'll reuse this CL for the code which uses the structs, to
keep the structs together with the code which uses them.  Should be coming
your way today.

- Randall

On Tue, Aug 10, 2010 at 8:15 AM, <semenzato@chromium.org> wrote:

> LGTM if you want to commit this separately.
>
>
> On 2010/08/09 20:58:16, Luigi Semenzato wrote:
>
>> Good idea.
>>
>
>  Do we have to worry that the MSVC and GNU compilers will generate the same
>> structure offsets?  Also, is it possible to pack individual structures
>> with
>>
> GNU
>
>> cc?  I know there is a global flag (I've used it) but that could be a
>> problem
>> when compiling for user-level for library calls that pass structure
>> arguments.
>>
>
>  Is your plan to do the writes to in-memory copies of the structure, and
>> then
>> flush the structures to the TPM (if needed) before locking?  Then there is
>> the
>> extra pain of passing the struct around in the RO firmware (no globals).
>>
>
>
>
> http://codereview.chromium.org/3084030/show
>

[Luigi Semenzato, 2010-08-12 01:12:30]

LGTM after considering comments and changing type *var to type* var if it
pleases you.

BTW, I tried hard to find errors (new or old ones).

http://codereview.chromium.org/3084030/diff/5001/6001
File firmware/lib/include/rollback_index.h (right):

http://codereview.chromium.org/3084030/diff/5001/6001#newcode97
firmware/lib/include/rollback_index.h:97: /* TODO: use a 32-bit version instead
of 2 version pieces */
Maybe have Gaurav review this (future) change.

http://codereview.chromium.org/3084030/diff/5001/6002
File firmware/lib/rollback_index.c (left):

http://codereview.chromium.org/3084030/diff/5001/6002#oldcode107
firmware/lib/rollback_index.c:107: RETURN_ON_FAILURE(TPMClearAndReenable());
We just discussed this in person.  We have this code here because of possible
random development flow on devices---but this is going to be so uncommon that we
might as well let the space creation fail and go to recovery mode.

http://codereview.chromium.org/3084030/diff/5001/6002
File firmware/lib/rollback_index.c (right):

http://codereview.chromium.org/3084030/diff/5001/6002#newcode73
firmware/lib/rollback_index.c:73: return TlclRead(FIRMWARE_NV_INDEX, rsf,
sizeof(RollbackSpaceFirmware));
I think Google's convention is to put the star near the type, not the variable
(wrong, but that's the convention).

http://codereview.chromium.org/3084030/diff/5001/6002#newcode125
firmware/lib/rollback_index.c:125: RETURN_ON_FAILURE(WriteSpaceKernel(rsk));
Let's see if I have this right.  You removed the TPM_IS_INITIALIZED_NV_INDEX
because we can detect if the initializing write happened to the KERNEL_NV_INDEX
by the presence of the GRWL.  But now we have an ambiguity: is the GRWL not
present because the initialization didn't complete, or because of an attack or a
bug?  It may not matter.

[gauravsh, 2010-08-12 02:15:51]

http://codereview.chromium.org/3084030/diff/5001/6002
File firmware/lib/rollback_index.c (right):

http://codereview.chromium.org/3084030/diff/5001/6002#newcode73
firmware/lib/rollback_index.c:73: return TlclRead(FIRMWARE_NV_INDEX, rsf,
sizeof(RollbackSpaceFirmware));
The style guide doesn't enforce any. convention but requires you to be
consistent. I think.

On 2010/08/12 01:12:30, Luigi Semenzato wrote:
> I think Google's convention is to put the star near the type, not the variable
> (wrong, but that's the convention).

[Randall Spangler, 2010-08-12 20:00:47]

Fixed and submitting.  Thanks.

http://codereview.chromium.org/3084030/diff/5001/6002
File firmware/lib/rollback_index.c (right):

http://codereview.chromium.org/3084030/diff/5001/6002#newcode73
firmware/lib/rollback_index.c:73: return TlclRead(FIRMWARE_NV_INDEX, rsf,
sizeof(RollbackSpaceFirmware));
On 2010/08/12 01:12:30, Luigi Semenzato wrote:
> I think Google's convention is to put the star near the type, not the variable
> (wrong, but that's the convention).

Done.

http://codereview.chromium.org/3084030/diff/5001/6002#newcode125
firmware/lib/rollback_index.c:125: RETURN_ON_FAILURE(WriteSpaceKernel(rsk));
On 2010/08/12 01:12:30, Luigi Semenzato wrote:
> Let's see if I have this right.  You removed the TPM_IS_INITIALIZED_NV_INDEX
> because we can detect if the initializing write happened to the
KERNEL_NV_INDEX
> by the presence of the GRWL.  But now we have an ambiguity: is the GRWL not
> present because the initialization didn't complete, or because of an attack or
a
> bug?  It may not matter.

Either way, we'll need to fix it in recovery mode.