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

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

On Thu, Aug 11, 2016 at 11:50:44AM -0600, Tamas K Lengyel wrote:
> On Thu, Aug 11, 2016 at 11:17 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> > 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.
> >
> 
> That's the idea but what if the user sets altp2mhvm=2 by accident
> instead of altp2m? If we just check l being non-zero it opens the door
> for some errors.

Oh, I see where you come from now. You're afraid of people accidentally
enabling MIXED mode.

I would normally argue that people should read manpage more carefully
and not type in the wrong thing. But I suspect most people only ever use
0 or 1 in setting so keeping your code won't affect most users and on
the other hand prevents error.  Please keep your original code.

Wei.

> 
> Tamas

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