| Message ID | 20211101220513.835940-2-erichte@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Secvar adjustments and fixes | expand |
Eric Richter <erichte@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@linux.ibm.com> > Tested-by: Nick Child <nick.child@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? > + > + 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
On 11/2/21 8:33 AM, Daniel Axtens wrote: > Eric Richter <erichte@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@linux.ibm.com> >> Tested-by: Nick Child <nick.child@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 >
>>> 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@linux.ibm.com> >>> Tested-by: Nick Child <nick.child@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@axtens.net> Regards, Daniel
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])); + + tpmnv_control_image->active_bit = 0; 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); if (rc) { prlog(PR_ERR, "Could not write fresh formatted bank hashes to CONTROL index, rc=%d\n", rc); return rc;