Re: [Xen-devel] [PATCH 8/9] libxl: Kill QEMU by uid when possible

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

George Dunlap writes ("[PATCH 8/9] libxl: Kill QEMU by uid when possible"):
> The privcmd fd that a dm_restrict'ed QEMU has gives it permission to
> one specific domain ID.  This domain ID will probably eventually be
> used again.  It is therefore necessary to make absolutely sure that a
> rogue QEMU process cannot hang around after its domain has exited.

Thanks.  This looks roughly right but I have some coding style
quibbles, and I am not convinced the error handling is right.
                                  
> @@ -2382,6 +2389,15 @@ void libxl__spawn_local_dm(libxl__egc *egc, 
> libxl__dm_spawn_state *dmss)
>  
>      const char *dom_path = libxl__xs_get_dompath(gc, domid);
>  
> +    /* 
> +     * If we're stating the dm with a non-root UID, save the UID so

Typo for `starting'.

> +     * that we can reliably kill it and any subprocesses
> +     */
...
> +static void kill_device_model_uid_cb(libxl__egc *egc,
> +                                     libxl__ev_child *destroyer,
> +                                     pid_t pid, int status);
> +
>  void libxl__destroy_device_model(libxl__egc *egc,
>                                   libxl__destroy_devicemodel_state *ddms)
>  {
> @@ -2658,15 +2678,103 @@ void libxl__destroy_device_model(libxl__egc *egc,
>      int rc;
>      int domid = ddms->domid;
>      char *path = DEVICE_MODEL_XS_PATH(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
> +    const char * dm_uid_str;
> +    uid_t dm_uid;
> +    int reaper_pid;
> +    int ret;
>      
>      if (!xs_rm(CTX->xsh, XBT_NULL, path))
>          LOGD(ERROR, domid, "xs_rm failed for %s", path);
>      
> -    /* We should try to destroy the device model anyway. */
> -    rc = kill_device_model(gc,
> -              GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
> +    /* 
> +     * We should try to destroy the device model anyway.  Check to see
> +     * if we can kill by UID
> +     */
> +    ret = libxl__xs_read_checked(gc, XBT_NULL,
> +                                
> GCSPRINTF("/local/domain/%d/image/device-model-uid",
> +                                           domid),
> +                                 &dm_uid_str);

I know this function is bad in its use of `rc' for syscall return but
please don't make it worse by introducing `ret' for what should be
`rc'.  Would you mind adding a pre-patch to change `rc' to `r' and
then you can use `rc' ?

> +    if (ret || !dm_uid_str) {

Shouldn't we fail if libxl__xs_read_checked fails ?
Otherwise we risk leaving fragments of domain behind even if we fail.
(Possibly we should carry on, but accumulate errors in rc.)

> +        /* No uid in xenstore; just kill the pid we have */
> +        LOGD(DEBUG, domid, "Didn't find dm UID; destroying by pid");
> +        
> +        rc = kill_device_model(gc,
> +                               
> GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
> +    
> +        libxl__qmp_cleanup(gc, domid);
> +
> +        ddms->callback(egc, ddms, rc);
> +        return;

Can you please break out this little exit code fragment

  +        libxl__qmp_cleanup(gc, domid);
  +        ddms->callback(egc, ddms, rc);

into a sub-function ?  It occurs twice and it is easier to reason
about things if the operation has a single exit path.

> +    /* QEMU has its own uid; kill all processes with that UID */
> +    LOGD(DEBUG, domid, "Found DM uid %s, destroying by uid", dm_uid_str);
> +    
> +    dm_uid = atoi(dm_uid_str);

I am tempted to suggest a more robust use of strtoul here but since
this came from our own xenstore node, fine.

> +    reaper_pid = libxl__ev_child_fork(gc, &ddms->destroyer,
> +                                      kill_device_model_uid_cb);
> +    if (reaper_pid < 0)
> +        ddms->callback(egc, ddms, ERROR_FAIL);
> +    }
> +

And, voila!  Didn't you mean to call libxl__qmp_cleanup ?  You're
missing the exit path.  And maybe the call to it should be an `out'
block in this function.

Also, amazingly, you don't return or anything here.  This is only not
a bug because the rest is all the child process.

> +    if (!reaper_pid) { /* child */
> +        const char * call;
> +
> +        /* 
> +         * FIXME: the second uid needs to be distinct to avoid being
> +         * killed by a potential rogue process
> +         */

This is a bit of a funny way of introducing this but fine.

> +        ret = setresuid(dm_uid, dm_uid, 0);
> +        if (ret) {
> +            call = "setresuid";
> +            goto badchild;
> +        }
> +
> +        /* And kill everyone but me */
> +        ret = kill(-1, 9);
> +        if (ret) {
> +            call = "kill";
> +            goto badchild;
> +        }
> +        _exit(0);
> +        
> +    badchild:
> +        if (errno > 0  && errno < 126) {
> +            _exit(errno);
> +        } else {
> +            LOGED(ERROR, domid,
> +                  "Call %s failed (with difficult errno value %d)",
> +                  call, errno);
> +            _exit(-1);
> +        }
> +    }

I don't much like these gotos either.  Maybe a subfunction is called
for.

Furthermore, once again please see CODING_STYLE re conventional
variable names `r' and `rc'·

> +out:
> +    libxl__qmp_cleanup(gc, ddms->domid);

>      ddms->callback(egc, ddms, rc);

Here's that finish fragment again.

> index 899a86e84b..59eac0662a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1135,7 +1135,7 @@ typedef struct {
>      const char * shim_cmdline;
>      const char * pv_cmdline;
>  
> -    char * dm_runas;
> +    char * dm_runas,  *dm_uid;

It would be nice to drop the spurious space before dm_runas while you
are here, and then presumably the double space before *dm_uid would
not be needed.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel