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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list