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

[Daniel P . Berrangé]

On Wed, Jun 23, 2021 at 11:41:56AM +0300, Dov Murik wrote:
> 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?

We don't have any public constants right now - they're just hardcoded
in hash.c struct. We could define public constants, and use those in
the struct instead, as well as in other callers.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|