Re: [Xen-devel] [PATCH v7] run QEMU as non-root

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 7 Aug 2015, Wei Liu wrote:
> On Thu, Jul 23, 2015 at 06:08:02PM +0100, Stefano Stabellini wrote:
> [...]
> > +For security reasons, libxl tries to pass a non-root username to QEMU as
> > +argument. During initialization QEMU calls setuid and setgid with the
> > +user ID and the group ID of the user passed as argument.
> > +Libxl looks for the following users in this order:
> > +
> > +1) a user named "xen-qemuuser-domid$domid", 
> > +Where $domid is the domid of the domain being created.
> > +This requires the reservation of 65535 uids from xen-qemuuser-domid1
> > +to xen-qemuuser-domid65535. To use this mechanism, you might want to
> > +create a large number of users at installation time. For example:
> > +
> > +for ((i=1; i<65536; i++))
> > +do
> > +    adduser --no-create-home --system xen-qemuuser-domid$i
> > +done
> > +
> > +You might want to consider passing --group to adduser to create a new
> > +group for each new user.
> > +
> > +
> > +2) a user named "xen-qemuuser-shared"
> > +As a fall back if both 1) and 2) fail, libxl will use a single user for
>                      ^^^^^^^^^^^^^^
> This is 2)

right

> > +all QEMU instances. The user is named xen-qemuuser-shared. This is
> > +less secure but still better than running QEMU as root. Using this is as
> > +simple as creating just one more user on your host:
> > +
> > +adduser --no-create-home --system xen-qemuuser-shared
> > +
> > +
> > +3) root
> > +As a last resort, libxl will start QEMU as root.
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index efc0617..3f4283f 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -192,6 +192,12 @@
> >   * is not present, instead of ERROR_INVAL.
> >   */
> >  #define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
> > +
> > +/* libxl_domain_build_info has device_model_user to specify the user to
> > + * run the device model with. See docs/misc/qemu-deprivilege.txt.
> > + */
> > +#define LIBXL_HAVE_DEVICE_MODEL_USER 1
> > +
> >  /*
> >   * libxl ABI compatibility
> >   *
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 0c6408d..24c43df 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -19,6 +19,8 @@
> >  
> >  #include "libxl_internal.h"
> >  #include <xen/hvm/e820.h>
> > +#include <sys/types.h>
> > +#include <pwd.h>
> >  
> >  static const char *libxl_tapif_script(libxl__gc *gc)
> >  {
> > @@ -418,6 +420,33 @@ static char *dm_spice_options(libxl__gc *gc,
> >      return opt;
> >  }
> >  
> > +/* return 1 if the user was found, 0 if it was not, -1 on error */
> > +static int libxl__dm_runas_helper(libxl__gc *gc, char *username)
> 
> const char *

ok


> > +{
> > +    struct passwd pwd, *user = NULL;
> > +    char *buf = NULL;
> > +    long buf_size;
> > +
> > +    buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
> > +    if (buf_size < 0) {
> > +        LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error %ld",
> > +                buf_size);
> > +        return -1;
> > +    }
> > +
> > +retry:
> > +    buf = libxl__realloc(gc, buf, buf_size);
> 
> This should be libxl__alloc and placed out of the loop?

But the point is that buf can be increase by 128 bytes chunks in
following iterations. How can I do that if I place it outside the loop?


> > +    errno = 0;
> > +    getpwnam_r(username, &pwd, buf, buf_size, &user);
> > +    if (user != NULL)
> > +        return 1;
> > +    if (errno == ERANGE) {
> > +        buf_size += 128;
> > +        goto retry;
> > +    }
> 
> Please use for / while to loop.

The goto retry loop is a very common patter for error handling, but I
can turn it into a loop if you are keen on it.


> Also you might want to save and restore errno.

Across the getpwnam_r call? Or across the loop? Or across the function
libxl__dm_runas_helper?


> > +    return 0;
> > +}
> > +
> >  static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> >                                          const char *dm, int guest_domid,
> >                                          const libxl_domain_config 
> > *guest_config,
> > @@ -439,6 +468,7 @@ static char ** 
> > libxl__build_device_model_args_new(libxl__gc *gc,
> >      int i, connection, devid;
> >      uint64_t ram_size;
> >      const char *path, *chardev;
> > +    char *user = NULL;
> >  
> >      dm_args = flexarray_make(gc, 16, 1);
> >  
> > @@ -878,6 +908,31 @@ static char ** 
> > libxl__build_device_model_args_new(libxl__gc *gc,
> >          default:
> >              break;
> >          }
> > +
> > +        if (b_info->device_model_user) {
> > +            user = b_info->device_model_user;
> > +            goto end_search;
> > +        }
> > +
> > +        user = libxl__sprintf(gc, "%s%d", LIBXL_QEMU_USER_BASE, 
> > guest_domid);
> > +        if (libxl__dm_runas_helper(gc, user) > 0)
> > +            goto end_search;
> 
> You haven't checked if libxl__dm_runas_helper returns -1. In that case
> we should bail?

Yes, I think return NULL would be OK.


> > +
> > +        user = LIBXL_QEMU_USER_SHARED;
> > +        if (libxl__dm_runas_helper(gc, user) > 0) {
> > +            LOG(WARN, "Could not find user %s%d, falling back to %s",
> > +                    LIBXL_QEMU_USER_BASE, guest_domid, 
> > LIBXL_QEMU_USER_SHARED);
> > +            goto end_search;
> > +        }
> > +
> > +        user = NULL;
> > +        LOG(WARN, "Could not find user %s, starting QEMU as root", 
> > LIBXL_QEMU_USER_SHARED);
> > +
> 
> Line too long.
> 
> Wei.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel