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

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(a)linux.ibm.com&gt; >>>>> >>>>> 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.

>>>> 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, > >