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

[Daniel P . Berrangé]

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.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|