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

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