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

Wed Jan 23 15:29:25 UTC 2019

On Wed, Jan 23, 2019 at 03:02:28PM +0000, Singh, Brijesh wrote:
>
>
> On 1/23/19 7:36 AM, Daniel P. Berrangé wrote:
> > 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.
> >
>
> Agree its not safe to do 0644.
>
> Currently, anyone who has access to /dev/sev (read or write) will be
> able to execute SEV platform command. In other words there is no
> permission check per command basis. I must admit that while developing
> the driver I was under assumption that only root will ever access the
> /dev/sev device hence overlooked it. But now knowing that others may
> also need to access the /dev/sev, I can submit patch in kernel to do
> per command access control.
>
> Until then, can we follow Daniel's recommendation to elevate privilege
> of the probing code?

If that is the case, then I absolutely agree with Dan, I'll start working on
the patches for libvirt, I'm glad I learned that there's no strict relation
between ioctl ops and filesystem permissions unless explicitly defined.

Thanks,
Erik