[Qemu-devel] [PATCH 0/2] fix: unix sockets created for virtio-serail has insufficient permissions

[Qemu-devel] [PATCH 0/2] fix: unix sockets created for virtio-serail has insufficient permissions

[Chunyan Liu]

https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11
https://bugzilla.novell.com/show_bug.cgi?id=888166

To use virtio-serial device, unix socket created for chardev with
default umask(022) has insufficient permissions.
e.g.:
-device virtio-serial \
-chardev socket,path=/tmp/foo,server,nowait,id=foo \
-device virtserialport,chardev=foo,name=org.fedoraproject.port.0

#ls -l /tmp/somefile.sock
srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock

Other users in the same group (like real user, test engines, etc)
cannot write to this socket.

These patch series contains a qemu patch and a libvirt patch:
qemu patch: adds a new 'umask' option to -chardev, so that user can
    change the umask.
libvirt patch: pass 'umask=0x002' paramter to qemu command line
    for virtio-serial device

Chunyan Liu (2):
  qemu side: add 'umask' option to chardev
  libvirt side: qemu: add umask(002) to virtio-serial chardev commandline

-- 
1.8.4.5

[Qemu-devel] [PATCH 1/2] add 'umask' option to -chardev

[Chunyan Liu]

To use virtio-serial device, unix socket created for chardev with
default umask(022) has insufficient permissions.

e.g. start kvm guest with:
-device virtio-serial \
-chardev socket,path=/tmp/foo,server,nowait,id=foo \
-device virtserialport,chardev=foo,name=org.fedoraproject.port.0

Check permissions for the socket file that has been created in the host
to enable communication through virtual serial ports in the guest:
#ls -l /tmp/somefile.sock
srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock

Other users in the qemu group (like real user, test engines, etc) cannot
write to this socket.

Problem reported here:
https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11
https://bugzilla.novell.com/show_bug.cgi?id=888166

This patch tries to add a 'umask' option to 'chardev', so that user
can have chance to indicate a umask overwritting the default one (default
is 022), then create unix sockets with expected permissions.

Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
This is patch for qemu.

 qemu-char.c         |  3 +++
 qemu-options.hx     |  9 +++++++--
 util/qemu-sockets.c | 12 +++++++++++-
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index d4f327a..a39a5e4 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3856,6 +3856,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "chardev",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "umask",
+            .type = QEMU_OPT_NUMBER,
         },
         { /* end of list */ }
     },
diff --git a/qemu-options.hx b/qemu-options.hx
index ecd0e34..078e9db 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1929,7 +1929,7 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
     "-chardev null,id=id[,mux=on|off]\n"
     "-chardev socket,id=id[,host=host],port=port[,to=to][,ipv4][,ipv6][,nodelay]\n"
     "         [,server][,nowait][,telnet][,mux=on|off] (tcp)\n"
-    "-chardev socket,id=id,path=path[,server][,nowait][,telnet],[mux=on|off] (unix)\n"
+    "-chardev socket,id=id,path=path[,umask][,server][,nowait][,telnet],[mux=on|off] (unix)\n"
     "-chardev udp,id=id[,host=host],port=port[,localaddr=localaddr]\n"
     "         [,localport=localport][,ipv4][,ipv6][,mux=on|off]\n"
     "-chardev msmouse,id=id[,mux=on|off]\n"
@@ -2001,12 +2001,17 @@ Options to each backend are described below.
 A void device. This device will not emit any data, and will drop any data it
 receives. The null backend does not take any options.
 
-@item -chardev socket ,id=@var{id} [@var{TCP options} or @var{unix options}] [,server] [,nowait] [,telnet]
+@item -chardev socket ,id=@var{id} [@var{TCP options} or @var{unix options}] [,umask][,server] [,nowait] [,telnet]
 
 Create a two-way stream socket, which can be either a TCP or a unix socket. A
 unix socket will be created if @option{path} is specified. Behaviour is
 undefined if TCP options are specified for a unix socket.
 
+@option{umask} specifies the umask used for creating a unix socket. Without
+this option, default umask(022) will be used, permission is not sufficient
+for virtio-serial device. One can indicate umask=0x002 for virtio-serial
+device for correct usage.
+
 @option{server} specifies that the socket shall be a listening socket.
 
 @option{nowait} specifies that QEMU should not block waiting for a client to
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 5d38395..facf2c6 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -680,7 +680,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
 {
     struct sockaddr_un un;
     const char *path = qemu_opt_get(opts, "path");
-    int sock, fd;
+    int newmask = qemu_opt_get_number(opts, "umask", 0);
+    int sock, fd, oldmask;
 
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
@@ -708,10 +709,19 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
     }
 
     unlink(un.sun_path);
+    if (newmask) {
+        oldmask = umask(newmask);
+    }
     if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
+        if (newmask) {
+            umask(oldmask);
+        }
         error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
         goto err;
     }
+    if (newmask) {
+        umask(oldmask);
+    }
     if (listen(sock, 1) < 0) {
         error_set_errno(errp, errno, QERR_SOCKET_LISTEN_FAILED);
         goto err;
-- 
1.8.5.2

Re: [Qemu-devel] [PATCH 1/2] add 'umask' option to -chardev

[Daniel P. Berrange]

On Tue, Sep 02, 2014 at 03:40:42PM +0800, Chunyan Liu wrote:
> To use virtio-serial device, unix socket created for chardev with
> default umask(022) has insufficient permissions.
> 
> e.g. start kvm guest with:
> -device virtio-serial \
> -chardev socket,path=/tmp/foo,server,nowait,id=foo \
> -device virtserialport,chardev=foo,name=org.fedoraproject.port.0
> 
> Check permissions for the socket file that has been created in the host
> to enable communication through virtual serial ports in the guest:
> #ls -l /tmp/somefile.sock
> srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock
> 
> Other users in the qemu group (like real user, test engines, etc) cannot
> write to this socket.
> 
> Problem reported here:
> https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11
> https://bugzilla.novell.com/show_bug.cgi?id=888166
> 
> This patch tries to add a 'umask' option to 'chardev', so that user
> can have chance to indicate a umask overwritting the default one (default
> is 022), then create unix sockets with expected permissions.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
> This is patch for qemu.
> 
>  qemu-char.c         |  3 +++
>  qemu-options.hx     |  9 +++++++--
>  util/qemu-sockets.c | 12 +++++++++++-
>  3 files changed, 21 insertions(+), 3 deletions(-)

> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 5d38395..facf2c6 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -680,7 +680,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>  {
>      struct sockaddr_un un;
>      const char *path = qemu_opt_get(opts, "path");
> -    int sock, fd;
> +    int newmask = qemu_opt_get_number(opts, "umask", 0);
> +    int sock, fd, oldmask;
>  
>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
> @@ -708,10 +709,19 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>      }
>  
>      unlink(un.sun_path);
> +    if (newmask) {
> +        oldmask = umask(newmask);
> +    }
>      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
> +        if (newmask) {
> +            umask(oldmask);
> +        }
>          error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
>          goto err;
>      }
> +    if (newmask) {
> +        umask(oldmask);
> +    }

Setting  umask() is not thread-safe as it affects the entire process. While
this is OK for chardevs which are cold-plugged at startup, once QEMU is
running it is not OK to alter umask during hotplug of devices.

Wouldn't it be simpler for libvirt to simply set the umask to 002 when it
first launches QEMU, avoiding the need for trying todo this per device.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 1/2] add 'umask' option to -chardev

[Chun Yan Liu]

>>> On 9/2/2014 at 04:54 PM, in message <20140902085434.GA21282@redhat.com>,
"Daniel P. Berrange" <berrange@redhat.com> wrote: 
> On Tue, Sep 02, 2014 at 03:40:42PM +0800, Chunyan Liu wrote: 
> > To use virtio-serial device, unix socket created for chardev with 
> > default umask(022) has insufficient permissions. 
> >  
> > e.g. start kvm guest with: 
> > -device virtio-serial \ 
> > -chardev socket,path=/tmp/foo,server,nowait,id=foo \ 
> > -device virtserialport,chardev=foo,name=org.fedoraproject.port.0 
> >  
> > Check permissions for the socket file that has been created in the host 
> > to enable communication through virtual serial ports in the guest: 
> > #ls -l /tmp/somefile.sock 
> > srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock 
> >  
> > Other users in the qemu group (like real user, test engines, etc) cannot 
> > write to this socket. 
> >  
> > Problem reported here: 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11 
> > https://bugzilla.novell.com/show_bug.cgi?id=888166 
> >  
> > This patch tries to add a 'umask' option to 'chardev', so that user 
> > can have chance to indicate a umask overwritting the default one (default 
> > is 022), then create unix sockets with expected permissions. 
> >  
> > Signed-off-by: Chunyan Liu <cyliu@suse.com> 
> > --- 
> > This is patch for qemu. 
> >  
> >  qemu-char.c         |  3 +++ 
> >  qemu-options.hx     |  9 +++++++-- 
> >  util/qemu-sockets.c | 12 +++++++++++- 
> >  3 files changed, 21 insertions(+), 3 deletions(-) 
>  
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c 
> > index 5d38395..facf2c6 100644 
> > --- a/util/qemu-sockets.c 
> > +++ b/util/qemu-sockets.c 
> > @@ -680,7 +680,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp) 
> >  { 
> >      struct sockaddr_un un; 
> >      const char *path = qemu_opt_get(opts, "path"); 
> > -    int sock, fd; 
> > +    int newmask = qemu_opt_get_number(opts, "umask", 0); 
> > +    int sock, fd, oldmask; 
> >   
> >      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); 
> >      if (sock < 0) { 
> > @@ -708,10 +709,19 @@ int unix_listen_opts(QemuOpts *opts, Error **errp) 
> >      } 
> >   
> >      unlink(un.sun_path); 
> > +    if (newmask) { 
> > +        oldmask = umask(newmask); 
> > +    } 
> >      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) { 
> > +        if (newmask) { 
> > +            umask(oldmask); 
> > +        } 
> >          error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED); 
> >          goto err; 
> >      } 
> > +    if (newmask) { 
> > +        umask(oldmask); 
> > +    } 
>  
> Setting  umask() is not thread-safe as it affects the entire process. While 
> this is OK for chardevs which are cold-plugged at startup, once QEMU is 
> running it is not OK to alter umask during hotplug of devices. 
>  
> Wouldn't it be simpler for libvirt to simply set the umask to 002 when it 
> first launches QEMU, avoiding the need for trying todo this per device. 

I think that's OK. Only one thing: I'm not sure if permissions of any other
file created in qemu will be changed due to this change, and if that is
unexpected or not.
 
>  
> Regards, 
> Daniel 
> --  
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/  
> :| 
> |: http://libvirt.org              -o-             http://virt-manager.org  
> :| 
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/  
> :| 
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc  
> :| 
>  
>  
>  

Re: [Qemu-devel] [PATCH 1/2] add 'umask' option to -chardev

[Daniel P. Berrange]

On Tue, Sep 02, 2014 at 03:08:51AM -0600, Chun Yan Liu wrote:
> 
> 
> >>> On 9/2/2014 at 04:54 PM, in message <20140902085434.GA21282@redhat.com>,
> "Daniel P. Berrange" <berrange@redhat.com> wrote: 
> > On Tue, Sep 02, 2014 at 03:40:42PM +0800, Chunyan Liu wrote: 
> > > To use virtio-serial device, unix socket created for chardev with 
> > > default umask(022) has insufficient permissions. 
> > >  
> > > e.g. start kvm guest with: 
> > > -device virtio-serial \ 
> > > -chardev socket,path=/tmp/foo,server,nowait,id=foo \ 
> > > -device virtserialport,chardev=foo,name=org.fedoraproject.port.0 
> > >  
> > > Check permissions for the socket file that has been created in the host 
> > > to enable communication through virtual serial ports in the guest: 
> > > #ls -l /tmp/somefile.sock 
> > > srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock 
> > >  
> > > Other users in the qemu group (like real user, test engines, etc) cannot 
> > > write to this socket. 
> > >  
> > > Problem reported here: 
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11 
> > > https://bugzilla.novell.com/show_bug.cgi?id=888166 
> > >  
> > > This patch tries to add a 'umask' option to 'chardev', so that user 
> > > can have chance to indicate a umask overwritting the default one (default 
> > > is 022), then create unix sockets with expected permissions. 
> > >  
> > > Signed-off-by: Chunyan Liu <cyliu@suse.com> 
> > > --- 
> > > This is patch for qemu. 
> > >  
> > >  qemu-char.c         |  3 +++ 
> > >  qemu-options.hx     |  9 +++++++-- 
> > >  util/qemu-sockets.c | 12 +++++++++++- 
> > >  3 files changed, 21 insertions(+), 3 deletions(-) 
> >  
> > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c 
> > > index 5d38395..facf2c6 100644 
> > > --- a/util/qemu-sockets.c 
> > > +++ b/util/qemu-sockets.c 
> > > @@ -680,7 +680,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp) 
> > >  { 
> > >      struct sockaddr_un un; 
> > >      const char *path = qemu_opt_get(opts, "path"); 
> > > -    int sock, fd; 
> > > +    int newmask = qemu_opt_get_number(opts, "umask", 0); 
> > > +    int sock, fd, oldmask; 
> > >   
> > >      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); 
> > >      if (sock < 0) { 
> > > @@ -708,10 +709,19 @@ int unix_listen_opts(QemuOpts *opts, Error **errp) 
> > >      } 
> > >   
> > >      unlink(un.sun_path); 
> > > +    if (newmask) { 
> > > +        oldmask = umask(newmask); 
> > > +    } 
> > >      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) { 
> > > +        if (newmask) { 
> > > +            umask(oldmask); 
> > > +        } 
> > >          error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED); 
> > >          goto err; 
> > >      } 
> > > +    if (newmask) { 
> > > +        umask(oldmask); 
> > > +    } 
> >  
> > Setting  umask() is not thread-safe as it affects the entire process. While 
> > this is OK for chardevs which are cold-plugged at startup, once QEMU is 
> > running it is not OK to alter umask during hotplug of devices. 
> >  
> > Wouldn't it be simpler for libvirt to simply set the umask to 002 when it 
> > first launches QEMU, avoiding the need for trying todo this per device. 
> 
> I think that's OK. Only one thing: I'm not sure if permissions of any other
> file created in qemu will be changed due to this change, and if that is
> unexpected or not.

Whether or not it causes problems today is only half the story. I'm more
concerned about the long term problems. The use of threads is increasing
in QEMU over time, so manipulating umask() is a time-bomb waiting to strike
at some point in the future when people have forgotten that this proposed
feature exists. IMHO umask() should simply never be used when multiple
threads are running, to avoid this long term risk entirely.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 1/2] add 'umask' option to -chardev

[Chun Yan Liu]

>>> On 9/2/2014 at 05:16 PM, in message <20140902091655.GB21282@redhat.com>,
"Daniel P. Berrange" <berrange@redhat.com> wrote: 
> On Tue, Sep 02, 2014 at 03:08:51AM -0600, Chun Yan Liu wrote: 
> >  
> >  
> > >>> On 9/2/2014 at 04:54 PM, in message <20140902085434.GA21282@redhat.com>, 
> > "Daniel P. Berrange" <berrange@redhat.com> wrote:  
> > > On Tue, Sep 02, 2014 at 03:40:42PM +0800, Chunyan Liu wrote:  
> > > > To use virtio-serial device, unix socket created for chardev with  
> > > > default umask(022) has insufficient permissions.  
> > > >   
> > > > e.g. start kvm guest with:  
> > > > -device virtio-serial \  
> > > > -chardev socket,path=/tmp/foo,server,nowait,id=foo \  
> > > > -device virtserialport,chardev=foo,name=org.fedoraproject.port.0  
> > > >   
> > > > Check permissions for the socket file that has been created in the host  
> > > > to enable communication through virtual serial ports in the guest:  
> > > > #ls -l /tmp/somefile.sock  
> > > > srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock  
> > > >   
> > > > Other users in the qemu group (like real user, test engines, etc) cannot  
> > > > write to this socket.  
> > > >   
> > > > Problem reported here:  
> > > > https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11  
> > > > https://bugzilla.novell.com/show_bug.cgi?id=888166  
> > > >   
> > > > This patch tries to add a 'umask' option to 'chardev', so that user  
> > > > can have chance to indicate a umask overwritting the default one (default  
>  
> > > > is 022), then create unix sockets with expected permissions.  
> > > >   
> > > > Signed-off-by: Chunyan Liu <cyliu@suse.com>  
> > > > ---  
> > > > This is patch for qemu.  
> > > >   
> > > >  qemu-char.c         |  3 +++  
> > > >  qemu-options.hx     |  9 +++++++--  
> > > >  util/qemu-sockets.c | 12 +++++++++++-  
> > > >  3 files changed, 21 insertions(+), 3 deletions(-)  
> > >   
> > > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c  
> > > > index 5d38395..facf2c6 100644  
> > > > --- a/util/qemu-sockets.c  
> > > > +++ b/util/qemu-sockets.c  
> > > > @@ -680,7 +680,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)  
> > > >  {  
> > > >      struct sockaddr_un un;  
> > > >      const char *path = qemu_opt_get(opts, "path");  
> > > > -    int sock, fd;  
> > > > +    int newmask = qemu_opt_get_number(opts, "umask", 0);  
> > > > +    int sock, fd, oldmask;  
> > > >    
> > > >      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);  
> > > >      if (sock < 0) {  
> > > > @@ -708,10 +709,19 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)  
> > > >      }  
> > > >    
> > > >      unlink(un.sun_path);  
> > > > +    if (newmask) {  
> > > > +        oldmask = umask(newmask);  
> > > > +    }  
> > > >      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {  
> > > > +        if (newmask) {  
> > > > +            umask(oldmask);  
> > > > +        }  
> > > >          error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);  
> > > >          goto err;  
> > > >      }  
> > > > +    if (newmask) {  
> > > > +        umask(oldmask);  
> > > > +    }  
> > >   
> > > Setting  umask() is not thread-safe as it affects the entire process.  
> While  
> > > this is OK for chardevs which are cold-plugged at startup, once QEMU is  
> > > running it is not OK to alter umask during hotplug of devices.  
> > >   
> > > Wouldn't it be simpler for libvirt to simply set the umask to 002 when it  
> > > first launches QEMU, avoiding the need for trying todo this per device.  
> >  
> > I think that's OK. Only one thing: I'm not sure if permissions of any other 
> > file created in qemu will be changed due to this change, and if that is 
> > unexpected or not. 
>  
> Whether or not it causes problems today is only half the story. I'm more 
> concerned about the long term problems. The use of threads is increasing 
> in QEMU over time, so manipulating umask() is a time-bomb waiting to strike 
> at some point in the future when people have forgotten that this proposed 
> feature exists. IMHO umask() should simply never be used when multiple 
> threads are running, to avoid this long term risk entirely. 

I agree. Now suppose when it explicitly sets the mode S_IWGRP while creating
new file, group user writing permission is really expected, I think setting umask
to 002 to the whole qemu process should be OK.

>  
> Regards, 
> Daniel 
> --  
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/  
> :| 
> |: http://libvirt.org              -o-             http://virt-manager.org  
> :| 
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/  
> :| 
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc  
> :| 
>  
>  
>  

Re: [Qemu-devel] [PATCH 1/2] add 'umask' option to -chardev

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, Sep 02, 2014 at 03:40:42PM +0800, Chunyan Liu wrote:
>> To use virtio-serial device, unix socket created for chardev with
>> default umask(022) has insufficient permissions.
>> 
>> e.g. start kvm guest with:
>> -device virtio-serial \
>> -chardev socket,path=/tmp/foo,server,nowait,id=foo \
>> -device virtserialport,chardev=foo,name=org.fedoraproject.port.0
>> 
>> Check permissions for the socket file that has been created in the host
>> to enable communication through virtual serial ports in the guest:
>> #ls -l /tmp/somefile.sock
>> srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock
>> 
>> Other users in the qemu group (like real user, test engines, etc) cannot
>> write to this socket.
>> 
>> Problem reported here:
>> https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11
>> https://bugzilla.novell.com/show_bug.cgi?id=888166
>> 
>> This patch tries to add a 'umask' option to 'chardev', so that user
>> can have chance to indicate a umask overwritting the default one (default
>> is 022), then create unix sockets with expected permissions.
>> 
>> Signed-off-by: Chunyan Liu <cyliu@suse.com>
>> ---
>> This is patch for qemu.
>> 
>>  qemu-char.c         |  3 +++
>>  qemu-options.hx     |  9 +++++++--
>>  util/qemu-sockets.c | 12 +++++++++++-
>>  3 files changed, 21 insertions(+), 3 deletions(-)
>
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index 5d38395..facf2c6 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -680,7 +680,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>>  {
>>      struct sockaddr_un un;
>>      const char *path = qemu_opt_get(opts, "path");
>> -    int sock, fd;
>> +    int newmask = qemu_opt_get_number(opts, "umask", 0);
>> +    int sock, fd, oldmask;
>>  
>>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>>      if (sock < 0) {
>> @@ -708,10 +709,19 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>>      }
>>  
>>      unlink(un.sun_path);
>> +    if (newmask) {
>> +        oldmask = umask(newmask);
>> +    }
>>      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
>> +        if (newmask) {
>> +            umask(oldmask);
>> +        }
>>          error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
>>          goto err;
>>      }
>> +    if (newmask) {
>> +        umask(oldmask);
>> +    }
>
> Setting  umask() is not thread-safe as it affects the entire process. While
> this is OK for chardevs which are cold-plugged at startup, once QEMU is
> running it is not OK to alter umask during hotplug of devices.

Indeed.  No telling what else could be affected while the temporary
umask is in effect.

> Wouldn't it be simpler for libvirt to simply set the umask to 002 when it
> first launches QEMU, avoiding the need for trying todo this per device.

I guess that's okay as long as you also make sure QEMU's effective group
is sane.

If you must have laxer permissions just for a specific character device,
you can perhaps chmod() after creating it.  No good for tightening
permissions, of course.

[Qemu-devel] [PATCH 2/2] qemu: add umask(002) to virtio-serial chardev commandline

[Chunyan Liu]

To use virtio-serial device, unix socket created for communication with
default umask(022) has insufficient permissions.
e.g.
1. Setup a virtual machine with a virtio-serial device:
# virsh edit myvm
(...)
    <channel type='unix'>
      <source mode='bind' path='/tmp/somefile.sock'/>
      <target type='virtio' name='com.suse.sometest'/>
      <address type='virtio-serial' controller='0' bus='0' port='1'/>
    </channel>
(...)
    <controller type='virtio-serial' index='0'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x06'
function='0x0'/>
    </controller>

2. Start this virtual machine:
# virsh start myvm

3. Check permissions for the socket file that has been created in the
host to enable communication through virtual serial ports in the guest:
# ls -l /tmp/somefile.sock
srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock

Other users in the qemu group (like real user, test engines, etc) cannot
write to this socket.

Problem reported here:
https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11
https://bugzilla.novell.com/show_bug.cgi?id=888166

This patch tries to pass a 'umask' option to '-chardev' when
building qemu command line in above configuration case. In
qemu side, there is another patch to handle the 'umask' option
to overwrite default umask(022). With these changes, unix
socket created for virtio-serial device can have expected
permissions.

Signed-off-by: Chunyan Liu <cyliu@suse.com> 
---
This is patch for libvirt.

 src/qemu/qemu_command.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index beb8ca8..11eee44 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8509,6 +8509,18 @@ qemuBuildCommandLine(virConnectPtr conn,
                                                       channel->info.alias,
                                                       qemuCaps)))
                     goto error;
+                /* use umask(002) instead of default umask(022) to create
+                 * a unix socket, so that virtio-serial device has sufficient
+                 * permissions for correct usage.
+                 */
+                if (channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX) {
+                    char *tmpstr = NULL;
+                    if (virAsprintf(&tmpstr, "%s,umask=0x002", devstr) < 0)
+                        goto error;
+                    VIR_FREE(devstr);
+                    devstr = tmpstr;
+                }
+
                 virCommandAddArg(cmd, devstr);
                 VIR_FREE(devstr);
             }
-- 
1.8.4.5