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