|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case
On Thu, Aug 11, 2016 at 11:17 AM, Wei Liu <wei.liu2@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>>
>> 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.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |