Re: [libvirt] AMD SEV's /dev/sev permissions and probing QEMU for capabilities

On Wed, Jan 23, 2019 at 02:33:01PM +0100, Erik Skultety wrote: > On Wed, Jan 23, 2019 at 01:24:13PM +0000, Daniel P. Berrangé wrote: >> On Wed, Jan 23, 2019 at 02:22:12PM +0100, Erik Skultety wrote: >>> On Wed, Jan 23, 2019 at 01:10:42PM +0000, Daniel P. Berrangé wrote: >>>> On Wed, Jan 23, 2019 at 01:55:06PM +0100, Erik Skultety wrote: >>>>> On Fri, Jan 18, 2019 at 12:51:50PM +0000, Singh, Brijesh wrote: >>>>>> >>>>>> On 1/18/19 3:39 AM, Erik Skultety wrote: >>>>>>> Hi, >>>>>>> this is a summary of a private discussion I've had with guys CC'd on this email >>>>>>> about finding a solution to [1] - basically, the default permissions on >>>>>>> /dev/sev (below) make it impossible to query for SEV platform capabilities, >>>>>>> since by default we run QEMU as qemu:qemu when probing for capabilities. It's >>>>>>> worth noting is that this is only relevant to probing, since for a proper QEMU >>>>>>> VM we create a mount namespace for the process and chown all the nodes (needs a >>>>>>> SEV fix though). >>>>>>> >>>>>>> # ll /dev/sev >>>>>>> crw-------. 1 root root >>>>>>> >>>>>>> I suggested either force running QEMU as root for probing (despite the obvious >>>>>>> security implications) or using namespaces for probing too. Dan argued that >>>>>>> this would have a significant perf impact and suggested we ask systemd to add a >>>>>>> global udev rule. >>>>>>> >>>>>>> I proceeded with cloning [1] to systemd and creating an udev rule that I planned >>>>>>> on submitting to systemd upstream - the initial idea was to mimic /dev/kvm and >>>>>>> make it world accessible to which Brijesh from AMD expressed a concern that >>>>>>> regular users might deplete the resources (limit on the number of guests >>>>>>> allowed by the platform). >>>>>> >>>>>> >>>>>> During private discussion I didn't realized that we are discussing a >>>>>> probe issue hence things I have said earlier may not be applicable >>>>>> during the probe. The /dev/sev is managed by the CCP (aka PSP) driver. >>>>>> The /dev/sev is used for communicating with the SEV FW running inside >>>>>> the PSP. The SEV FW offers platform and guest specific services. The >>>>>> guest specific services are used during the guest launch, these services >>>>>> are available through KVM driver only. Whereas the platform services can >>>>>> be invoked at anytime. A typical platform specific services are: >>>>>> >>>>>> - importing certificates >>>>>> >>>>>> - exporting certificates >>>>>> >>>>>> - querying the SEV FW version etc etc >>>>>> >>>>>> In case of the probe we are not launch SEV guest hence we should not be >>>>>> worried about depleting the SEV ASID resources. >>>>>> >>>>>> IIRC, libvirt uses QEMP query-sev-capabilities to probe the SEV support. >>>>>> QEMU executes the below sequence to complete the request: >>>>>> >>>>>> 1. Exports the platform certificates  (this is when /dev/sev is accessed). >>>>>> >>>>>> 2. Read the host MSR to determine the C-bit and reduced phys-bit position >>>>>> >>>>>> I don't see any reason why we can't give world a 'read' permission to >>>>>> /dev/sev. Anyone should be able to export the certificates and query >>>>> >>>>> Okay, makes sense to me. The problem I see is the sev_platform_ioctl function >>>>> in QEMU which makes an _IOWR request, therefore the file descriptor being >>>>> opened in sev_get_capabilities is O_RDWR. Now, I only understand ioctl from >>>>> what I've read in the man page, so I don't quite understand the need for IOWR >>>>> here - but my honest guess would be that it's because the commands like >>>>> SEV_PDH_CERT_EXPORT or SEV_PLATFORM_STATUS need to be copied from userspace to >>>>> kernel to instruct kernel which services we want, ergo _IOWR, is that right? >>>> >>>> I'm not seeing any permissions checks in the sev_ioctl() function in the >>>> kernel, so IIUC, that means any permissions are entirely based on whether >>>> you can open the /dev/sev, once open you can run any ioctl. What, if anything, >>>> enforces which ioctls you can run when the device is only O_RDONLY vs O_RDWR ? >>> >>> I don't know, that's why I'm asking, because the manual didn't make it any >>> clear for me whether there's a connection between the device permissions and >>> ioctls that you're allowed to run. >>> >>>> >>>>> In any case, a fix of some sort needs to land in QEMU first, because no udev >>>>> rule would fix the current situation. Afterwards, I expect that having a rule >>>>> like this: >>>>> >>>>> KERNEL=="sev", GROUP="kvm", MODE="0644" >>>>> >>>>> and a selinux policy rule adding the kvm_device_t label, we should be fine, do >>>>> we agree on that? >>>> >>>> Based on what I think I see above, this looks like a bad idea. >>>> >>>> It still looks like we can solve this entirely in libvirt by just giving >>>> the libvirt capabilities probing code CAP_DAC_OVERRIDE. This would make >>>> libvirt work for all currently released SEV support in kernel/qemu. >>> >>> Sure we can, but that would make libvirt the only legitimate user of /dev/sev >>> and everything else would require the admin to change the permissions >>> explicitly so that other apps could at least retrieve the platform info, if >>> it was intended to be for public use? >>> Additionally, we'll still get shot down by SELinux because svirt_t wouldn't be >>> able to access /dev/sev by default. >> >> That's separate form probing and just needs SELinux policy to define >> a new sev_device_t type and grant svirt_t access to it. > > I know, I misread "we can solve this entirely in libvirt" then, I thought you > the SELinux part was included in the statement, my bad then. Still, back to the > original issue, we could technically do both, libvirt would have run qemu with > CAP_DAC_OVERRIDE and we keep working with everything's been released for > SEV in kernel/qemu and for everyone else, systemd might add 0644 for /dev/sev, > that way, everyone's happy, not that I'd be a fan of libvirt often having > to work around something because projects underneath wouldn't backport fixes to > all the distros we support, thus leaving the dirty work to us. Setting 0644 for /dev/sev looks unsafe to me unless someone can show where the permissions checks take place for the many ioctls that /dev/sev allows, such that only SEV_PDH_CERT_EXPORT or SEV_PLATFORM_STATUS is allowed when /dev/sev is opened by a user who doesn't have write permissions.