Re: [libvirt] [PATCH] qemu: Copy SELinux labels for namespace too

On 01/13/2017 12:43 PM, Martin Kletzander wrote: > On Fri, Jan 13, 2017 at 11:12:43AM +0100, Michal Privoznik wrote: >> When creating new /dev/* for qemu, we do chown() and copy ACLs to >> create the exact copy from the original /dev. I though that >> copying SELinux labels is not necessary as SELinux will chose the >> sane defaults. Surprisingly, it does not leaving namespace with >> the following labels: >> >> crw-rw-rw-. root root system_u:object_r:tmpfs_t:s0 random >> crw-------. root root system_u:object_r:tmpfs_t:s0 rtc0 >> drwxrwxrwt. root root system_u:object_r:tmpfs_t:s0 shm >> crw-rw-rw-. root root system_u:object_r:tmpfs_t:s0 urandom >> >> As a result, domain is unable to start: >> >> error: internal error: process exited while connecting to monitor: >> Error in GnuTLS initialization: Failed to acquire random data. >> qemu-kvm: cannot initialize crypto: Unable to initialize GNUTLS >> library: Failed to acquire random data. >> >> The solution is to copy the SELinux labels as well. >> >> Reported-by: Andrea Bolognani <abologna(a)redhat.com&gt; >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/qemu/qemu_domain.c | 61 >> ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 61 insertions(+) >> >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index 1399dee0d..a29866673 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -63,6 +63,9 @@ >> #if defined(HAVE_SYS_MOUNT_H) >> # include <sys/mount.h> >> #endif >> +#ifdef WITH_SELINUX >> +# include <selinux/selinux.h> >> +#endif >> >> #include <libxml/xpathInternals.h> >> >> @@ -6958,6 +6961,9 @@ qemuDomainCreateDevice(const char *device, >> char *canonDevicePath = NULL; >> struct stat sb; >> int ret = -1; >> +#ifdef WITH_SELINUX >> + char *tcon = NULL; >> +#endif >> >> if (virFileResolveAllLinks(device, &canonDevicePath) < 0) { >> if (errno == ENOENT && allow_noent) { >> @@ -7023,10 +7029,34 @@ qemuDomainCreateDevice(const char *device, >> goto cleanup; >> } >> >> +#ifdef WITH_SELINUX >> + if (getfilecon_raw(canonDevicePath, &tcon) < 0 && >> + (errno != ENOTSUP && errno != ENODATA)) { >> + virReportSystemError(errno, >> + _("Unable to get SELinux label on %s"), >> canonDevicePath); >> + goto cleanup; >> + } >> + >> + if (tcon && >> + setfilecon_raw(devicePath, (VIR_SELINUX_CTX_CONST char *) >> tcon) < 0) { >> + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR >> + if (errno != EOPNOTSUPP && errno != ENOTSUP) { >> + VIR_WARNINGS_RESET >> + virReportSystemError(errno, >> + _("Unable to set SELinux label on %s"), >> + devicePath); >> + goto cleanup; >> + } >> + } >> +#endif >> + > > I think, instead of all the ifdefs, this should be a security driver API > instead of being hardcoded in places. That way it will be processed > properly by all the security drivers. I don't think I see what you mean. Firstly, we want to set seclabels for some paths that are not touched by secdrivers at all (e.g. /dev/*random, /dev/kvm). Secondly, secdrivers should honour norelabel flag and be a no-op if that one is set. This would clash with sysadmins handling seclabels themselves. Thirdly, no secdriver of ours deals with ACLs. We have to in here.

Michal -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list