[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Set umask before calling mknod()



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]