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 :|
