[Skiboot] [PATCH v2 1/6] secvar/secboot_tpm: correctly reset the control index on secboot format

[Daniel Axtens]

>>> When the SECBOOT partition is formatted, the bank hash stored in the
>>> control TPM NV index must be updated to match, or else we will immediately
>>> fail to load the freshly formatted data at the .load_bank() step.
>>>
>>> However, while the secboot_format() function does calculate and update the
>>> bank hash, it only writes the new hash for bank 0. It does not update the
>>> value for bank 1, or set the current active bank. This works as expected if
>>> the active bank bit happens to be set to 0. On the other hand, if the active
>>> bit is set to 1, the freshly formatted bank 1 will be compared against the
>>> unchanged bank hash in bank 1 at the load step, therefore causing an error.
>>>
>>> This patch fixes this issue by also setting the active bit to 0 to match
>>> the freshly calculated hash.
>>>
>>> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
>>> Tested-by: Nick Child <nick.child at ibm.com>
>>> ---
>>>  libstb/secvar/storage/secboot_tpm.c | 11 +++++++----
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libstb/secvar/storage/secboot_tpm.c b/libstb/secvar/storage/secboot_tpm.c
>>> index 129f674a..5907ff07 100644
>>> --- a/libstb/secvar/storage/secboot_tpm.c
>>> +++ b/libstb/secvar/storage/secboot_tpm.c
>>> @@ -127,12 +127,15 @@ static int secboot_format(void)
>>>  		prlog(PR_ERR, "Bank hash failed to calculate somehow\n");
>>>  		return rc;
>>>  	}
>>> +	/* Clear bank_hash[1] anyway, to match state of PNOR */
>>> +	memset(tpmnv_control_image->bank_hash[1], 0x00, sizeof(tpmnv_control_image->bank_hash[1]));
>> 
>> I am a bit confused by this line. The next thing you do is to set the
>> active bit to 0, which I understand and which matches what the commit
>> message says, but I don't understand why setting the bank hash of the
>> first bank to 0 is the right thing to do. The sha256 of a run of zeros
>> isn't going to be 0, so this is still - afaict - wrong, just differently
>> wrong?
>> 
>
> It's not entirely needed, no. The hash won't match, but it will match 
> the TPM NV state from the first boot, where both PNOR partitions are
> empty, active_bit = 0, bank_hash[0] = sha256(empty), bank_hash[0] = 0
>
> Only the bank specified by the active bit is verified, so the hash
> of the other bank isn't read, and will be overwritten on the next
> variable bank write.

Ok, fair enough. It makes sense to put _something_ there rather than
leaving it with the preexisting hash value which will now be effectively
random value. (Certainly if I ever had to go through a
hexdump of the pnor I'd appreciate less random data.) I guess zeros are
as good as anything, unless you wanted to have '0x0bad' or something.

Reviewed-by: Daniel Axtens <dja at axtens.net>

Regards,
Daniel