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

[Michal Privoznik]

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 at redhat.com>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> 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