Re: [PATCH 0/3] SEV: fixes for -kernel launch with incompatible OVMF

[Brijesh Singh <brijesh.singh@amd.com>]

On 11/3/21 9:08 AM, Dr. David Alan Gilbert wrote:
> * Brijesh Singh (brijesh.singh@amd.com) wrote:
>>
>>
>> On 11/2/21 8:22 AM, Dov Murik wrote:
>>>
>>>
>>> On 02/11/2021 12:52, Brijesh Singh wrote:
>>>> Hi Dov,
>>>>
>>>> Overall the patch looks good, only question I have is that now we are
>>>> enforce qemu to hash the kernel, initrd and cmdline unconditionally for
>>>> any of the SEV guest launches. This requires anyone wanting to
>>>> calculating the expected measurement need to account for it. Should we
>>>> make the hash page build optional ?
>>>>
>>>
>>> The problem with adding a -enable-add-kernel-hashes QEMU option (or
>>> suboption) is yet another complexity for the user.  I'd also argue that
>>> adding these hashes can lead to a more secure VM boot process, so it
>>> makes sense for it to be the default (and maybe introduce a
>>> -allow-insecure-unmeasured-kernel-via-fw-cfg option to prevent the
>>> measurement from changing due to addition of hashes?).
>>>
>>> Maybe, on the other hand, OVMF should "report" whether it supports
>>> hashes verification. If it does, it should have the GUID in the table
>>> (near the reset vector), like the current OvmfPkg/AmdSev edk2 build. If
>>> it doesn't support that, then the entry should not appear at all, and
>>> then QEMU won't add the hashes (with patch 1 from this series).  This
>>> means that in edk2 we need to remove the SEV Hash Table block from the
>>> ResetVectorVtf0.asm for OvmfPkg, but include it in the AmdSev build.
>>>
>>
>> By leaving it ON is conveying a wrong message to the user. The library used
>> for verifying the hash is a NULL library for all the builds of Ovmf except
>> the AmdSev package. In the NULL library case, OVMF does not perform any
>> checks and hash table is useless. I will raise this on concern on your Ovmf
>> patch series.
>>
>> IMHO, if you want to turn it ON by default then make sure all the OVMF
>> package builds supports validating the hash.
>>
>>
>>> But the problem with this approach is that it prevents the future
>>> unification of AmdSev and OvmfPkg, which is a possibility we discussed
>>> (at least with Dave Gilbert), though not sure it's a good/feasible goal.
>>>
>>>
>>
>> This is my exact concern, we are auto enabling the features in Qemu that is
>> supported by AmdSev package only.
> 
> I'm confused; wouldn't the trick be to only define the GUIDs for the
> builds that support the validation?
> 

The GUID is hardcoded in the OVMF reset vector asm file, and the file 
gets included for all the flavor of OVMF builds. In its current form, 
GUID is defined for all the package.

thanks