Re: [Xen-devel] [PATCH v2 3/6] tools/dm_restrict: Ask QEMU to chroot

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
> Of George Dunlap
> Sent: 21 September 2018 18:04
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>
> Subject: [Xen-devel] [PATCH v2 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)
> * 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 restcirted, shouldn't have been able to write anything

^ typo... 'restricted'

> anyway.
> 
> Also test that this worked in depriv-process-checker.sh.  This
> requires XEN_RUN_DIR to be set in the environment so we know where to
> look.
> 
> Suggested-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> ---
> Not sure the best way to get XEN_RUN_DIR; having configure process this
> file seems like a bit overkill.
> 
> CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Anthony Perard <anthony.perard@xxxxxxxxxx>
> ---
>  docs/designs/qemu-deprivilege.md             | 12 +++---
>  tools/libxl/libxl_dm.c                       | 39 +++++++++++++++++++-
>  tools/tests/depriv/depriv-process-checker.sh | 17 +++++++++
>  3 files changed, 61 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/designs/qemu-deprivilege.md b/docs/designs/qemu-
> deprivilege.md
> index 1e731c16aa..df5bb07d7c 100644
> --- a/docs/designs/qemu-deprivilege.md
> +++ b/docs/designs/qemu-deprivilege.md
> @@ -58,12 +58,6 @@ FIXME: Double-check the correctness of the above
> 
>  '''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
> @@ -81,6 +75,12 @@ Then adds the following to the qemu command-line:
> 
>  '''Tested''': Not tested

^ should this change to 'tested' now?

  Paul

> 
> +## 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''': Enter QEMU into its own mount & IPC namespaces.
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index abd31ee6f2..df6fb64bee 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1410,9 +1410,46 @@ 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);
> +
>          flexarray_append(dm_args, "-xen-domid-restrict");
> 
> +        /*
> +         * Run QEMU in a chroot at /var/run/xen/root-%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 anyway, 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.
> +         *
> +         * rmdir the directory before attempting to create
> +         * it; if it returns anything other than ENOENT, fail domain
> +         * creation.
> +         */
> +        if (rmdir(chroot_dir) != 0
> +            && errno != ENOENT) {
> +            LOGED(ERROR, guest_domid,
> +                  "failed to remove existing chroot dir %s", chroot_dir);
> +            return ERROR_FAIL;
> +        }
> +
> +        for (;;) {
> +            if (!mkdir(chroot_dir, 0000))
> +                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);
> diff --git a/tools/tests/depriv/depriv-process-checker.sh
> b/tools/tests/depriv/depriv-process-checker.sh
> index 4aa58e7760..cb240fd0ed 100755
> --- a/tools/tests/depriv/depriv-process-checker.sh
> +++ b/tools/tests/depriv/depriv-process-checker.sh
> @@ -64,6 +64,23 @@ else
>  fi
>  echo $result
> 
> +# TEST: chroot
> +#
> +# Read /proc/<dmpid>/root to see if it's correct.
> +echo -n "Chroot: "
> +if [[ -n "$XEN_RUN_DIR" ]] ; then
> +    tgt_chroot=$XEN_RUN_DIR/qemu-root-$domid
> +    root=$(readlink /proc/$dmpid/root)
> +    if [[ "$root" != "$tgt_chroot" ]] ; then
> +     echo "FAILED"
> +     failed="true"
> +    else
> +     echo "PASSED"
> +    fi
> +else
> +    echo "SKIPPED (XEN_RUN_DIR undefined)"
> +fi
> +
>  if $failed ; then
>      exit 1
>  else
> --
> 2.18.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel