On 22/08/2017 18:27, Paolo Bonzini wrote: > Hi all,

> > I am adding a new daemon to QEMU, that QEMU can connect to in order to > issue persistent reservation commands.

> > The daemon can only issue the commands on file descriptor that QEMU > already has. In addition normal users shouldn't have access to the > daemon's Unix socket in /run, so the daemon is protected against misuse. > > My question is what is the best way to handle the connection to the > daemon socket. Currently, the path to the socket is passed to QEMU on > the command line: > > -object pr-manager-helper,id=mgr,path=/run/qemu-pr-helper.sock \ > -drive if=none,id=hd,driver=raw,filename=/dev/sdb,file.pr-manager=mgr \ > -device scsi-block,drive=hd > > (the new parts are "-object pr-manager-helper" and "file.pr-manager"). > > I could just make it root:root and pass a file descriptor from libvirt > to QEMU, but this would make it impossible for QEMU to reconnect to the > daemon in case someone does a "systemctl restart" or even just kills it > inadvertently. The daemon is stateless, so transparent reconnection > would be a nice feature to have. > > The alternative is to somehow label the daemon socket so that it can be > accessed by QEMU, but I'm not very well versed in SELinux. Thinking more about it, Libvirt could start the daemon on its own, assigning a socket per virtual machine. SELinux MCS should then just work, because the same category is assigned to the daemon instance and QEMU.

In particular, Libvirt could create the socket itself, label it, and pass it to the daemon through the systemd socket activation protocol (which qemu-pr-helper supports).

The XML to use the helper with a predefined socket could be: <disk ...> <pr mode='connect'>/path/to/unix.socket'</pr> </disk>

while to use it with a dedicated daemon <disk ...> <pr mode='private'>/path/to/qemu-pr-helper</pr> </disk>

The daemon can then be placed in the same devices cgroup and SELinux MCS category as QEMU.

Ah, this breaks my design. I guess <disk> <pr> <source type='unix' path='/path/to/qemu-pr-helper' mode='server'/> </pr> </disk> is pure madness, isn't it?

On Tue, Oct 03, 2017 at 06:07:53PM +0200, Paolo Bonzini wrote: > Yes, but OTOH if libvirtd starts the daemon, nobody cares about the > source type, so perhaps > > <pr driver='helper' mode='shared'> > <source ... /> > </pr> > > (mandatory source) vs. > > <pr driver='helper' mode='private'> > <path>/path/to/qemu-pr-helper</path> > </pr> > > (optional path, default from global configuration) vs. > > <pr driver='passthrough'/> > > (no helper)? I tend to think we should not expose the concept of a 'helper' at all in the XML. The fact that QEMU has a separate binary to do this is really an internal implementation detail due to the need for privilege separation/ elevation. Libvirt should just do the right thing running the helper with a suitable configuration when needed, just like we run the TAP device helper when needed.

IOW, just be as simple as an attribute "reservations=on|off" and we'll decide the UNIX socket path, daemon forking, etc ourselves Regards, Daniel

On Tue, Oct 03, 2017 at 06:35:03PM +0200, Paolo Bonzini wrote: > And later on we might have other ways to implement persistent > reservations in QEMU. So while I'm not a big fan(*) of the > driver='helper' moniker, I don't think an attribute is enough. Maybe > driver='external'?... Yes, if there's a choice of ways to manage reservations, we could reflect that as 'reservations=passthrough|emulated' or something like that. I just don't think the concept of a helper program should be visible in the XML, as it is an impl detail that is totally QEMU specific and could conceivably change eg not even needed with unpriv_sgio,

and if kernel were enhanced could be usable without needing a helper elsewhere too.

On Tue, Oct 03, 2017 at 06:53:56PM +0200, Paolo Bonzini wrote: > On 03/10/2017 18:39, Daniel P. Berrange wrote: >> On Tue, Oct 03, 2017 at 06:35:03PM +0200, Paolo Bonzini wrote: >>> And later on we might have other ways to implement persistent >>> reservations in QEMU. So while I'm not a big fan(*) of the >>> driver='helper' moniker, I don't think an attribute is enough. Maybe >>> driver='external'?... >> >> Yes, if there's a choice of ways to manage reservations, we could >> reflect that as 'reservations=passthrough|emulated' or something >> like that. >> >> I just don't think the concept of a helper program should be visible >> in the XML, as it is an impl detail that is totally QEMU specific and >> could conceivably change eg not even needed with unpriv_sgio, > > Not sure about that, the mpathpersist behavior is somewhat magic and I > am not really sure we should enable it by default, even with unpriv_sgio. > >> and if >> kernel were enhanced could be usable without needing a helper elsewhere >> too. > > It's an implementation detail for system mode, but not for user mode > (where ACLs on the socket are used to allow access to a privileged > operation). > > So: > > <reservations enable='yes'/> > > (uses helper from global configuration if not a libiscsi drive) vs. > > <reservations enable='yes'/> > <source ... /> > </pr> What do you mean by <source> here ? If that's the chardev source I would really prefer not to have that visible.

> > (for user mode) vs. > > <reservations enable='no'> > > (fails if libiscsi || CAP_SYS_RAWIO || unpriv_sgio)? Regards, Daniel

On Tue, Aug 22, 2017 at 06:27:40PM +0200, Paolo Bonzini wrote: > Hi all, > > I am adding a new daemon to QEMU, that QEMU can connect to in order to > issue persistent reservation commands. Can you elaborate on what this daemon does ? IIUC, by 'persistent reservation' you are referring to SCSI LUN reservations ? If so, the security model needs to involve more than just policy on the socket that QEMU uses to talk to the PR daemon. We need to be able to control what device nodes this daemon is permitted to access. For iSCSI backed disks, the daemon might need to use the libiscsi driver, instead of assuming there is a device node on the local host too.

Libvirt has a generic lock manager facility and I've previously though might be able to be extended to acquire SCSI reservations for disks which are backed by SCSI/iSCSI, but never tried to work on this.

I'm not sure if want to have QEMU spawning this daemon itself or not.

On the one hand if QEMU spawns the daemon, then it means the daemon inherits the SELinux policy of QEMU by default, so is restricted to only access the devices that QEMU has been granted.

> So the helper is a bit different from QEMU with respect to both SELinux > MCS labeling and the devices cgroup. Access limitation comes from only > ever operating on devices that have been passed on the socket. SELinux > MCS could be used so that only the "right" QEMU can connect to each > instance of the helper, though. I wonder how this interacts with SELinux. IIUC if we grant access to the multipath device file, the kernel won't normally require us to grant access the underlying paths. I/O is just allowed because they are a member of the mpath device. Hopefully this applies to the persistent reservations too ?

On Mon, Sep 11, 2017 at 01:44:59PM +0200, Paolo Bonzini wrote: > On 11/09/2017 12:46, Daniel P. Berrange wrote: >>> So the helper is a bit different from QEMU with respect to both SELinux >>> MCS labeling and the devices cgroup. Access limitation comes from only >>> ever operating on devices that have been passed on the socket. SELinux >>> MCS could be used so that only the "right" QEMU can connect to each >>> instance of the helper, though. >> I wonder how this interacts with SELinux. IIUC if we grant access to >> the multipath device file, the kernel won't normally require us to >> grant access the underlying paths. I/O is just allowed because they >> are a member of the mpath device. Hopefully this applies to the >> persistent reservations too ? > > No, persistent reservations are special; they have to be registered > independently on each path. (As I said, this was the original reason to > have a separate daemon: implementing it in QEMU would be not just a bad > idea because you need CAP_SYS_ADMIN, it would be impossible for > libvirt-started guests because of sVirt and device cgroups). > > So I think the helper daemon needs to be granted blanket ioctl access to > devices, without using either device cgroups or MCS. If it was a single helper daemon for all guests, that would be ok to be granted access to all devices. If we did that though, the daemon would have to be SELinux aware. ie, libvirt would have to talk to it and say that svirt_t:s0:c236,c660 is permitted /dev/mpath/foo, and it would have to validate the requests from the socket to QEMU to make sure that QEMU is not requesting access to other mpath devices.

If it was a one helper daemon per QEMU instance, then we would want to directly confine it to just the underlying devices associated with the mpath device allowed for that QEMU instance. This would imply that we needed to label the underlying devices with the MCS label to match the VM.

> This is already handled via SCM_RIGHTS and is part of the design of the > helper daemon. QEMU cannot even open mpath devices which are not > accessible according to its SELinux category or device cgroup. Ah so the daemon relies on the fact that the client was not permitted to open another file. So the only FD it can receive from the client is one that was associated with a permitted mpath device. That would be sufficient to protect against a malicious qemu process trying to explicitly pass it invalid FD data. It wouldn't be sufficient to be safe against a QEMU process that somehow managed to trigger a bug that caused it to corrupt memory and thus trick into opening a different file. For the latter we would need to have some stricter policy about the helper daemon and labelling on files.

> On the other hand, the daemon has CAP_SYS_RAWIO and CAP_SYS_ADMIN, so if > you get memory corruption all bets are probably off anyway. That's where the benefit of strict selinux labelling comes in. If we had strict labelling of the individual paths below the device, then even if the daemon got corrupted, the policy would prevent it from doing any damage to the system beyond calling ioctl() the individual paths it had been granted. It wouldn't be able to access devices associated with the host OS mounts, or other non-VM related or non-multipath related block devices.

On Mon, Sep 11, 2017 at 05:27:20PM +0200, Paolo Bonzini wrote: > On 11/09/2017 17:23, Daniel P. Berrange wrote: >>> On the other hand, the daemon has CAP_SYS_RAWIO and CAP_SYS_ADMIN, so if >>> you get memory corruption all bets are probably off anyway. >> That's where the benefit of strict selinux labelling comes in. If we had >> strict labelling of the individual paths below the device, then even if >> the daemon got corrupted, the policy would prevent it from doing any >> damage to the system beyond calling ioctl() the individual paths it had >> been granted. It wouldn't be able to access devices associated with >> the host OS mounts, or other non-VM related or non-multipath related >> block devices. > > Sure, but those capabilities let you do a lot of nasty things > indirectly, even within the constraints of the SELinux policy. > > For example, if you are able to reconfigure device mapper, you can > convince the kernel to write to any block device---even if you cannot > open it. IDWEFAL (I don't write exploits for a living) but I'm sure > that's just scraping the surface. Surely we would not write an SELinux policy that allows this daemon to reconfigure device mapper. IIUC, all this daemon should need is the ability to request persistent reservations on the individual paths associated with the mpath device. Is it not possible to write a SElinux policy which allows that, without also allowing reconfiguration of device mapper.

On Mon, Sep 11, 2017 at 05:47:28PM +0200, Paolo Bonzini wrote: > On 11/09/2017 17:33, Daniel P. Berrange wrote: >> On Mon, Sep 11, 2017 at 05:27:20PM +0200, Paolo Bonzini wrote: >>> On 11/09/2017 17:23, Daniel P. Berrange wrote: >>>>> On the other hand, the daemon has CAP_SYS_RAWIO and CAP_SYS_ADMIN, so if >>>>> you get memory corruption all bets are probably off anyway. >>>> That's where the benefit of strict selinux labelling comes in. If we had >>>> strict labelling of the individual paths below the device, then even if >>>> the daemon got corrupted, the policy would prevent it from doing any >>>> damage to the system beyond calling ioctl() the individual paths it had >>>> been granted. It wouldn't be able to access devices associated with >>>> the host OS mounts, or other non-VM related or non-multipath related >>>> block devices. >>> >>> Sure, but those capabilities let you do a lot of nasty things >>> indirectly, even within the constraints of the SELinux policy. >>> >>> For example, if you are able to reconfigure device mapper, you can >>> convince the kernel to write to any block device---even if you cannot >>> open it. IDWEFAL (I don't write exploits for a living) but I'm sure >>> that's just scraping the surface. >> >> Surely we would not write an SELinux policy that allows this daemon >> to reconfigure device mapper. >> >> IIUC, all this daemon should need is the ability to request persistent >> reservations on the individual paths associated with the mpath device. >> >> Is it not possible to write a SElinux policy which allows that, without >> also allowing reconfiguration of device mapper. > > As far as I know, querying and reconfiguring the device mapper are both > done with ioctls on /dev/mapper/control, and both require CAP_SYS_ADMIN. > > Maybe future versions of Linux could change it to require CAP_SYS_ADMIN > only for reconfiguration, so that the PR helper daemon does not require > the capability anymore. However, that would be independent from > SELinux, which only controls "ioctl" access without finer-grain choice > of which ioctls to allow. I don't suppose that the LUNS behind a mpath device end up being surface in /sys/block anywhere do they ?

> I understand that you want to protect in depth, but unfortunately this > only works if all layers are aware of SELinux. Luckily the daemon is > much, much smaller than QEMU, and so is the attack surface. It would be ok if we think its possible to lock it down later (once any needed kernel enhancements are developed), without having to change the interaction between QEMU / libvirt / helper daemon. I'm beginning to feel that is ok.

On 08/22/2017 06:27 PM, Paolo Bonzini wrote: > Hi all, > Sorry for resurrecting old thread but seems like there was no agreement reached. We don't want to expose any paths because the fact that PR helper is a separate binary that uses a UNIX socket to talk to qemu is a implementation detail of qemu. Other HVs may have it differently. However, we want to be able to turn it on/off or not mention it at all on per-disk basis. So what has been suggested so far is: <disk type='block' device='disk'> <source dev='/dev/sda'> <target dev='sda' bus='scsi'/> <reservations enable='yes'/> </disk> for privileged qemu, then: <disk type='block' device='disk'> <source dev='/dev/sda'> <target dev='sda' bus='scsi'/> <reservations enable='yes'> <source type='unix' path='/path/to/a/socket' mode='server'/> <reservations> </disk> for unprivileged qemu, or: <disk type='block' device='disk'> <source dev='/dev/sda'> <target dev='sda' bus='scsi'/> <reservations enable='no'/> </disk> for PR feature turned off (equivalent to leaving it out completely). Now, my question is, in the first case - how should libvirt chose the path? Should it be different for each disk/domain? How is the daemon started in the first place - should libvirt start it? And when should libvirt kill it?

On 24/11/2017 15:52, Daniel P. Berrange wrote: >> So what has been suggested so far is: >> >> <disk type='block' device='disk'> >> <source dev='/dev/sda'> >> <target dev='sda' bus='scsi'/> >> <reservations enable='yes'/> >> </disk> <reservations> without an inner <source> element leaves libvirtd with the choice of a daemon per QEMU, or a daemon per host in a well-known location. Unprivileged libvirtd would always use the latter; for privileged libvirtd it is implementation-defined. I like it. <reservations> with an inner <source> always gives a daemon per host in a custom location. It can be used by either unprivileged or privileged libvirtd. >> Now, my question is, in the first case - how should libvirt chose the >> path? Should it be different for each disk/domain? How is the daemon >> started in the first place - should libvirt start it? And when should >> libvirt kill it? > > The core question is one daemon per QEMU, or one daemon per host. I'd be > more inclined to have one daemon per QEMU so we always have isolation > and thus do't have to worry about a shared daemon being a potential > attack point between distinct QEMU's. One daemon per QEMU is nicer IMO because it lets us do MCS. Of course one daemon per QEMU can only apply to system libvirtd; session must use one daemon per host.

One daemon per host is easy, because it's just a couple new command-line options as far as libvirtd is concerned, but we need to check that it works well with MCS.

> If one daemon per host, then for privileged libvirtd, we should make sure > the daemon ships with a systemd unit file + socket activation file, then > we have a well-known cross-distro standardized socket path. Ok, then I will send a patch for upstream QEMU that adds the Fedora systemd unit files to contrib/. They are useful anyway.

On 24/11/2017 18:07, Michal Privoznik wrote: > On 11/24/2017 04:42 PM, Paolo Bonzini wrote: >> One daemon per QEMU is nicer IMO because it lets us do MCS. Of course >> one daemon per QEMU can only apply to system libvirtd; session must use >> one daemon per host. > > Agreed. One daemon per QEMU it is then. Just to make sure whether I> understand correctly - libvirtd would start it *before* exec()-ing qemu > (so that qemu can connect) and kill it after qemu dies. But what should > happen if the helper dies while qemu is running? Should qemu pause > itself, fire up an event on the monitor so that libvirtd can spawn the > helper again? QEMU currently tries to reconnect five times every second, and then fails the I/O command. This seemed okay to me because PRs are generally used in a HA environment where the failure will be broadcast and another node will pick up the work. > The helper is going to be daemonized (for the same reason > that qemu process is) => there's no SIGCHLD for libvirtd to receive. Couldn't (or shouldn't!) libvirt register itself as a subreaper instead?

> Also, I don't really expect anything special when it comes to migration, > just want to make sure I'm not mislead. No, everything is fine for migration. >> One daemon per host is easy, because it's just a couple new command-line >> options as far as libvirtd is concerned, but we need to check that it >> works well with MCS. > > It's very likely that it wouldn't. Users can chose arbitrary DAC/SELinux > labels for their qemus. In general we will not find any intersection > that the helper socket can have. I know some of those words. :) Can you explain to an SELinux layman?

On 24/11/2017 19:13, Michal Privoznik wrote: > On 11/24/2017 06:18 PM, Paolo Bonzini wrote: >> On 24/11/2017 18:07, Michal Privoznik wrote: >>> The helper is going to be daemonized (for the same reason >>> that qemu process is) => there's no SIGCHLD for libvirtd to receive. >> >> Couldn't (or shouldn't!) libvirt register itself as a subreaper instead? > > Whether libvirt can't have a > separate thread where nothing waitpid() would run? That could hardly > work - not only would libvirt need to spawn separate thread for every > helper, it would not work across restarts of libvirtd. We would have no > guarantee that PID we recorded is still the same process as it was when > we were dying. Oh, you're right. :( Even if libvirtd were a subreaper (see prctl(2) manpage), the daemonized qemu-pr-helper would be reparented to init when libvirtd restarts. libvirtd could test whether the helper is alive by connecting to its socket. If that's not enough, I'll add an event.

Another thing: we have <reservations enable='yes'/> which represents "one daemon per QEMU" in privileged libvirtd and "well-known socket path" in session libvirtd. What would the live-domain XML look like? Maybe: <!-- one daemon per QEMU, privileged libvirtd --> <reservations enable='yes' managed='yes'/> <source type='unix' path='/var/lib/libvirtd/...' mode='server'/> </reservations> <!-- one daemon per host, session libvirtd --> <reservations enable='yes' managed='no'/> <source type='unix' path='/var/run/qemu-pr-helper.sock' mode='server'/> </reservations>

And then, would it be an error to specify the "wrong" managed setting in the domain-creation XML?

Or would it be possible to use a managed='no' helper in privileged libvirtd? I take it as a "no" given your remark below.

On 24/11/2017 15:52, Daniel P. Berrange wrote: >> So what has been suggested so far is: >> >> <disk type='block' device='disk'> >> <source dev='/dev/sda'> >> <target dev='sda' bus='scsi'/> >> <reservations enable='yes'/> >> </disk> <reservations> without an inner <source> element leaves libvirtd with the choice of a daemon per QEMU, or a daemon per host in a well-known location. Unprivileged libvirtd would always use the latter; for privileged libvirtd it is implementation-defined. I like it. <reservations> with an inner <source> always gives a daemon per host in a custom location. It can be used by either unprivileged or privileged libvirtd. >> Now, my question is, in the first case - how should libvirt chose the >> path? Should it be different for each disk/domain? How is the daemon >> started in the first place - should libvirt start it? And when should >> libvirt kill it? > > The core question is one daemon per QEMU, or one daemon per host. I'd be > more inclined to have one daemon per QEMU so we always have isolation > and thus do't have to worry about a shared daemon being a potential > attack point between distinct QEMU's. One daemon per QEMU is nicer IMO because it lets us do MCS. Of course one daemon per QEMU can only apply to system libvirtd; session must use one daemon per host. One daemon per host is easy, because it's just a couple new command-line options as far as libvirtd is concerned, but we need to check that it works well with MCS.

On 27/11/2017 10:40, Daniel P. Berrange wrote: > > If we had one daemon per QEMU, then we would give the daemon the same > MCS label as QEMU. The kernel will thus enforce this label matches the > label on the QEMU process when it connects to the UNIX socket. The kernel > will also validate the label on the disk file descriptor passed to the > daemon by QEMU. > > If we had one daemon per host, then that daemon will need a generic > label that lets all QEMUs connect to it. When QEMU passes in a disk > FD, the daemon will need to query the SELinux context of the remote > QEMU process, and then perform a userspace ACL check of that against > the FD that is passed in. > > The latter case means the QEMU helper will need to link to libselinux > and performs checks itself. Then it seems much better to use one daemon per QEMU, indeed.

On 27/11/2017 11:59, Daniel P. Berrange wrote: > On Mon, Nov 27, 2017 at 11:57:56AM +0100, Paolo Bonzini wrote: >> On 27/11/2017 10:40, Daniel P. Berrange wrote: >>> >>> If we had one daemon per QEMU, then we would give the daemon the same >>> MCS label as QEMU. The kernel will thus enforce this label matches the >>> label on the QEMU process when it connects to the UNIX socket. The kernel >>> will also validate the label on the disk file descriptor passed to the >>> daemon by QEMU. >>> >>> If we had one daemon per host, then that daemon will need a generic >>> label that lets all QEMUs connect to it. When QEMU passes in a disk >>> FD, the daemon will need to query the SELinux context of the remote >>> QEMU process, and then perform a userspace ACL check of that against >>> the FD that is passed in. >>> >>> The latter case means the QEMU helper will need to link to libselinux >>> and performs checks itself. >> >> Then it seems much better to use one daemon per QEMU, indeed. > > That would rule out using persistent reservations for unprivileged QEMU > though, because we still need sVirt protection for that. 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.

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?

In other words, what are the SELinux best practices when you don't want a process to have blanket access to a permission, but you may be fine with a subset of those? If you think of unpriv_sgio=0 as a very simple MAC, this is actually the very case that the PR helper is designed for.

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

Also, what about AppArmor?

On 27/11/2017 13:57, Daniel P. Berrange wrote: >> 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. But can you also test _more_ permissions if you want? So if QEMU passed to the helper a file for which it has "lock" but not "ioctl" access, would it make sense for the helper to let it through? Together with the socket MAC, this should be precise enough.

>> But can you also test _more_ permissions if you want? So if QEMU passed >> to the helper a file for which it has "lock" but not "ioctl" access, >> would it make sense for the helper to let it through? Together with the >> socket MAC, this should be precise enough. > Sure, you can check any of the permissions which are defined for the > type of object you've got. The goal is to check permissions which > correspond to actions you're taking on the object. So if you do > something other than just ioctl() on the passed in FD, it would make > sense to check further permissions as appropriate. Just to make sure I understand correctly: the PD passing is done by qemu and not libvirt, right? Technically, we don't open the disks.

On Fri, Nov 24, 2017 at 15:38:54 +0100, Michal Privoznik wrote: > On 08/22/2017 06:27 PM, Paolo Bonzini wrote: >> Hi all, >> > > Sorry for resurrecting old thread but seems like there was no agreement > reached. > > We don't want to expose any paths because the fact that PR helper is a > separate binary that uses a UNIX socket to talk to qemu is a > implementation detail of qemu. Other HVs may have it differently. > > However, we want to be able to turn it on/off or not mention it at all > on per-disk basis. So what has been suggested so far is: > > <disk type='block' device='disk'> > <source dev='/dev/sda'> > <target dev='sda' bus='scsi'/> > <reservations enable='yes'/> > </disk> Note that if the reservation is applied to the image and not the disk the element should become child of the <source> elememt.

Especially if there is only a ever so slight chance that it will work with formats supporting backing store (qcow2) and thus there would be a need to configure it per-image.