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

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.

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