New anti-rollback strategy (no TPM NVRAM write cycles for locking).

New anti-rollback strategy (no TPM NVRAM write cycles for locking).

[Luigi Semenzato, 2010-05-26 22:18:26]

Hi Gaurav,

these are the locking changes.  It occurs to me that we need a better space
initialization strategy.  I will work on it in a separate change.

[gauravsh, 2010-05-27 06:48:13]

Looking at this now but - your local repo seems to be out of date by a few
commits. Bill pushed a CL yesterday which changed the layout of the
vboot_reference directory. You might want to rebaseline your patch and make sure
everything works. 

On 2010/05/26 22:18:26, Luigi Semenzato wrote:
> Hi Gaurav,
> 
> these are the locking changes.  It occurs to me that we need a better space
> initialization strategy.  I will work on it in a separate change.

[gauravsh, 2010-05-27 06:57:50]

Just a few minor comments. I don't see any code that handles the backup spaces
(which was in your original design) - is that a part of NVRAM initialization
that you are planning for a separate CL?

http://codereview.chromium.org/2225005/diff/2001/3001
File src/platform/vboot_reference/common/include/rollback_index.h (right):

http://codereview.chromium.org/2225005/diff/2001/3001#newcode26
src/platform/vboot_reference/common/include/rollback_index.h:26: #define
FIRMWARE_KEY_VERSION_NV_INDEX  0x1005
Is there any differences logically/physically on which NVRAM index number we
use? In addition, where does the numbering start? It might make sense to use the
lowest 4 indices? (make it less arbitrary)

http://codereview.chromium.org/2225005/diff/2001/3003
File src/platform/vboot_reference/common/rollback_index.c (right):

http://codereview.chromium.org/2225005/diff/2001/3003#newcode154
src/platform/vboot_reference/common/rollback_index.c:154: void
LockKernelVersionsByLockingPP() {
Do we really want to expose implementation details in the function name? Why not
just have LockKernelVersions()?

http://codereview.chromium.org/2225005/diff/2001/3007
File src/platform/vboot_reference/vkernel/kernel_image_fw.c (right):

http://codereview.chromium.org/2225005/diff/2001/3007#newcode430
src/platform/vboot_reference/vkernel/kernel_image_fw.c:430:
LockKernelVersionsByLockingPP();
The verify kernel driver function is most likely going away. All the TPM
checking and updating logic during kernel verification will be performed by a
separate LoadKernel() function. 

In any case, no harm in having the change here - just giving you a heads up
about this function's eventual demise.

[Luigi Semenzato, 2010-05-27 19:19:03]

Thanks, done.

[Luigi Semenzato, 2010-05-27 19:29:08]

On 2010/05/27 06:57:50, gauravsh wrote:
> Just a few minor comments. I don't see any code that handles the backup spaces
> (which was in your original design) - is that a part of NVRAM initialization
> that you are planning for a separate CL?

Yes, I would like to do this in a separate CL today.


> LockKernelVersionsByLockingPP();
> The verify kernel driver function is most likely going away. All the TPM
> checking and updating logic during kernel verification will be performed by a
> separate LoadKernel() function. 
> 
> In any case, no harm in having the change here - just giving you a heads up
> about this function's eventual demise.

Thanks!

[Luigi Semenzato, 2010-05-27 19:30:50]

No changes to the code, but please read the explanation for the space indices
and the function name and let me know your preference.

http://codereview.chromium.org/2225005/diff/2001/3001
File src/platform/vboot_reference/common/include/rollback_index.h (right):

http://codereview.chromium.org/2225005/diff/2001/3001#newcode26
src/platform/vboot_reference/common/include/rollback_index.h:26: #define
FIRMWARE_KEY_VERSION_NV_INDEX  0x1005
On 2010/05/27 06:57:50, gauravsh wrote:
> Is there any differences logically/physically on which NVRAM index number we
> use? In addition, where does the numbering start? It might make sense to use
the
> lowest 4 indices? (make it less arbitrary)

The numbers don't make any difference.  I was bumping them to avoid problems for
those that have already initialized the spaces (basically only Nelson). 
Otherwise they will need to install the nvram tool and manually delete those
spaces.  Do you suggest that it would be better to do that?

http://codereview.chromium.org/2225005/diff/2001/3003
File src/platform/vboot_reference/common/rollback_index.c (right):

http://codereview.chromium.org/2225005/diff/2001/3003#newcode154
src/platform/vboot_reference/common/rollback_index.c:154: void
LockKernelVersionsByLockingPP() {
On 2010/05/27 06:57:50, gauravsh wrote:
> Do we really want to expose implementation details in the function name? Why
not
> just have LockKernelVersions()?

Locking the kernel version has the side effect of locking the PP.  This is a big
side effect and I wanted to make sure that it stood out.  In other words, the
concern to abstract out the locking operation is much smaller than the concern
to make it immediately clear what's going on.  It is counterproductive to
pretend we have a clean abstraction here.  Also, if we change the locking
scheme, many other things will need to change.

But if you prefer it, I'll change the name and just add a comment!

[gauravsh, 2010-05-27 20:24:28]

lgtm

http://codereview.chromium.org/2225005/diff/2001/3001
File src/platform/vboot_reference/common/include/rollback_index.h (right):

http://codereview.chromium.org/2225005/diff/2001/3001#newcode26
src/platform/vboot_reference/common/include/rollback_index.h:26: #define
FIRMWARE_KEY_VERSION_NV_INDEX  0x1005
On 2010/05/27 19:30:50, Luigi Semenzato wrote:
> On 2010/05/27 06:57:50, gauravsh wrote:
> > Is there any differences logically/physically on which NVRAM index number we
> > use? In addition, where does the numbering start? It might make sense to use
> the
> > lowest 4 indices? (make it less arbitrary)
> 
> The numbers don't make any difference.  I was bumping them to avoid problems
for
> those that have already initialized the spaces (basically only Nelson). 
> Otherwise they will need to install the nvram tool and manually delete those
> spaces.  Do you suggest that it would be better to do that?
> 
> 

My concern is that this is not a very scalable solution. We'll need to bump up
the indices again when we make any minor changes to the locking mechanism.

Maybe, we should have a function that deletes any existing spaces with that
index (only to be run during test runs). That could be a part of a separate CL
though.

http://codereview.chromium.org/2225005/diff/2001/3003
File src/platform/vboot_reference/common/rollback_index.c (right):

http://codereview.chromium.org/2225005/diff/2001/3003#newcode154
src/platform/vboot_reference/common/rollback_index.c:154: void
LockKernelVersionsByLockingPP() {
On 2010/05/27 19:30:50, Luigi Semenzato wrote:
> On 2010/05/27 06:57:50, gauravsh wrote:
> > Do we really want to expose implementation details in the function name? Why
> not
> > just have LockKernelVersions()?
> 
> Locking the kernel version has the side effect of locking the PP.  This is a
big
> side effect and I wanted to make sure that it stood out.  In other words, the
> concern to abstract out the locking operation is much smaller than the concern
> to make it immediately clear what's going on.  It is counterproductive to
> pretend we have a clean abstraction here.  Also, if we change the locking
> scheme, many other things will need to change.
> 
> But if you prefer it, I'll change the name and just add a comment!
> 
> 

Ok, that makes sense. Let's revisit the issue of a clean abstraction at a later
point. Can you just add a small TODO for this?

[Luigi Semenzato, 2010-05-27 21:04:35]

Thanks.  I unbumped the spaces; we'll remove them at user level if we need to. 
I also added the TODO.  Pushing now.