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

[Dov Murik]

On 08/11/2021 23:22, Brijesh Singh wrote:
> 
> 
> On 11/5/21 1:32 PM, Dov Murik wrote:
>>
>>
>> On 02/11/2021 16:48, Brijesh Singh 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 am thinking this more for the SEV-SNP guest. As you may be aware
>>>>> that
>>>>> with SEV-SNP the attestation is performed by the guest, and its
>>>>> possible
>>>>> for the launch flow to pass 512-bits of host_data that gets
>>>>> included in
>>>>> the report. If a user wants to do the hash'e checks for the SNP then
>>>>> they can pass a hash of kernel, initrd and cmdline through a
>>>>> launch_finish.ID_BLOCK.host_data and does not require a special hash
>>>>> page. This it will simplify the expected hash calculation.
>>>>
>>>> That is a new measured boot "protocol" that we can discuss, and see
>>>> whether it's better/easier than the existing one at hand that works on
>>>> SEV and SEV-ES.
>>>>
>>>> What I don't understand in your suggestion is who performs a SHA256 of
>>>> the fw_cfg blobs (kernel/initrd/cmdline) so they can later be verified
>>>> (though ideally earlier is better).  Can you describe the details
>>>> (step-by-step) of an SNP VM boot with -kernel/-initrd/-append and how
>>>> the measurement/attestation is performed?
>>>>
>>>>
>>>
>>> There are a multiple ways on how you can do a measured boot with the
>>> SNP.
>>>
>>> 1) VMPL0 (SVSM) can provide a complete vTPM (see the MSFT proposal on
>>> SNP mailing list).
>>>
>>> 2) Use your existing hashing approach with some changes to provide a bit
>>> more flexibility.
>>>
>>> 3) Use your existing hashing approach but zero out the hash page when
>>> -kernel is not used.
>>>
>>> Let me expand #2.
>>>
>>> While launching the SNP guest, a guest owner can provide a ID block that
>>> KVM will pass to the PSP during the guest launch flow. In the ID block
>>> there is a field called "host_data". A guest owner can do a hash of
>>> kernel/initrd/cmdline and include it in the "host_data" field. During
>>> the hash verification, the OVMF can call the SNP_GET_REPORT. The PSP
>>> will includes the "host_data" passed in the launch process in the report
>>> and OVMF can use it for the verification. Unlike the current
>>> implementation, this enables a guest owner to provides the hash without
>>> requiring any changes in the Qemu and thus affecting the measurement.
>>>
>>
>> Is there a way (in the current NP patches for OVMF) for OVMF to call
>> SNP_GET_REPORT? Or is this something we need to add support for? Will it
>> mess up the sequence numbers that are later going to be used by the
>> kernel as well when managing SNP guest requests?
>>
>>
> 
> The current OVMF patches does not add a library to query the attestation
> report yet. If required it should be possible to add such a libraries.
> The VMGEXIT is available to both Guest OS and Guest BIOS. The sequence
> number should not be an issue. As per the GHCB spec, the guest BIOS will
> save the sequence number in the secrets page reserved area and guest
> kernel can picked the next number from that region (its same as the
> kexec approach).
> 

OK, good to know. *If* we decide to use the host_data field to store the
hashes, then we would have to add the SNP_GET_REPORT functionality to OVMF.

>>
>>> One thing to note that both #2 and #3 requires ovmf to connect to guest
>>> owner to validate the report before using the "host_data" or "hash
>>> page".
>>>
>>
>> For direct boot (with -kernel/-initrd), I don't understand why OVMF
>> needs to contact the GO.  If OVMF can fetch the host_data field, and use
>> that to verify the blobs delivered from QEMU via fw_cfg, it should be
>> enough.
>>
> 
> Well, I am trying to match with the current model in which the Hash's
> are provided through the secrets injection for the comparision. In other
> words, the attestation is completed before OVMF does the hash
> comparison. So, if you want to have the same security property then you
> need to perform the attestation before comparing the hash'es because a
> malicious HV may bypass the guid check.
> 

In the current model (works for SEV and SEV-ES) the hashes are not
provided via secret injection; they are added by QEMU to the designated
hashes area in the guest.

If we only need the secret later (in userspace) then we can use a
similar model.  Hashes are either (a) added to the designated page (by
QEMU), or (b) passed in host_data (by QEMU).  OVMF fetch the hashes: (a)
by reading a memory page, or (b) by using SNP_GET_REPORT.  It will
verify them against the blobs from fw_cfg, and will continue to boot
only if they match.

and then it continues as I previously wrote:

> 
>> Later in userspace a user program will contact the GO with the
>> attestation report (which measures host_data and the OVMF memory). If
>> the measurement is not what the GO expects, then it won't release the
>> secret (which should be necessary for the actual meaningful workload
>> performed in the guest).
>>
>>
>> This should mitigate the following attacks:
>>
>> 1. Rogue CSP replaces OVMF with a rogue-OVMF that doesn't actually check
>> the hashes (the GO won't release the secret due to wrong measurement in
>> attestation report).
>> 2. Rogue CSP uses "good" host_data content (kernel hash) but delivers
>> malicious kernel via fw_cfg (stopped by OVMF verifying the hashes).
>> 3. Rogue CSP uses malicious kernel and its hashes in host_data (the GO
>> won't release the secret due to wrong host_data in attestation report).
>>


-Dov