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

[Markus Armbruster]

"Daniel P. Berrange" <berrange at 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 at 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.