| Message ID | 20210621082641.26476-2-dja@axtens.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | secvar cleanups and fixes | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (1cdde9466ab658fb5b5a53af8c4e6a8929eef698) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
Hi Daniel, I hope you are still trying to merge this series despite the 4th patch being merged already. As for this patch: Agreed. Reviewed-by: Nick Child <nick.child@ibm.com> Also, I used secvarctl's test suite to test the new edk2-compat-process file. Not sure if that counts as Tested-by material since I did not flash a skiboot image onto any POWER NV machines but if so: Tested-by: Nick Child <nick.child@ibm.com> -Nick On Mon, Jun 21, 2021 at 4:27 AM Daniel Axtens <dja@axtens.net> wrote: > > verify_signature() currently takes newcert and new_data_len. However, > these variables are used only as parameters to > mbedtls_pkcs7_signed_hash_verify() where they represent a hash value > and the length of the hash value. > > verify_signature() is static, and the only caller of the function is > process_update(). process_update() passes in tbhbuffer and tbhbuffersize. > Those are unfortunate names too - because the data that process_update() > passes in is not a to-be-hashed buffer, but a hash. We'll fix that later. > > Call the parameters hash and hash_len. > > Signed-off-by: Daniel Axtens <dja@axtens.net> > --- > libstb/secvar/backend/edk2-compat-process.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c > index 244f23403fe0..8324dc068b8e 100644 > --- a/libstb/secvar/backend/edk2-compat-process.c > +++ b/libstb/secvar/backend/edk2-compat-process.c > @@ -455,7 +455,7 @@ out: > > /* Verify the PKCS7 signature on the signed data. */ > static int verify_signature(const struct efi_variable_authentication_2 *auth, > - const char *newcert, const size_t new_data_size, > + const char *hash, const size_t hash_len, > const struct secvar *avar) > { > mbedtls_pkcs7 *pkcs7 = NULL; > @@ -534,7 +534,7 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth, > free(x509_buf); > x509_buf = NULL; > > - rc = mbedtls_pkcs7_signed_hash_verify(pkcs7, &x509, newcert, new_data_size); > + rc = mbedtls_pkcs7_signed_hash_verify(pkcs7, &x509, hash, hash_len); > > /* If you find a signing certificate, you are done */ > if (rc == 0) { > -- > 2.30.2 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c index 244f23403fe0..8324dc068b8e 100644 --- a/libstb/secvar/backend/edk2-compat-process.c +++ b/libstb/secvar/backend/edk2-compat-process.c @@ -455,7 +455,7 @@ out: /* Verify the PKCS7 signature on the signed data. */ static int verify_signature(const struct efi_variable_authentication_2 *auth, - const char *newcert, const size_t new_data_size, + const char *hash, const size_t hash_len, const struct secvar *avar) { mbedtls_pkcs7 *pkcs7 = NULL; @@ -534,7 +534,7 @@ static int verify_signature(const struct efi_variable_authentication_2 *auth, free(x509_buf); x509_buf = NULL; - rc = mbedtls_pkcs7_signed_hash_verify(pkcs7, &x509, newcert, new_data_size); + rc = mbedtls_pkcs7_signed_hash_verify(pkcs7, &x509, hash, hash_len); /* If you find a signing certificate, you are done */ if (rc == 0) {
verify_signature() currently takes newcert and new_data_len. However, these variables are used only as parameters to mbedtls_pkcs7_signed_hash_verify() where they represent a hash value and the length of the hash value. verify_signature() is static, and the only caller of the function is process_update(). process_update() passes in tbhbuffer and tbhbuffersize. Those are unfortunate names too - because the data that process_update() passes in is not a to-be-hashed buffer, but a hash. We'll fix that later. Call the parameters hash and hash_len. Signed-off-by: Daniel Axtens <dja@axtens.net> --- libstb/secvar/backend/edk2-compat-process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)