On Fri, Jan 13, 2017 at 03:47:16PM +0100, Michal Privoznik wrote:
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@xxxxxxxxxx> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> --- 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.
It might sound a bit weird to have a security API that does not honour relabel=no and does label an arbritrary file, but SELinux stuff should go into SELinux security driver. Otherwise this will be a mess. The only problem I see here is when the driver is disabled in the configuration file, it will not be triggered. But we might get around that. Just so you know, I'm not special-casing SELinux here. I think DAC-related stuff should go to DAC driver and so on. Just my 2 cents, though.
Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
