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

[Dov Murik]

On 03/11/2021 17:44, Brijesh Singh wrote:
> 
> 
> 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.
> 

(We can overcome that by changing to a new GUID and modifying the reset
vector asm file to include the SEV hashes GUID only in the AmdSev build.
Requires changes both in OVMF and in QEMU.)

But some people want to use a non-hash-validating OVMF build with
-kernel (that's the "old OVMF" scenario that Tom presented).  In that case:

1. QEMU won't find the GUID, and will fail. This is breaking behaviour
for existing users.
2. If we just warn (as this patch suggested), then we don't enforce the
secure behaviour that some users want.

Following Daniel's suggestion, I think I'm going to send another round
in which the whole kernel hashes addition will happen only if the user
explicitly requested:

  -object sev_guest,id=sev0,...,kernel_hashes=on

With the default being 'off'.

(and change the warn_report above to error_report so they are fatal.)

This is basically keeping the new functionality for the release under a
feature flag, so users that want to use it will enable it explicitly and
all other users have the same behaviour as in previous releases.


As Daniel mentioned, we can also consider an 'auto' mode in which we add
the kernel hashes only if the GUID exists in OVMF, but I actually came
to an understanding that this is too confusing in the state of OVMF
builds right now.

Users that use the tighter OVMF build (AmdSev) will get a boot failure
from OVMF if they don't define kernel_hashes=on, so that won't be
accidentally missed.



-Dov