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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 6/22/21 11:44 AM, Dov Murik wrote:
> On 21/06/2021 23:32, Philippe Mathieu-Daudé wrote:

>> and added qemu_uuid_copy() to complete the API, but that's fine.
> 
> I think simple C assignment works for structs (and hence QemuUUID),
> something like:
> 
>     SevHashTable *ht = ...;
>     ht->guid = sev_hash_table_header_guid;
> 
> (where both ht->guid and sev_hash_table_header_guid are QemuUUID.)

OK.

>>> +    if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
>>
>> If we never use the data_len argument, can we simplify the prototype?
> 
> The current uses for the OVMF reset vector GUIDed table is for simple
> structs with known length (secret injection page address, SEV-ES reset
> address, SEV table of hashes address).  But keeping the length there
> allows adding variable-sized entries such as strings/blobs.

OK. Good opportunity to document the prototype declaration ;)

> 
>>
>>> +        error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid");
>>> +        return false;
>>> +    }
>>> +    area = (SevHashTableDescriptor *)data;
>>> +
>>> +    ht = qemu_map_ram_ptr(NULL, area->base);
>>> +
>>> +    /* Populate the hashes table header */
>>> +    memcpy(ht->guid, sev_hash_table_header_guid, sizeof(ht->guid));
>>> +    ht->len = sizeof(*ht);
>>> +
>>> +    /* Calculate hash of kernel command-line */
>>> +    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->cmdline_data,
>>> +                           ctx->cmdline_size,
>>> +                           &hash, &hash_len, errp) < 0) {
>>> +        return false;
>>> +    }
>>
>> Maybe move the qcrypto_hash_bytes() call before filling ht?
> 
> (below)
> 
>>
>>> +    e = &ht->entries[ht_index++];
>>> +    fill_sev_hash_table_entry(e, sev_cmdline_entry_guid, hash, hash_len);
>>> +
>>> +    /* Calculate hash of initrd */
>>> +    if (ctx->initrd_data) {
>>> +        if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->initrd_data,
>>> +                               ctx->initrd_size, &hash, &hash_len, errp) < 0) {
>>> +            return false;
>>> +        }
>>
>> Ah, now I see the pattern. Hmm OK then.
>>
> 
> But this might change if initrd_hash is no longer optional (see separate
> self-reply to this patch).  In such a case I'll probably first calculate
> all the three hashes, and then fill in the SevHashTable struct.

Yes, sounds simpler.

Regards,

Phil.