Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Dov Murik]

Hi Connor,

+cc: Daniel

On 23/06/2021 0:15, Connor Kuehl wrote:
> On 6/21/21 2:05 PM, Dov Murik wrote:
>> +static void fill_sev_hash_table_entry(SevHashTableEntry *e, const uint8_t 
>> *guid,
>> +                                      const uint8_t *hash, size_t hash_len)
>> +{
>> +    memcpy(e->guid, guid, sizeof(e->guid));
>> +    e->len = sizeof(*e);
>> +    memcpy(e->hash, hash, hash_len);
> 
> Should this memcpy be constrained to MIN(sizeof(e->hash), hash_len)? Or
> perhaps an assert statement since I see below that this function's
> caller sets this to HASH_SIZE which is currently == sizeof(e->hash).
> 
> Actually, the assert statement would be easier to debug if the input
> to this function is ever unexpected, especially since it avoids an
> outcome where the input is silently truncated; which is a pitfall that
> that the memcpy clamping would fall into.

I agree.  I'll change it to:

    assert(hash_len == sizeof(e->hash));
    memcpy(e->hash, hash, sizeof(e->hash));

This way also the memcpy length is known at compile-time.



Related: I wondered if I could replace HASH_SIZE in:


  /* hard code sha256 digest size */
  #define HASH_SIZE 32

  typedef struct QEMU_PACKED SevHashTableEntry {
      QemuUUID guid;
      uint16_t len;
      uint8_t hash[HASH_SIZE];
  } SevHashTableEntry;


with some SHA256-related constant from crypto/hash.h, but I only found
the qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_SHA256) function which
doesn't work for setting sizes of arrays at compile-time.

Daniel: do you know what would be the proper way?


Thanks,
-Dov