Re: [PATCH v4 3/6] tools/dm_restrict: Ask QEMU to chroot

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of George Dunlap
> Sent: 05 November 2018 18:07
> To: xen-devel@lists.xenproject.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>
> Subject: [Xen-devel] [PATCH v4 3/6] tools/dm_restrict: Ask QEMU to chroot
> 
> When dm_restrict is enabled, ask QEMU to chroot into an empty directory.
> 
> * Create /var/run/qemu/root-domid (deleting the old one if it's there)

This does not appear to match the code: the path should be /var/run/qemu-root-<domid> AFAICT

> * Pass the -chroot option to QEMU
> 
> Rather than running `rm -rf` on the directory before creating it
> (since there is no library function to do this), simply rmdir the
> directory, relying on the fact that the previous QEMU instance, if
> properly restricted, shouldn't have been able to write anything
> anyway.
> 
> Suggested-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> Changes since v2:
> - Style fixes
> - Testing moved to a different patch
> 
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Anthony Perard <anthony.perard@citrix.com>
> ---
>  docs/designs/qemu-deprivilege.md | 12 +++++-----
>  tools/libxl/libxl_dm.c           | 41 +++++++++++++++++++++++++++++++-
>  2 files changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/designs/qemu-deprivilege.md b/docs/designs/qemu-
> deprivilege.md
> index 787ae1ac7c..0395bbbb40 100644
> --- a/docs/designs/qemu-deprivilege.md
> +++ b/docs/designs/qemu-deprivilege.md
> @@ -61,12 +61,6 @@ source tree.)
> 
>  '''Testing status''': Tested
> 
> -# Restrictions / improvements still to do
> -
> -This lists potential restrictions still to do.  It is meant to be
> -listed in order of ease of implementation, with low-hanging fruit
> -first.
> -
>  ## Chroot
> 
>  '''Description''': Qemu runs in its own chroot, such that even if it
> @@ -84,6 +78,12 @@ Then adds the following to the qemu command-line:
> 
>  '''Tested''': Not tested
> 
> +## Restrictions / improvements still to do
> +
> +This lists potential restrictions still to do.  It is meant to be
> +listed in order of ease of implementation, with low-hanging fruit
> +first.
> +
>  ## Namespaces for unused functionality (Linux only)
> 
>  '''Description''': QEMU doesn't use the functionality associated with
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 26eb16af34..ad3efcc783 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1410,9 +1410,48 @@ static int
> libxl__build_device_model_args_new(libxl__gc *gc,
>          }
>      }
> 
> -    if (libxl_defbool_val(b_info->dm_restrict))
> +    if (libxl_defbool_val(b_info->dm_restrict)) {
> +        char *chroot_dir = GCSPRINTF("%s/qemu-root-%d",
> +                                      libxl__run_dir_path(),
> guest_domid);
> +        int r;
> +
>          flexarray_append(dm_args, "-xen-domid-restrict");
> 
> +        /*
> +         * Run QEMU in a chroot at XEN_RUN_DIR/qemu-root-%d

Maybe '<domid>' in the comment rather than '%d'?

> +         *
> +         * There is no library function to do the equivalent of `rm
> +         * -rf`.  However deprivileged QEMU in theory shouldn't be
> +         * able to write any files, as the chroot would be owned by
> +         * root, but it would be running as an unprivileged process.
> +         * So in theory, old chroots should always be empty.

How does logging work if QEMU can't write to the chroot? I assume we are relying on stderr? Does using syslog still work?

  Paul

> +         *
> +         * rmdir the directory before attempting to create
> +         * it; if it returns anything other than ENOENT, fail domain
> +         * creation.
> +         */
> +        r = rmdir(chroot_dir);
> +        if (r != 0 && errno != ENOENT) {
> +            LOGED(ERROR, guest_domid,
> +                  "failed to remove existing chroot dir %s", chroot_dir);
> +            return ERROR_FAIL;
> +        }
> +
> +        for (;;) {
> +            r = mkdir(chroot_dir, 0000);
> +            if (!r)
> +                break;
> +            if (errno == EINTR) continue;
> +            LOGED(ERROR, guest_domid,
> +                  "failed to create chroot dir %s", chroot_dir);
> +            return ERROR_FAIL;
> +        }
> +
> +        /* Add "-chroot [dir]" to command-line */
> +        flexarray_append(dm_args, "-chroot");
> +        flexarray_append(dm_args, chroot_dir);
> +    }
> +
>      if (state->saved_state) {
>          /* This file descriptor is meant to be used by QEMU */
>          *dm_state_fd = open(state->saved_state, O_RDONLY);
> --
> 2.19.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel