[libvirt] [PATCH] qemu: Set umask before calling mknod()
When we populate the private /dev that's going to be used by an isolated QEMU process, we take care all metadata matches what's in the top-level namespace: in particular, we copy the file permissions directly. However, since the permissions passed to mknod() are still affected by the active umask, we need to set it to a very permissive value before creating device nodes to avoid file access issues. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036 --- src/qemu/qemu_domain.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f62bf8f..7993acc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7040,6 +7040,7 @@ qemuDomainCreateDeviceRecursive(const char *device, #ifdef WITH_SELINUX char *tcon = NULL; #endif + mode_t oldUmask = umask((mode_t) 0); if (!ttl) { virReportSystemError(ELOOP, @@ -7205,6 +7206,7 @@ qemuDomainCreateDeviceRecursive(const char *device, #ifdef WITH_SELINUX freecon(tcon); #endif + umask(oldUmask); return ret; } @@ -7678,6 +7680,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, int ret = -1; bool delDevice = false; bool isLink = S_ISLNK(data->sb.st_mode); + mode_t oldUmask = umask((mode_t) 0); virSecurityManagerPostFork(data->driver->securityManager); @@ -7756,6 +7759,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, freecon(data->tcon); #endif virFileFreeACLs(&data->acl); + umask(oldUmask); return ret; } -- 2.7.4
On 02/13/2017 09:18 PM, Andrea Bolognani wrote:
When we populate the private /dev that's going to be used by an isolated QEMU process, we take care all metadata matches what's in the top-level namespace: in particular, we copy the file permissions directly.
However, since the permissions passed to mknod() are still affected by the active umask, we need to set it to a very permissive value before creating device nodes to avoid file access issues.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036 --- src/qemu/qemu_domain.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f62bf8f..7993acc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7040,6 +7040,7 @@ qemuDomainCreateDeviceRecursive(const char *device, #ifdef WITH_SELINUX char *tcon = NULL; #endif + mode_t oldUmask = umask((mode_t) 0);
if (!ttl) { virReportSystemError(ELOOP, @@ -7205,6 +7206,7 @@ qemuDomainCreateDeviceRecursive(const char *device, #ifdef WITH_SELINUX freecon(tcon); #endif + umask(oldUmask); return ret; }
There are couple of returns in between these two chunks so new umask will be leaked. Moreover, the current umask at this point should be 002 as a result of virCommandSetUmask() called from qemuProcessLaunch(). virCommandRun() then sets the umask also for the pre-exec hook. Does it make sense to run the pre-exec hook with unchanged umask? But as we are discussing in person right now, there is something fishy going on: on ppc and aarch64 the umask is honoured when calling mknod() but on x86_64 it is not. Maybe somebody on the list has a bright idea why that might be? Michal
On Tue, 2017-02-14 at 11:37 +0100, Michal Privoznik wrote:
@@ -7040,6 +7040,7 @@ qemuDomainCreateDeviceRecursive(const char *device, #ifdef WITH_SELINUX char *tcon = NULL; #endif + mode_t oldUmask = umask((mode_t) 0); if (!ttl) { virReportSystemError(ELOOP, @@ -7205,6 +7206,7 @@ qemuDomainCreateDeviceRecursive(const char *device, #ifdef WITH_SELINUX freecon(tcon); #endif + umask(oldUmask); return ret; } There are couple of returns in between these two chunks so new umask will be leaked.
Noted.
Moreover, the current umask at this point should be 002 as a result of virCommandSetUmask() called from qemuProcessLaunch(). virCommandRun() then sets the umask also for the pre-exec hook. Does it make sense to run the pre-exec hook with unchanged umask?
I think it would make sense, but then again, the default umask is probably going to be 022 or something like that, so in practice we'll still want to set it to 000 for this specific task (copying the top-level device nodes as-is).
But as we are discussing in person right now, there is something fishy going on: on ppc and aarch64 the umask is honoured when calling mknod() but on x86_64 it is not. Maybe somebody on the list has a bright idea why that might be?
Turns out mknod() honors the umask just fine, but something after device node creation changes the file permission and does so only on x86. More digging happening as we speak :) -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Michal Privoznik