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

[Eric Richter]

On 11/2/21 8:33 AM, Daniel Axtens wrote:
> Eric Richter <erichte at linux.ibm.com> writes:
> 
>> 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.

>> +
>> +	tpmnv_control_image->active_bit = 0;
> 
> This bit I get.
> 
>>  
>>  	rc = tpmnv_ops.write(SECBOOT_TPMNV_CONTROL_INDEX,
>> -			     tpmnv_control_image->bank_hash[0],
>> -			     SHA256_DIGEST_SIZE,
>> -			     offsetof(struct tpmnv_control,
>> -			     bank_hash[0]));
>> +			     tpmnv_control_image,
>> +			     sizeof(struct tpmnv_control),
>> +			     0);
> 
> I understand that this writes out both bank hashes and the active
> bit. It'll also write out the header again but hard to see that being an
> issue. This looks good to me.
> 
> Kind regards,
> Daniel
>