[libvirt] [PATCH] qemuProcessStart: Be tolerant to relabel errors for session mode
https://bugzilla.redhat.com/show_bug.cgi?id=1124841 When the daemon is running under unprivileged user, that is under qemu:///session, there are plenty of operations we can't do. What we can do is to go with best effort. One of such cases is relabeling domain resources (be it disks, sockets, regular files, etc.) during domain startup process. While we may successfully set DAC labels, we can be fairly certain that any attempt to change SELinux labels will fail. Therefore we should tolerate relabelling errors and just let qemu to try access the resources. If it fails, our error reporting system is strong enough to articulate the exact error to the user anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1c0c734..58ed631 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4856,8 +4856,13 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, - vm->def, stdin_path) < 0) - goto cleanup; + vm->def, stdin_path) < 0) { + /* Be tolerant to relabel errors if we are running unprivileged. */ + if (virQEMUDriverIsPrivileged(driver)) + goto cleanup; + else + VIR_DEBUG("Ignoring relabel errors for unprivileged daemon"); + } /* Security manager labeled all devices, therefore * if any operation from now on fails and we goto cleanup, -- 2.3.6
On 07/15/2015 09:02 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1124841
When the daemon is running under unprivileged user, that is under qemu:///session, there are plenty of operations we can't do. What we can do is to go with best effort. One of such cases is relabeling domain resources (be it disks, sockets, regular files, etc.) during domain startup process. While we may successfully set DAC labels, we can be fairly certain that any attempt to change SELinux labels will fail. Therefore we should tolerate relabelling errors and just let qemu to try access the resources. If it fails, our error reporting system is strong enough to articulate the exact error to the user anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1c0c734..58ed631 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4856,8 +4856,13 @@ int qemuProcessStart(virConnectPtr conn,
VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, - vm->def, stdin_path) < 0) - goto cleanup; + vm->def, stdin_path) < 0) { + /* Be tolerant to relabel errors if we are running unprivileged. */ + if (virQEMUDriverIsPrivileged(driver)) + goto cleanup; + else + VIR_DEBUG("Ignoring relabel errors for unprivileged daemon");
How about just if (cond) goto VIR_DEBUG(or WARN) virResetLastError() Otherwise, seems reasonable in principal, so ACK John
+ }
/* Security manager labeled all devices, therefore * if any operation from now on fails and we goto cleanup,
On Wed, Jul 15, 2015 at 03:02:13PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1124841
When the daemon is running under unprivileged user, that is under qemu:///session, there are plenty of operations we can't do. What we can do is to go with best effort. One of such cases is relabeling domain resources (be it disks, sockets, regular files, etc.) during domain startup process. While we may successfully set DAC labels, we can be fairly certain that any attempt to change SELinux labels will fail. Therefore we should tolerate relabelling errors and just let qemu to try access the resources. If it fails, our error reporting system is strong enough to articulate the exact error to the user anyway.
Errr, isn't it entirely the opposite to what you say. Running as an unprivileged user ID has no bearing on whether you are allowed to set SELinux labels. If the user acount is unconfined_t it can set any SELinux labels it wants. It will only fail if the libvird process is confined in some way. IMHO we shold not be ignoring such failures. What *will* fail is any attempt to set DAC labels, since you need CAP_CHOWN capability, but we shouldn't have the DAC security maanger running when in session mode.
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1c0c734..58ed631 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4856,8 +4856,13 @@ int qemuProcessStart(virConnectPtr conn,
VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, - vm->def, stdin_path) < 0) - goto cleanup; + vm->def, stdin_path) < 0) { + /* Be tolerant to relabel errors if we are running unprivileged. */ + if (virQEMUDriverIsPrivileged(driver)) + goto cleanup; + else + VIR_DEBUG("Ignoring relabel errors for unprivileged daemon"); + }
I really don't think we should do this here as it affects all security managers. What is the failure you are actually seeing without this ? SElinux label changes should be succeeding in session mode and we should not even be applying DAC labels Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 20.07.2015 15:50, Daniel P. Berrange wrote:
On Wed, Jul 15, 2015 at 03:02:13PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1124841
When the daemon is running under unprivileged user, that is under qemu:///session, there are plenty of operations we can't do. What we can do is to go with best effort. One of such cases is relabeling domain resources (be it disks, sockets, regular files, etc.) during domain startup process. While we may successfully set DAC labels, we can be fairly certain that any attempt to change SELinux labels will fail. Therefore we should tolerate relabelling errors and just let qemu to try access the resources. If it fails, our error reporting system is strong enough to articulate the exact error to the user anyway.
Errr, isn't it entirely the opposite to what you say. Running as an unprivileged user ID has no bearing on whether you are allowed to set SELinux labels. If the user acount is unconfined_t it can set any SELinux labels it wants. It will only fail if the libvird process is confined in some way. IMHO we shold not be ignoring such failures.
What *will* fail is any attempt to set DAC labels, since you need CAP_CHOWN capability, but we shouldn't have the DAC security maanger running when in session mode.
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1c0c734..58ed631 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4856,8 +4856,13 @@ int qemuProcessStart(virConnectPtr conn,
VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, - vm->def, stdin_path) < 0) - goto cleanup; + vm->def, stdin_path) < 0) { + /* Be tolerant to relabel errors if we are running unprivileged. */ + if (virQEMUDriverIsPrivileged(driver)) + goto cleanup; + else + VIR_DEBUG("Ignoring relabel errors for unprivileged daemon"); + }
I really don't think we should do this here as it affects all security managers.
What is the failure you are actually seeing without this ? SElinux label changes should be succeeding in session mode and we should not even be applying DAC labels
Unable to complete install: 'unable to set security context 'system_u:object_r:virt_content_t:s0' on '/usr/share/virtio-win/virtio-win.iso': Operation not permitted' Traceback (most recent call last): File "/usr/share/virt-manager/virtManager/asyncjob.py", line 89, in cb_wrapper callback(asyncjob, *args, **kwargs) File "/usr/share/virt-manager/virtManager/create.py", line 1873, in do_install guest.start_install(meter=meter) File "/usr/share/virt-manager/virtinst/guest.py", line 417, in start_install noboot) File "/usr/share/virt-manager/virtinst/guest.py", line 481, in _create_guest dom = self.conn.createLinux(start_xml or final_xml, 0) File "/usr/lib64/python2.7/site-packages/libvirt.py", line 3585, in createLinux if ret is None:raise libvirtError('virDomainCreateLinux() failed', conn=self) libvirtError: unable to set security context 'system_u:object_r:virt_content_t:s0' on '/usr/share/virtio-win/virtio-win.iso': Operation not permitted # ls -lZ /usr/share/virtio-win/virtio-win.iso lrwxrwxrwx. root root system_u:object_r:usr_t:s0 /usr/share/virtio-win/virtio-win.iso -> virtio-win-1.7.4.iso # ls -lZ /usr/share/virtio-win/virtio-win-1.7.4.iso -rw-r--r--. root root system_u:object_r:usr_t:s0 /usr/share/virtio-win/virtio-win-1.7.4.iso So maybe we should ignore just selinux labelling errors when running as unprivileged? Michal
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Michal Privoznik