[libvirt] New QEMU daemon for persistent reservations

[Daniel P. Berrange]

On Mon, Nov 27, 2017 at 01:49:41PM +0100, Paolo Bonzini wrote:
> On 27/11/2017 13:18, Daniel P. Berrange wrote:
> > On Mon, Nov 27, 2017 at 12:51:33PM +0100, Paolo Bonzini wrote:
> >> On 27/11/2017 12:37, Daniel P. Berrange wrote:
> >>> On Mon, Nov 27, 2017 at 12:13:24PM +0100, Paolo Bonzini wrote:
> >>>> Hm, I see what you mean now.  But it would be "just" a qemu-pr-helper
> >>>> bug that it trusts the caller to have "ioctl" permissions on the file
> >>>> descriptor, wouldn't it?
> >>>>
> >>>> And it could be a feature even, since the remote QEMU process also has
> >>>> to have "connect" permissions on the qemu-pr-helper socket.  So you
> >>>> could give it ioctl access *limited to persistent reservations* by
> >>>> granting the appropriate permissions on the socket.
> >>>
> >>> We can't grant access to the persistent reservation helper's socket on a
> >>> per QEMU basis. Permissions are granted on the domain type svirt_t, and
> >>> we don't want to invent a new domain type just for having access to the
> >>> PR helper.
> >>
> >> You can do so via DAC and MAC on the socket itself, or is that not enough?
> > 
> > Yes, you can do MAC on the socket, but that merely gives you protection
> > betweeen the host & guest. The goal of sVirt is to give protection
> > between VMs, and MAC on the socket alone doesn't guarantee that.
> > In the shared-PR helper the PR helper context would not have an
> > MAC level applied, so the MAC check is now
> > 
> >    process context == qemu_pr_helper_t:s0
> >    file context == svirt_image_t:s0,c340,c524
> > 
> > This is still a MAC check, but it is a weaker check than what sVirt promises,
> > because a QEMU with MCS   c340,c524 could pass a file descriptor with a MCS
> > level c123,c422.  QEMU would be denied to use this bogus FD, but the PR
> > helper would happily allow this bad access.
> 
> So the issue could be that QEMU could have received this bogus FD from
> an attacker.  Got it.
> 
> > So to satisfy sVirt separation of QEMU instances, we need the PR helper to
> > be able to validate the MCS levels, as the kernel no longer has enough info
> > todo this automatically. 
> > 
> > In the PR helper this is achieved with something like
> > 
> >     getpeercon(socketfd, &qemucon)  => returns svirt_t:s0,c340,c524
> >     fgetfilecont(filefd, &diskcon)  => returns svirt_image_t:s0,c340,c524
> > 
> > Then it would call something like
> > 
> >    security_compute_av(qemucon, diskcon, ...file class..., ...ioctl perm...)
> > 
> > to get a MAC decision from the kernel. This preserves the sVirt isolation
> > of individual QEMU instances.
> 
> Got it.  My problem here is that ioctl permission might be too strict.
> One use case for the helper is to bypass the ioctl permission, and only
> grant SCSI passthrough access for the specific case of persistent
> reservation commands.  Would it make sense to also allow e.g. "lock"
> (and would it pass the SELinux policy)?

When I write "...ioctl perm..." I use that just as a short cut to represent
whatever SELinux access vector would be checked if QEMU were to do the SCSI
PR calls itself.  The access vector permission bits are defined in the policy
file, and in the auto-generated header file /usr/include/selinux/av_permissions.h

AFAICT, there's only a coarse COMMON_FILE_IOCTL access vector defined, nothing
on the level of individual ioctl commands. So, yes, the MAC policy check
would allow other ioctl commands to be run, aside from those required for
persistent reservations. We just have to rely on the PR helper code for
that restriction.

> Also, what about AppArmor?

I don't think it has a way to do the delegated permission checks in the
way I describe for SELinux. 

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