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

[Erik Skultety]

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?

So, sorry for not coming back earlier, but I'm still fighting a permission
issue when opening the /dev/sev device and I honestly don't know what's
happening. If you apply the patch bellow (or attachment) and you run libvirtd
with LIBVIRT_LOG_OUTPUTS=1:file:<your_log_file> env, you should see something
like this in the logs:

warning : virExec:778 : INHERITABLE CAPS: 'dac_override'
warning : virExec:781 : EFFECTIVE CAPS: 'dac_override'
warning : virExec:784 : PERMITTED CAPS: 'dac_override'
warning : virExec:787 : BOUNDING CAPS: 'dac_override'

...and that is right before we issue execve. Yet, if you debug further into the
QEMU process right after execve, it doesn't report any capabilities at all, so
naturally it'll still get permission denied. Is there something I'm missing
here?

An alternative question I've been playing ever since we exchanged the last few
emails is that can't we wait until the ioctls are compared against permissions
in kernel so that upstream libvirt (and downstream too for that matter) doesn't
have to work around it and stick with that workaround for eternity?

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index f504db7d05..4d7ec2781a 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -53,6 +53,10 @@
 #include <stdarg.h>
 #include <sys/utsname.h>

+#if WITH_CAPNG
+# include <cap-ng.h>
+#endif
+
 #define VIR_FROM_THIS VIR_FROM_QEMU

 VIR_LOG_INIT("qemu.qemu_capabilities");
@@ -4521,6 +4525,12 @@ 
virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
                                     NULL);
     virCommandAddEnvPassCommon(cmd->cmd);
     virCommandClearCaps(cmd->cmd);
+#if WITH_CAPNG
+    /* QEMU might run into permission issues, e.g. /dev/sev (0600), overriding
+     * it just for probing is okay from security POV */
+    virCommandAllowCap(cmd->cmd, CAP_DAC_OVERRIDE);
+#endif
+
     virCommandSetGID(cmd->cmd, cmd->runGid);
     virCommandSetUID(cmd->cmd, cmd->runUid);

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index d965068369..6d49416704 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -771,6 +771,23 @@ virExec(virCommandPtr cmd)
             goto fork_error;
     }

+#if WITH_CAPNG
+    if (strstr(cmd->args[0], "qemu")) {
+        VIR_WARN("INHERITABLE CAPS: '%s'",
+                 capng_print_caps_text(CAPNG_PRINT_BUFFER,
+                                       CAPNG_INHERITABLE));
+        VIR_WARN("EFFECTIVE CAPS: '%s'",
+                 capng_print_caps_text(CAPNG_PRINT_BUFFER,
+                                       CAPNG_EFFECTIVE));
+        VIR_WARN("PERMITTED CAPS: '%s'",
+                 capng_print_caps_text(CAPNG_PRINT_BUFFER,
+                                       CAPNG_PERMITTED));
+        VIR_WARN("BOUNDING CAPS: '%s'",
+                 capng_print_caps_text(CAPNG_PRINT_BUFFER,
+                                       CAPNG_BOUNDING_SET));
+    }
+#endif
+
     /* Close logging again to ensure no FDs leak to child */
     virLogReset();



Thanks,
Erik

dac_override.patch
Description: Text document