Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline

[Philippe Mathieu-Daudé]

Hi Dov,

+Thomas

On 6/17/21 2:48 PM, Dov Murik wrote:
> On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
>> Hi Dov, James,
>>
>> +Connor who asked to be reviewer.
>>
>> On 6/15/21 5:20 PM, Eduardo Habkost wrote:
>>> On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
>>>> From: James Bottomley <jejb@linux.ibm.com>
>>>>
>>>> If the VM is using memory encryption and also specifies a kernel/initrd
>>>> or appended command line, calculate the hashes and add them to the
>>>> encrypted data.  For this to work, OVMF must support an encrypted area
>>>> to place the data which is advertised via a special GUID in the OVMF
>>>> reset table (if the GUID doesn't exist, the user isn't allowed to pass
>>>> in the kernel/initrd/cmdline via the fw_cfg interface).
>>>>
>>>> The hashes of each of the files is calculated (or the string in the case
>>>> of the cmdline with trailing '\0' included).  Each entry in the hashes
>>>> table is GUID identified and since they're passed through the memcrypt
>>>> interface, the hash of the encrypted data will be accumulated by the
>>>> PSP.
>>>>
>>>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>>>> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
>>>> strings, remove GCC pragma, fix checkpatch errors]
>>>> ---
>>>>
>>>> OVMF support for handling the table of hashes (verifying that the
>>>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
>>>> to the measured hashes in the table) will be posted soon to edk2-devel.
>>>>
>>>> ---
>>>>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 119 insertions(+), 1 deletion(-)
>>>>
>>>
>>> This is not an objection to the patch itself, but: can we do
>>> something to move all sev-related code to sev.c?  It would make
>>> the process of assigning a maintainer and reviewing/merging
>>> future patches much simpler.
>>>
>>> I am not familiar with SEV internals, so my only question is
>>> about configurations where SEV is disabled:
>>>
>>> [...]
>>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>>>> @@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
>>>>      const char *initrd_filename = machine->initrd_filename;
>>>>      const char *dtb_filename = machine->dtb;
>>>>      const char *kernel_cmdline = machine->kernel_cmdline;
>>>> +    uint8_t buf[HASH_SIZE];
>>>> +    uint8_t *hash = buf;
>>>> +    size_t hash_len = sizeof(buf);
>>>> +    struct sev_hash_table *sev_ht = NULL;
>>>> +    int sev_ht_index = 0;
>>
>> Can you move all these variable into a structure, and use it as a
>> SEV loader context?
>>
>> Then each block of code you added can be moved to its own function,
>> self-described, working with the previous context.
>>
>> The functions can be declared in sev_i386.h and defined in sev.c as
>> Eduardo suggested.
>>
> 
> Thanks Philippe, I'll try this approach.
> 
> 
> I assume you mean that an addition like this:
> 
> +    if (sev_ht) {
> +        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
> +
> +        qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
> +                           strlen(kernel_cmdline) + 1,
> +                           &hash, &hash_len, &error_fatal);
> +        memcpy(e->hash, hash, hash_len);
> +        e->len = sizeof(*e);
> +        memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
> +    }
> 
> will be extracted to a function, and here (in x86_load_linux()) replaced
> with a single call:
> 
>     sev_kernel_loader_calc_cmdline_hash(&sev_loader_context, kernel_cmdline);
>   
> and that function will have an empty stub in non-SEV builds, and a do-
> nothing condition (at the beginning) if it's an SEV-disabled VM, and
> will do the actual work in SEV VMs.

This works, but I'd rather use:

  if (sev_enabled()) {
      sev_kernel_loader_calc_cmdline_hash(&sev_loader_context,
                                          kernel_cmdline);
  }

And have sev_enabled() defined as:

#ifdef CONFIG_SEV
bool sev_enabled(void);
#else
#define sev_enabled() false
#endif

So the compiler could elide the statement if SEV is disabled,
and stub is not necessary.

But that means we'd need to add "#include CONFIG_DEVICES" in
a sysemu/ header, which looks like an anti-pattern.

Thomas / Paolo, what do you think?

> Also, should I base my next version on top of the current master, or on
> top of your SEV-Housekeeping patch series, or on top of some other tree?

It depends its shape :> If the maintainers disagree with it, you better
base your patch on Eduardo's tree. Indeed the code will be different.
If you think the housekeeping is worthwhile, you could also review it
to increase the odds to get it queued ;)

Regards,

Phil.