Re: [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

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

On Wed, Aug 10, 2016 at 09:00:15AM -0600, Tamas K Lengyel wrote:
> 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>
> ---
> 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
> ---
>  docs/man/xl.cfg.pod.5.in        | 31 +++++++++++++++++++++++++++++++
>  tools/libxl/libxl_create.c      |  5 ++---
>  tools/libxl/libxl_dom.c         |  2 +-
>  tools/libxl/libxl_types.idl     |  9 ++++++++-
>  tools/libxl/xl_cmdimpl.c        | 34 +++++++++++++++++++++++++++++++++-
>  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 +-
>  10 files changed, 109 insertions(+), 24 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index 48c9c0d..0044b98 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 for hvm guests.
> +Alternate-p2m allows a guest to manage multiple p2m guest physical
> +"memory views" (as opposed to a single p2m). 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.
> +
> +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.
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 04f8ae9..c9076da 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -319,7 +319,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          libxl_defbool_setdefault(&b_info->u.hvm.hpet,               true);
>          libxl_defbool_setdefault(&b_info->u.hvm.vpt_align,          true);
>          libxl_defbool_setdefault(&b_info->u.hvm.nested_hvm,         false);
> -        libxl_defbool_setdefault(&b_info->u.hvm.altp2m,             false);
>          libxl_defbool_setdefault(&b_info->u.hvm.usb,                false);
>          libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
>  
> @@ -918,8 +917,8 @@ 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");
> +         d_config->b_info.u.hvm.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..0b8b2a8 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -292,7 +292,7 @@ static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
>      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));
> +                     info->u.hvm.altp2m);
>  }
>  
>  int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index ef614be..97948fd 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -439,6 +439,13 @@ libxl_rdm_reserve = Struct("rdm_reserve", [
>      ("policy",      libxl_rdm_reserve_policy),
>      ])
>  
> +# Consistent with the values defined for HVM_PARAM_ALTP2M
> +libxl_altp2m_mode = Enumeration("altp2m_mode", [
> +    (0, "disabled"),
> +    (1, "mixed"),
> +    (2, "external_only"),
> +    ], init_val = "LIBXL_ALTP2M_MODE_DISABLED")
> +
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
>      ("avail_vcpus",     libxl_bitmap),
> @@ -512,7 +519,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("mmio_hole_memkb",  MemKB),
>                                         ("timer_mode",       libxl_timer_mode),
>                                         ("nested_hvm",       libxl_defbool),
> -                                       ("altp2m",           libxl_defbool),
> +                                       ("altp2m",           libxl_altp2m_mode),

This is a breaking change.

Let me think a bit how to make it backward compatible.

>                                         ("smbios_firmware",  string),
>                                         ("acpi_firmware",    string),
>                                         ("hdtype",           libxl_hdtype),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 7f961e3..a655296 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1667,7 +1667,39 @@ static void parse_config_data(const char *config_source,
>  
>          xlu_cfg_get_defbool(config, "nestedhvm", &b_info->u.hvm.nested_hvm, 0);
>  
> -        xlu_cfg_get_defbool(config, "altp2mhvm", &b_info->u.hvm.altp2m, 0);
> +        /*
> +         * The config parameter "altp2mhvm" is considered deprecated, however
> +         * further considered because of legacy reasons. The config parameter
> +         * "altp2m" shall be used instead.
> +         */
> +        if (!xlu_cfg_get_long(config, "altp2mhvm", &l, 0)) {
> +            fprintf(stderr, "WARNING: Specifying \"altp2mhvm\" is deprecated. "
> +                    "Please use a \"altp2m\" instead.\n");

You might want to delete this check, and ...

> +
> +            if (l < LIBXL_ALTP2M_MODE_DISABLED ||
> +                l > LIBXL_ALTP2M_MODE_MIXED) {
> +                fprintf(stderr, "ERROR: invalid value %ld for \"altp2mhvm\"\n", l);
> +                exit (1);
> +            }
> +
> +            b_info->u.hvm.altp2m = l;

       b_info->u.hvm.altp2m = l ? LIBXL_ALTP2M_MIXED : LIBXL_ALTP2M_DISABLED;

Assuming that's the original semantics of this option, i.e. zero means
disabled, non-zero value means MIXED mode.

Wei.

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