[PATCH 3/6] qemu: check if AMD secure guest support is enabled

[Brijesh Singh]

On 5/13/20 5:40 AM, Boris Fiuczynski wrote:
> On 5/12/20 7:52 PM, Brijesh Singh wrote:
>>
>> On 5/12/20 10:32 AM, Erik Skultety wrote:
>>> On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
>>>> On 5/11/20 9:40 PM, Brijesh Singh wrote:
>>>>> Thanks for the patch Paulo, Few comments.
>>>>>
>>>>> On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
>>>>>> From: Paulo de Rezende Pinatti <ppinatti at linux.ibm.com>
>>>>>>
>>>>>> Implement secure guest check for AMD SEV (Secure Encrypted
>>>>>> Virtualization) in order to invalidate the qemu capabilities
>>>>>> cache in case the availability of the feature changed.
>>>>>>
>>>>>> For AMD SEV the verification consists of:
>>>>>> - checking if /sys/module/kvm_amd/parameters/sev contains the
>>>>>> value '1': meaning SEV is enabled in the host kernel;
>>>>>> - checking if the kernel cmdline contains 'mem_encrypt=on': meaning
>>>>>> SME memory encryption feature on the host is enabled
>>>>>
>>>>> In addition to the kernel module parameter, we probably also need to
>>>>> check whether QEMU supports the feature. e.g, what if user has newer
>>>>> kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12.  To
>>>>> check the
>>>>> SEV feature support, we should verify the following conditions:
>>>>>
>>>>> 1) check kernel module parameter is set
>>>> Paulo implemented the checks following the documentation in
>>>> docs/kbase/launch_security_sev.rst. The check for the module
>>>> parameter sev
>>>> is included. Is the check for the kernel cmdline parameter
>>>> "mem_encrypt" not
>>>> necessary? Or would that be covered by your suggested check in 2)?
>>> Brijesh correct me here please, but IIRC having mem_encrypt set is
>>> merely
>>> a good security recommendation but is orthogonal with kvm.amd_sev
>>> feature, IOW
>>> SEV should work without SME. If my memory serves well here, we don't
>>> need
>>> and also should not parse the kernel cmdline in this case.
>>>
>>
>> Yes, that is correct.  mem_encrypt=on is not required for the SEV to
>> work. The mem_encrypt=on option is meant to enable the SME feature on
>> the host machine. In addition to the guest, if customer wants to protect
>> the Hypervsior memory from physical attacks then they can use SME or
>> TSME.  The SME can be enabled using mem_encrypt=on, whereas the TSME can
>> be enabled in the BIOS and does not require any kernel changes. AMD is
>> mostly recommending TSME to protect against the physical attacks.
>>
>>
>>>>> 2) check if /dev/sev device exist (aka firmware is detected)
>>>> This seems reasonable. Shouldn't it have been documented in
>>>> docs/kbase/launch_security_sev.rst?
>>> Sure, we can add a mention about this. Although, doesn't 1 imply 2?
>>> IOW can
>>> you have the kernel module parameter set to 1 and yet kernel doesn't
>>> expose the
>>> /dev/sev node?
>>
>>
>> Currently, 1 does not imply 2, KVM driver does not initialize the
>> firmware during the feature probe (i.e does not access the /dev/sev).
>> The firmware initialization is delayed until the first guest launch. So
>> only sane way to know whether firmware is been detected is check the
>> existence of the /dev/sev or issue a query-sev command . The query-sev
>> command will send the platform_status request to the firmware, if the
>> firmware is not ready then this command will fail.
>>
>>
>
> Just to summarize the checks you want implemented for AMD SEV support:
> 1) check kernel module parameter is set (as already implemented)
> 2) check if /dev/sev device exist
>
> Please note that these checks will also go into virt-host-validate in
> patch 5.
>

Sounds good to me, thanks.


>
>
>>>>> 3) Check if Qemu supports SEV feature (maybe we can detect the
>>>>> existence
>>>>> of the query-sev QMP command or detect Qemu version >= 2.12)
>>> ^This is already achieved by qemuMonitorJSONGetSEVCapabilities
>>>
>>>> The idea is to check the host capabilities to detect if qemus
>>>> capabilities
>>>> need to be reprobed. Therefore this would be a result if checks 1+2
>>>> change
>>>> but it would not be a cache invalidation trigger.
>>> I agree in the sense that the SEV support that is currently being
>>> reported in
>>> QEMU capabilities wasn't a good choice because it reports only
>>> platform data
>>> which are constant to the host and have nothing to do with QEMU. It
>>> would be
>>> okay to just say <sev supported="yes|no"/> in qemu capabilities and
>>> report the
>>> rest in host capabilities. I haven't added this info into host
>>> capabilities
>>> when I realized the mistake because I didn't want to duplicate the
>>> platform
>>> info on 2 places.  For the IBM secure guest feature, it makes total
>>> sense to
>>> report support within host capabilities, I'm just not sure whether
>>> we should do
>>> the same for SEV and try really hard to "fix" the mess.
>>>
>>> Regards,
>>
>>
>
>