Protect the kernel version space from redefinition.

Protect the kernel version space from redefinition.

[Luigi Semenzato, 2010-06-10 05:18:58]

This fixes the problem we discussed today, in which an attacker can try to
redefine the kernel version space, ultimately to no avail.  The logic is fairly
complex---I will have to read this many more time to convince myself it all
works, but in the meanwhile your help would be extremely useful.

One thing that worries me slightly is that the number of TPM operations at boot
has increased considerably.  In the "fast" path now we must read the value of
MUST_USE_BACKUP (this operation was there already for the recovery mode), and in
addition we must check the permissions on the kernel space and read the kernel
space to verify that the GRWL is still there.  The latter read may be combined
with the kernel space version numbers read---but that would be a premature
optimization for this CL.

[gauravsh, 2010-06-10 15:56:35]

http://codereview.chromium.org/2786005/diff/2001/1005
File vboot_firmware/include/rollback_index.h (right):

http://codereview.chromium.org/2786005/diff/2001/1005#newcode31
vboot_firmware/include/rollback_index.h:31: #define KERNEL_SPACE_UID "GRWL"     
            /* Our initials :-) */
nit: Who is "Our"? ;)

http://codereview.chromium.org/2786005/diff/2001/1006
File vboot_firmware/include/tlcl.h (right):

http://codereview.chromium.org/2786005/diff/2001/1006#newcode116
vboot_firmware/include/tlcl.h:116: 
This seems to be from your other patch which adds this function. Make sure
things don't get clobbered when you check them in.

http://codereview.chromium.org/2786005/diff/2001/1008
File vboot_firmware/lib/rollback_index.c (right):

http://codereview.chromium.org/2786005/diff/2001/1008#newcode184
vboot_firmware/lib/rollback_index.c:184: if (read_OK) {
is this assuming TPM_SUCCESS is non-zero? shouldn't you check if TPM_SUCESS ==
read_OK?

http://codereview.chromium.org/2786005/diff/5001/6004#newcode37
vboot_firmware/lib/rollback_index.c:37: static uint32_t
GetSpacesInitialized(int* initialized) {
a better name function for this would be InitializeSpaces(). In, usual fx name
convention, Get implies this retrieves and returns something.

http://codereview.chromium.org/2786005/diff/5001/6004#newcode229
vboot_firmware/lib/rollback_index.c:229: if (kernel_versions != backup_versions)
{
would it be safe to say that backup_version should always <= kernel_versions (if
kernel_versions can be trusted). Can you add that as an additional sanity check
before the write?

[Luigi Semenzato, 2010-06-10 17:07:05]

I uploaded a change to the MAXNVWRITE strategy, PTAL

http://codereview.chromium.org/2786005/diff2/5001:13001/14004

The only question I have is what to do when backup_version > kernel_version. 
Unlocked recovery mode?

http://codereview.chromium.org/2786005/diff/2001/1005
File vboot_firmware/include/rollback_index.h (right):

http://codereview.chromium.org/2786005/diff/2001/1005#newcode31
vboot_firmware/include/rollback_index.h:31: #define KERNEL_SPACE_UID "GRWL"     
            /* Our initials :-) */
On 2010/06/10 15:56:36, gauravsh wrote:
> nit: Who is "Our"? ;)

That would be Gaurav, Randall, Will, Luigi... wait, you know that.  Yes, I know,
I didn't put the names in.  That doesn't seem right, does it?

But wait... we do put names in the TODOs. OK.

http://codereview.chromium.org/2786005/diff/2001/1006
File vboot_firmware/include/tlcl.h (right):

http://codereview.chromium.org/2786005/diff/2001/1006#newcode116
vboot_firmware/include/tlcl.h:116: 
On 2010/06/10 15:56:36, gauravsh wrote:
> This seems to be from your other patch which adds this function. Make sure
> things don't get clobbered when you check them in.

OK will do, thanks.

http://codereview.chromium.org/2786005/diff/2001/1008
File vboot_firmware/lib/rollback_index.c (right):

http://codereview.chromium.org/2786005/diff/2001/1008#newcode184
vboot_firmware/lib/rollback_index.c:184: if (read_OK) {
On 2010/06/10 15:56:36, gauravsh wrote:
> is this assuming TPM_SUCCESS is non-zero? shouldn't you check if TPM_SUCESS ==
> read_OK?

Maybe you didn't read the whole line.  I am checking that the return code is
TPM_SUCCESS.

http://codereview.chromium.org/2786005/diff/5001/6004#newcode37
vboot_firmware/lib/rollback_index.c:37: static uint32_t
GetSpacesInitialized(int* initialized) {
On 2010/06/10 15:56:37, gauravsh wrote:
> a better name function for this would be InitializeSpaces(). In, usual fx name
> convention, Get implies this retrieves and returns something.

Yes---this is what this function is doing.  It's setting the "initialized"
parameter to 1 if the spaces have already been initialized, 0 if not.  There is
another function called InitializeSpaces below, and that one initializes the
spaces.

I did it this way to simplify some of the logic.  GetSpacesInitialized can fail
if the TPM fails.  The caller needs to do three different things: if the spaces
are initialized, do nothing; if they aren't, try to initialize them; if an error
occurs, enter recovery mode.

http://codereview.chromium.org/2786005/diff/5001/6004#newcode229
vboot_firmware/lib/rollback_index.c:229: if (kernel_versions != backup_versions)
{
On 2010/06/10 15:56:37, gauravsh wrote:
> would it be safe to say that backup_version should always <= kernel_versions
(if
> kernel_versions can be trusted). Can you add that as an additional sanity
check
> before the write? 

So at this point kernel version can be trusted, therefore yes, backup version
should be <=.  (Backup version can always be trusted.)  But what should I do if
backup version > kernel version?

[gauravsh, 2010-06-10 17:34:43]

lgtm

http://codereview.chromium.org/2786005/diff/2001/1008
File vboot_firmware/lib/rollback_index.c (right):

http://codereview.chromium.org/2786005/diff/2001/1008#newcode184
vboot_firmware/lib/rollback_index.c:184: if (read_OK) {
On 2010/06/10 17:07:05, Luigi Semenzato wrote:
> On 2010/06/10 15:56:36, gauravsh wrote:
> > is this assuming TPM_SUCCESS is non-zero? shouldn't you check if TPM_SUCESS
==
> > read_OK?
> 
> Maybe you didn't read the whole line.  I am checking that the return code is
> TPM_SUCCESS.
> 

Ah yes, missed that. :)

http://codereview.chromium.org/2786005/diff/5001/6004#newcode229
vboot_firmware/lib/rollback_index.c:229: if (kernel_versions != backup_versions)
{
On 2010/06/10 17:07:07, Luigi Semenzato wrote:
> On 2010/06/10 15:56:37, gauravsh wrote:
> > would it be safe to say that backup_version should always <= kernel_versions
> (if
> > kernel_versions can be trusted). Can you add that as an additional sanity
> check
> > before the write? 
> 
> So at this point kernel version can be trusted, therefore yes, backup version
> should be <=.  (Backup version can always be trusted.)  But what should I do
if
> backup version > kernel version?

recovery I guess, or maybe internal error and system halt.

http://codereview.chromium.org/2786005/diff/16001/2002
File vboot_firmware/include/rollback_index.h (right):

http://codereview.chromium.org/2786005/diff/16001/2002#newcode31
vboot_firmware/include/rollback_index.h:31: #define KERNEL_SPACE_UID "GRWL"  /*
Gaurav, Randall, Will, Luigi --of course! */
Actually, I think it is better to not have this comment. Vanity and all.