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. 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 :|
