Re: [PATCH v3 36/38] altp2m: Allow specifying external-only use-case

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Aug 17, 2016 at 12:17:12AM +0200, Sergej Proskurin wrote:
> From: Tamas K Lengyel <tamas.lengyel@zentific.com>
> 
> Currently setting altp2mhvm=1 in the domain configuration allows access to the
> altp2m interface for both in-guest and external privileged tools. This poses
> a problem for use-cases where only external access should be allowed, requiring
> the user to compile Xen with XSM enabled to be able to appropriately restrict
> access.
> 
> In this patch we deprecate the altp2mhvm domain configuration option and
> introduce the altp2m option, which allows specifying if by default the altp2m
> interface should be external-only. The information is stored in
> HVM_PARAM_ALTP2M which we now define with specific XEN_ALTP2M_* modes.
> If external_only mode is selected, the XSM check is shifted to use XSM_DM_PRIV
> type check, thus restricting access to the interface by the guest itself. Note
> that we keep the default XSM policy untouched. Users of XSM who wish to enforce
> external_only mode for altp2m can do so by adjusting their XSM policy directly,
> as this domain config option does not override an active XSM policy.
> 
> Also, as part of this patch we adjust the hvmop handler to require
> HVM_PARAM_ALTP2M to be of a type other then disabled for all ops. This has been
> previously only required for get/set altp2m domain state, all other options
> were gated on altp2m_enabled. Since altp2m_enabled only gets set during set
> altp2m domain state, this change introduces no new requirements to the other
> ops but makes it more clear that it is required for all ops.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>


During my review of Tamas's original patch, I figured that it would
be quite ugly to handle the old type vs new type of altp2m in order to
not break compilation for older application.

But it seems that we shall just keep altp2mhvm libxl_def_bool and use
the new type for altp2m.  I think by taking this patch in the series, we
manage to reduce the compatibility cruft needed, which is good.

> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> v2: Rename HVMALTP2M_* to XEN_ALTP2M_*
>     Relax xsm check to XSM_DM_PRIV for external-only mode
> 
> v3: Introduce macro LIBXL_HAVE_ARM_ALTP2M in parallel to the former
>     LIBXL_HAVE_ALTP2M to differentiate between altp2m for x86 and and
>     altp2m for ARM architectures.
> 
>     Document the option "altp2m" in ./docs/man/xl.cfg.pod.5.in.
> 
>     Maintain the legacy info->u.hvm.altp2m field for x86 HVM domains in
>     parallel to the introduced info->altp2m field for x86 HVM and ARM
>     domains.
> ---
>  docs/man/xl.cfg.pod.5.in        | 37 ++++++++++++++++++++++++++++++++++++-
>  tools/libxl/libxl.h             | 10 +++++++++-
>  tools/libxl/libxl_create.c      |  7 +++++--
>  tools/libxl/libxl_dom.c         | 30 ++++++++++++++++++++++++++++--
>  tools/libxl/libxl_types.idl     | 13 +++++++++++++
>  tools/libxl/xl_cmdimpl.c        | 25 ++++++++++++++++++++++++-
>  xen/arch/arm/hvm.c              | 14 +++++++++++++-
>  xen/arch/x86/hvm/hvm.c          | 20 ++++++++++----------
>  xen/include/public/hvm/params.h | 10 +++++++++-
>  xen/include/xsm/dummy.h         | 14 +++++++++++---
>  xen/include/xsm/xsm.h           |  6 +++---
>  xen/xsm/flask/hooks.c           |  2 +-
>  12 files changed, 162 insertions(+), 26 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index 48c9c0d..bf9a48a 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -1268,6 +1268,37 @@ enabled by default and you should usually omit it. It may be necessary
>  to disable the HPET in order to improve compatibility with guest
>  Operating Systems (X86 only)
>  
> +=item B<altp2m=MODE>
> +
> +Specifies access mode to the alternate-p2m capability. Alternate-p2m allows a
> +guest to manage multiple p2m guest physical "memory views" (as opposed to a
> +single p2m). This option is disabled by default and is available to x86 hvm and
> +ARM domains. You may want this option if you want to access-control/isolate
> +access to specific guest physical memory pages accessed by the guest, e.g. for
> +domain memory introspection or for isolation/access-control of memory between
> +components within a single guest domain.
> +
> +The valid values are as follows:
> +
> +=over 4
> +
> +=item B<"disabled">
> +
> +Altp2m is disabled for the domain (default).
> +
> +=item B<"mixed">
> +
> +The mixed mode allows access to the altp2m interface for both in-guest
> +and external tools as well.
> +
> +=item B<"external_only">
> +
> +Enables access to the alternate-p2m capability for hvm guests only
> +by external privileged tools. Note: if XSM is enabled then the XSM policy
> +should be used to specify external-only access to the interface.
> +
> +=back
> +
>  =item B<altp2mhvm=BOOLEAN>
>  
>  Enables or disables hvm guest access to alternate-p2m capability.
> @@ -1278,7 +1309,11 @@ You may want this option if you want to access-control/isolate
>  access to specific guest physical memory pages accessed by
>  the guest, e.g. for HVM domain memory introspection or
>  for isolation/access-control of memory between components within
> -a single guest hvm domain.
> +a single guest hvm domain. This option is deprecated, use the option
> +"altp2m" instead.
> +
> +Note: While the option "altp2mhvm" is deprecated, legacy applications for
> +x86 systems will continue to work using it.
>  
>  =item B<nestedhvm=BOOLEAN>
>  
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index ae21302..f78d63e 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -847,11 +847,19 @@ typedef struct libxl__ctx libxl_ctx;
>  
>  /*
>   * LIBXL_HAVE_ALTP2M
> - * If this is defined, then libxl supports alternate p2m functionality.
> + * If this is defined, then libxl supports alternate p2m functionality for
> + * x86 HVM guests.
>   */
>  #define LIBXL_HAVE_ALTP2M 1
>  
>  /*
> + * LIBXL_HAVE_ARM_ALTP2M
> + * If this is defined, then libxl supports alternate p2m functionality for
> + * ARM guests.
> + */
> +#define LIBXL_HAVE_ARM_ALTP2M 1
> +
> +/*
>   * LIBXL_HAVE_REMUS
>   * If this is defined, then libxl supports remus.
>   */
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 04f8ae9..21e32c9 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -406,6 +406,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>              libxl_domain_type_to_string(b_info->type));
>          return ERROR_INVAL;
>      }
> +

Unrelated change.

>      return 0;
>  }
>  
> @@ -918,8 +919,10 @@ static void initiate_domain_create(libxl__egc *egc,
>  
>      if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
>          (libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) &&
> -         libxl_defbool_val(d_config->b_info.u.hvm.altp2m))) {
> -        LOG(ERROR, "nestedhvm and altp2mhvm cannot be used together");
> +        /* Cannot combine nested_hvm with altp2m or altp2mhvm params. */

No need to have this comment. The code is self-explanatory and the error
message already states that.

> +        (libxl_defbool_val(d_config->b_info.u.hvm.altp2m) ||
> +        (d_config->b_info.altp2m != LIBXL_ALTP2M_MODE_DISABLED)))) {
> +        LOG(ERROR, "nestedhvm and altp2m cannot be used together");
>          goto error_out;
>      }
>  
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index eef5045..32af757 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -291,8 +291,6 @@ static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
>                      libxl_defbool_val(info->u.hvm.vpt_align));
>      xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM,
>                      libxl_defbool_val(info->u.hvm.nested_hvm));
> -    xc_hvm_param_set(handle, domid, HVM_PARAM_ALTP2M,
> -                    libxl_defbool_val(info->u.hvm.altp2m));
>  }
>  
>  int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> @@ -301,6 +299,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>      libxl_domain_build_info *const info = &d_config->b_info;
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      char *xs_domid, *con_domid;
> +    bool altp2m_support = false;
>      int rc;
>  
>      if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
> @@ -434,6 +433,33 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>  #endif
>      }
>  
> +#if defined(__i386__) || defined(__x86_64__)
> +    /* Alternate p2m support on x86 is available only for HVM guests. */
> +    if (info->type == LIBXL_DOMAIN_TYPE_HVM)
> +        altp2m_support = true;
> +#elif defined(__arm__) || defined(__aarch64__)
> +    /* Alternate p2m support on ARM is available for all guests. */
> +    altp2m_support = true;
> +#endif

Sorry for not mentioning this earlier, can you please make a function
called libxl__arch_altp2m_supported in libxl_{x86,arm}.c?

> +
> +    if (altp2m_support) {
> +        /* The config parameter "altp2m" replaces the parameter "altp2mhvm". For
> +         * legacy reasons, both parameters are accepted on x86 HVM guests (only
> +         * "altp2m" is accepted on ARM guests).
> +         *
> +         * If the legacy field info->u.hvm.altp2m is set, activate altp2m.
> +         * Otherwise set altp2m based on the field info->altp2m. */
> +#if defined(__i386__) || defined(__x86_64__)
> +        if (info->altp2m == LIBXL_ALTP2M_MODE_DISABLED &&
> +            libxl_defbool_val(info->u.hvm.altp2m))
> +            xc_hvm_param_set(ctx->xch, domid, HVM_PARAM_ALTP2M,
> +                             libxl_defbool_val(info->u.hvm.altp2m));
> +        else
> +#endif
> +            xc_hvm_param_set(ctx->xch, domid, HVM_PARAM_ALTP2M,
> +                             info->altp2m);
> +    }
> +

Or even move this snippet and the support check to a function named
libxl__arch_set_altp2m?

Wei.

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