Re: [Xen-devel] [PATCH 9/9] libxl: Kill QEMU with "reaper" ruid

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

George Dunlap writes ("[PATCH 9/9] libxl: Kill QEMU with "reaper" ruid"):
> Using kill(-1) to killing an untrusted dm process with the real uid
> equal to the dm_uid isn't guaranteed to succeed: the process in
> question may be able to kill the reaper process after the setresuid()
> and before the kill().
...
> +static int libxl__get_reaper_uid(libxl__gc *gc)
> +{
> +    struct passwd *user_base, user_pwbuf;
> +    int ret;
> +    ret = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_RANGE_BASE,
> +                                         &user_pwbuf, &user_base);

ret should be r I think.

Also I think you need to handle errors properly ?  Ie set and check
errno.

>  const char *libxl__domain_device_model(libxl__gc *gc,
>                                         const libxl_domain_build_info *info)
>  {
> @@ -2719,12 +2728,62 @@ void libxl__destroy_device_model(libxl__egc *egc,
...
> -        ret = setresuid(dm_uid, dm_uid, 0);
> +        fd = open(lockfile, O_RDWR|O_CREAT, 0666);
> +        if (fd < 0) {
> +            /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
> +            LOGED(ERROR, domid,
> +                  "unexpected error while trying to open lockfile %s, 
> errno=%d",
> +                  lockfile, errno);
> +            goto kill;

More gotos!  I doubt this error handling is right.

I'm also not convinced that it is sensible to handle the case where we
have multiple per-domain uids but no reaper uid.  This turns host
configuration errors into systems that are less secure than they
should be in a really obscure way.

> +        /* Try to lock the file */
> +        while (flock(fd, LOCK_EX)) {

CODING_STYLE, no call inside the condition please.


Overall I think this stuff needs a different error handling approach:

 * We should distinguish expected and reasonable configurations, where
   we fall back to less secure methods, from other unexpected
   situations.

 * In other unexpected situations (whether bad host configuration or
   syscall errors or whatever) we should make a best effort to
   destroy as much as we can.

 * But crucially in such situations (i) overall destroy ao should
   return a failure error code (ii) the domain itself should not be
   destroyed in Xen.  This means that `xl destroy' fails, and can be
   repeated after the problem is corrected.

Thanks,
Ian.

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