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.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
