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. > > > > > 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? > > > > > 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, -- Erik Skultety
