[Skiboot] [PATCH 2/3] secvar/backend: clarify variables in process_update

[Nayna]

On 5/25/21 8:49 AM, Nayna wrote:
>
> On 5/24/21 11:34 PM, Daniel Axtens wrote:
>> process_update() has tbhbuffer and tbhbuffersize. However, tbhbuffer
>> doesn't contain to-be-hashed data, but a hash:
>>
>> /* Prepare the data to be verified */
>> tbhbuffer = get_hash_to_verify(update->key, *newesl, *new_data_size,
>>             timestamp);
>>
>> And tbhbuffersize is initalised to zero and then never filled with
>> the actual length, so 0 is passed through to verify_signature.
>> verify_signature will in turn pass that to mbedtls, which will interpret
>> it as "figure out the length yourself based on the hash type". So this
>> has always worked, but by accident.
>>
>> Rename tbhbuffer to hash, as that's what it is.
>>
>> Drop tbhbuffersize, and pass 32 directly to verify_signature. We only
>> support SHA-256, and SHA-256 is 32 bytes long.
>>
>> Signed-off-by: Daniel Axtens <dja at axtens.net>
>
> Thanks Daniel for identifying and fixing it.
>
>
>> ---
>>   libstb/secvar/backend/edk2-compat-process.c | 14 ++++++--------
>>   1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/libstb/secvar/backend/edk2-compat-process.c 
>> b/libstb/secvar/backend/edk2-compat-process.c
>> index 8324dc068b8e..b39f35e0c3dc 100644
>> --- a/libstb/secvar/backend/edk2-compat-process.c
>> +++ b/libstb/secvar/backend/edk2-compat-process.c
>> @@ -662,8 +662,7 @@ int process_update(const struct secvar *update, 
>> char **newesl,
>>       void *auth_buffer = NULL;
>>       int auth_buffer_size = 0;
>>       const char *key_authority[3];
>> -    char *tbhbuffer = NULL;
>> -    size_t tbhbuffersize = 0;
>> +    char *hash = NULL;
>>       struct secvar *avar = NULL;
>>       int rc = 0;
>>       int i;
>> @@ -723,9 +722,9 @@ int process_update(const struct secvar *update, 
>> char **newesl,
>>       }
>>         /* Prepare the data to be verified */
>> -    tbhbuffer = get_hash_to_verify(update->key, *newesl, 
>> *new_data_size,
>> +    hash = get_hash_to_verify(update->key, *newesl, *new_data_size,
>>                   timestamp);
>> -    if (!tbhbuffer) {
>> +    if (!hash) {
>>           rc = OPAL_INTERNAL_ERROR;
>>           goto out;
>>       }
>> @@ -746,9 +745,8 @@ int process_update(const struct secvar *update, 
>> char **newesl,
>>           if (!avar || !avar->data_size)
>>               continue;
>>   -        /* Verify the signature */
>> -        rc = verify_signature(auth, tbhbuffer, tbhbuffersize,
>> -                      avar);
>> +        /* Verify the signature. sha256 is 32 bytes long. */
>> +        rc = verify_signature(auth, hash, 32, avar);
>
> This does work. I am thinking that we still pass here zero like it was 
> previously and fix the documentation of 
> mbedtls_pkcs7_signed_hash_verify() in pkcs7.h.
>
> The documentation makes it explicit that caller can pass 0 or 
> sizeof(hash).  This will also make it consistent with general 
> behaviour of mbedtls API where it allows to pass zero and figures out 
> by itself.
>
>

Oops, I wanted to write length of hash.. but by mistake wrote it as 
sizeof(hash).

Thanks & Regards,

      - Nayna