| Message ID | 20160318192224.GA11924@char.us.oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 18.03.16 at 20:22, <konrad.wilk@oracle.com> wrote: >> > + * return the number of bytes requested for the operation. Or an >> > + * negative value if an error is encountered. >> > + */ >> > + >> > +typedef uint64_t xen_version_op_val_t; >> > +DEFINE_XEN_GUEST_HANDLE(xen_version_op_val_t); >> > + >> > +typedef void xen_version_op_buf_t; >> > +DEFINE_XEN_GUEST_HANDLE(xen_version_op_buf_t); >> >> Are these actually useful for anything? And for the various strings, > > The xen_version_op_val_t is definitly used by the toolstack. > >> wouldn't a "char" handle be more natural? > > Heh. It was char[] before but Andrew liked it as void. But that was because you used it for non string types too, wasn't it? > @@ -380,6 +388,133 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > return -ENOSYS; > } > > +static const char *capabilities_info(unsigned int *len) > +{ > + static xen_capabilities_info_t __read_mostly cached_cap; > + static unsigned int __read_mostly cached_cap_len; > + static bool_t cached; > + > + if ( unlikely(!cached) ) > + { > + arch_get_xen_caps(&cached_cap); > + cached_cap_len = strlen(cached_cap) + 1; > + cached = 1; > + } I'm sorry for noticing this only now, but without any locking this is unsafe: x86's arch_get_xen_caps() using safe_strcat() to fill the buffer, simultaneous invocations would possibly produce garbled output to all (i.e. also subsequently started) guests. Either use a real lock here, or make the guard a tristate one, which gets transitioned e.g. from 0 to -1 by the first one coming here (doing the initialization), with everyone else waiting for it to become +1 (to which the initializing party sets it once it is done). > +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg, > + unsigned int len) > +{ > + union { > + xen_version_op_val_t val; > + xen_feature_info_t fi; > + } u = {}; > + unsigned int sz = 0; > + const void *ptr = NULL; > + int rc = xsm_version_op(XSM_OTHER, cmd); > + > + /* We can safely return -EPERM! */ > + if ( rc ) > + return rc; > + > + /* > + * The HYPERVISOR_xen_version differs in that some return the value, > + * and some copy it on back on argument. We follow the same rule for all > + * sub-ops: return 0 on success, positive value of bytes returned, and > + * always copy the result in arg. Yeey sanity! > + */ > + switch ( cmd ) > + { > + case XEN_VERSION_version: > + sz = sizeof(xen_version_op_val_t); > + u.val = (xen_major_version() << 16) | xen_minor_version(); > + break; > + > + case XEN_VERSION_extraversion: > + sz = strlen(xen_extra_version()) + 1; > + ptr = xen_extra_version(); > + break; > + > + case XEN_VERSION_capabilities: > + ptr = capabilities_info(&sz); > + break; > + > + case XEN_VERSION_changeset: > + sz = strlen(xen_changeset()) + 1; > + ptr = xen_changeset(); > + break; > + > + case XEN_VERSION_platform_parameters: > + sz = sizeof(xen_version_op_val_t); > + u.val = HYPERVISOR_VIRT_START; > + break; > + > + case XEN_VERSION_get_features: > + if ( copy_from_guest(&u.fi, arg, 1) ) Afaict this is incompatible with the null handle check further down (i.e. you also need to check for a null handle here). > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -128,6 +128,9 @@ > * * VCPUOP_register_vcpu_info > * * VCPUOP_register_runstate_memory_area > * > + * HYPERVISOR_version_op > + * All generic sub-operations > + * > * > * Other notes on the ARM ABI: I don't think the extra almost blank line is warranted here. > --- a/xen/include/public/version.h > +++ b/xen/include/public/version.h > @@ -30,7 +30,15 @@ > > #include "xen.h" > > -/* NB. All ops return zero on success, except XENVER_{version,pagesize} */ > +/* > + * There are two hypercalls mentioned in here. The XENVER_ are for > + * HYPERCALL_xen_version (17), while VERSION_ are for the > + * HYPERCALL_version_op (41). > + * > + * The subops are very similar except that the later hypercall has a > + * sane interface. > + */ > + > > /* arg == NULL; returns major:minor (16:16). */ Nor is the extra blank one here. > @@ -87,6 +95,66 @@ typedef struct xen_feature_info xen_feature_info_t; > #define XENVER_commandline 9 > typedef char xen_commandline_t[1024]; > > + > + > +/* > + * The HYPERCALL_version_op has a set of sub-ops which mirror the And three consecutive blank lines are too much in any event. (If for no other reason that because that provides extremely bad patch context if a later change happened right next to these three lines.) > +/* > + * arg == char. > + * > + * The toolstack fills it out for guest consumption. It is intended to hold > + * the UUID of the guest. > + */ > +#define XEN_VERSION_guest_handle 8 So this is the place where I agree with Andrew char is not an appropriate type. A void or uint8 handle seems like what you want here. > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -751,3 +751,22 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) > return xsm_default_action(XSM_PRIV, current->domain, NULL); > } > } > + > +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op) > +{ > + XSM_ASSERT_ACTION(XSM_OTHER); > + switch ( op ) > + { > + case XEN_VERSION_version: > + case XEN_VERSION_extraversion: > + case XEN_VERSION_capabilities: > + case XEN_VERSION_platform_parameters: > + case XEN_VERSION_get_features: > + case XEN_VERSION_pagesize: > + case XEN_VERSION_guest_handle: > + /* These MUST always be accessible to any guest by default. */ > + return xsm_default_action(XSM_HOOK, current->domain, NULL); > + default: > + return xsm_default_action(XSM_PRIV, current->domain, NULL); Considering that we seem to have settled on some exceptions here for the change adding XSM check to the legacy version op, do you really think going with no exception at all here is the right approach? Because if we do, that'll prevent guests getting fully converted over to the new interface. Of course, we could also make this conversion specifically a non-goal, and omit e.g. XEN_VERSION_VERSION from this new interface. Jan
On Mon, Mar 21, 2016 at 06:45:28AM -0600, Jan Beulich wrote: > >>> On 18.03.16 at 20:22, <konrad.wilk@oracle.com> wrote: > >> > + * return the number of bytes requested for the operation. Or an > >> > + * negative value if an error is encountered. > >> > + */ > >> > + > >> > +typedef uint64_t xen_version_op_val_t; > >> > +DEFINE_XEN_GUEST_HANDLE(xen_version_op_val_t); > >> > + > >> > +typedef void xen_version_op_buf_t; > >> > +DEFINE_XEN_GUEST_HANDLE(xen_version_op_buf_t); > >> > >> Are these actually useful for anything? And for the various strings, > > > > The xen_version_op_val_t is definitly used by the toolstack. > > > >> wouldn't a "char" handle be more natural? > > > > Heh. It was char[] before but Andrew liked it as void. > > But that was because you used it for non string types too, > wasn't it? Yes. For the build-id which is a binary blob. And as you noticed - also the domain handle which can be anything. > > > @@ -380,6 +388,133 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > return -ENOSYS; > > } > > > > +static const char *capabilities_info(unsigned int *len) > > +{ > > + static xen_capabilities_info_t __read_mostly cached_cap; > > + static unsigned int __read_mostly cached_cap_len; > > + static bool_t cached; > > + > > + if ( unlikely(!cached) ) > > + { > > + arch_get_xen_caps(&cached_cap); > > + cached_cap_len = strlen(cached_cap) + 1; > > + cached = 1; > > + } > > I'm sorry for noticing this only now, but without any locking this is > unsafe: x86's arch_get_xen_caps() using safe_strcat() to fill the > buffer, simultaneous invocations would possibly produce garbled > output to all (i.e. also subsequently started) guests. Either use a > real lock here, or make the guard a tristate one, which gets > transitioned e.g. from 0 to -1 by the first one coming here (doing > the initialization), with everyone else waiting for it to become +1 > (to which the initializing party sets it once it is done). That would indeed be bad. What if an _init_ code called this to 'pre-cache' it? > > > +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg, > > + unsigned int len) > > +{ > > + union { > > + xen_version_op_val_t val; > > + xen_feature_info_t fi; > > + } u = {}; > > + unsigned int sz = 0; > > + const void *ptr = NULL; > > + int rc = xsm_version_op(XSM_OTHER, cmd); > > + > > + /* We can safely return -EPERM! */ > > + if ( rc ) > > + return rc; > > + > > + /* > > + * The HYPERVISOR_xen_version differs in that some return the value, > > + * and some copy it on back on argument. We follow the same rule for all > > + * sub-ops: return 0 on success, positive value of bytes returned, and > > + * always copy the result in arg. Yeey sanity! > > + */ > > + switch ( cmd ) > > + { > > + case XEN_VERSION_version: > > + sz = sizeof(xen_version_op_val_t); > > + u.val = (xen_major_version() << 16) | xen_minor_version(); > > + break; > > + > > + case XEN_VERSION_extraversion: > > + sz = strlen(xen_extra_version()) + 1; > > + ptr = xen_extra_version(); > > + break; > > + > > + case XEN_VERSION_capabilities: > > + ptr = capabilities_info(&sz); > > + break; > > + > > + case XEN_VERSION_changeset: > > + sz = strlen(xen_changeset()) + 1; > > + ptr = xen_changeset(); > > + break; > > + > > + case XEN_VERSION_platform_parameters: > > + sz = sizeof(xen_version_op_val_t); > > + u.val = HYPERVISOR_VIRT_START; > > + break; > > + > > + case XEN_VERSION_get_features: > > + if ( copy_from_guest(&u.fi, arg, 1) ) > > Afaict this is incompatible with the null handle check further down (i.e. > you also need to check for a null handle here). Oh my. Indeed. > > > --- a/xen/include/public/arch-arm.h > > +++ b/xen/include/public/arch-arm.h > > @@ -128,6 +128,9 @@ > > * * VCPUOP_register_vcpu_info > > * * VCPUOP_register_runstate_memory_area > > * > > + * HYPERVISOR_version_op > > + * All generic sub-operations > > + * > > * > > * Other notes on the ARM ABI: > > I don't think the extra almost blank line is warranted here. > > > --- a/xen/include/public/version.h > > +++ b/xen/include/public/version.h > > @@ -30,7 +30,15 @@ > > > > #include "xen.h" > > > > -/* NB. All ops return zero on success, except XENVER_{version,pagesize} */ > > +/* > > + * There are two hypercalls mentioned in here. The XENVER_ are for > > + * HYPERCALL_xen_version (17), while VERSION_ are for the > > + * HYPERCALL_version_op (41). > > + * > > + * The subops are very similar except that the later hypercall has a > > + * sane interface. > > + */ > > + > > > > /* arg == NULL; returns major:minor (16:16). */ > > Nor is the extra blank one here. > > > @@ -87,6 +95,66 @@ typedef struct xen_feature_info xen_feature_info_t; > > #define XENVER_commandline 9 > > typedef char xen_commandline_t[1024]; > > > > + > > + > > +/* > > + * The HYPERCALL_version_op has a set of sub-ops which mirror the > > And three consecutive blank lines are too much in any event. (If > for no other reason that because that provides extremely bad > patch context if a later change happened right next to these three > lines.) > > > +/* > > + * arg == char. > > + * > > + * The toolstack fills it out for guest consumption. It is intended to hold > > + * the UUID of the guest. > > + */ > > +#define XEN_VERSION_guest_handle 8 > > So this is the place where I agree with Andrew char is not an > appropriate type. A void or uint8 handle seems like what you > want here. /me nods. > > > --- a/xen/include/xsm/dummy.h > > +++ b/xen/include/xsm/dummy.h > > @@ -751,3 +751,22 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) > > return xsm_default_action(XSM_PRIV, current->domain, NULL); > > } > > } > > + > > +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op) > > +{ > > + XSM_ASSERT_ACTION(XSM_OTHER); > > + switch ( op ) > > + { > > + case XEN_VERSION_version: > > + case XEN_VERSION_extraversion: > > + case XEN_VERSION_capabilities: > > + case XEN_VERSION_platform_parameters: > > + case XEN_VERSION_get_features: > > + case XEN_VERSION_pagesize: > > + case XEN_VERSION_guest_handle: > > + /* These MUST always be accessible to any guest by default. */ > > + return xsm_default_action(XSM_HOOK, current->domain, NULL); > > + default: > > + return xsm_default_action(XSM_PRIV, current->domain, NULL); > > Considering that we seem to have settled on some exceptions here > for the change adding XSM check to the legacy version op, do you > really think going with no exception at all here is the right approach? > Because if we do, that'll prevent guests getting fully converted over > to the new interface. Of course, we could also make this conversion > specifically a non-goal, and omit e.g. XEN_VERSION_VERSION from > this new interface. No no. I think convesion is the right long-term goal. However the nice thing about this hypercall is that it can return -EPERM. Making it always return an value for XEN_VERSION_version, XEN_VERSION_platform_parameters, XEN_VERSION_get_features means that there are some exceptions to this "can return -EPERM" as they will be guaranteed an postive return value. They can ignore the -EPERM case. And means that guest can still take shortcuts. I agree with you that guests need these hypercalls but at the same time I am not sure what can be done so they don't fall flat on their faces if they are presented with -EPERM. The Linux xenbus_init can be modified to deal with this returning -EPERM where it makes some assumptions. But in a likelyhood it is the bad assumption! Andrew, what do you think? > > Jan
>>> On 22.03.16 at 16:52, <konrad.wilk@oracle.com> wrote: > On Mon, Mar 21, 2016 at 06:45:28AM -0600, Jan Beulich wrote: >> >>> On 18.03.16 at 20:22, <konrad.wilk@oracle.com> wrote: >> > @@ -380,6 +388,133 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> > return -ENOSYS; >> > } >> > >> > +static const char *capabilities_info(unsigned int *len) >> > +{ >> > + static xen_capabilities_info_t __read_mostly cached_cap; >> > + static unsigned int __read_mostly cached_cap_len; >> > + static bool_t cached; >> > + >> > + if ( unlikely(!cached) ) >> > + { >> > + arch_get_xen_caps(&cached_cap); >> > + cached_cap_len = strlen(cached_cap) + 1; >> > + cached = 1; >> > + } >> >> I'm sorry for noticing this only now, but without any locking this is >> unsafe: x86's arch_get_xen_caps() using safe_strcat() to fill the >> buffer, simultaneous invocations would possibly produce garbled >> output to all (i.e. also subsequently started) guests. Either use a >> real lock here, or make the guard a tristate one, which gets >> transitioned e.g. from 0 to -1 by the first one coming here (doing >> the initialization), with everyone else waiting for it to become +1 >> (to which the initializing party sets it once it is done). > > That would indeed be bad. > > What if an _init_ code called this to 'pre-cache' it? That's one of the options you have. >> > --- a/xen/include/xsm/dummy.h >> > +++ b/xen/include/xsm/dummy.h >> > @@ -751,3 +751,22 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) >> > return xsm_default_action(XSM_PRIV, current->domain, NULL); >> > } >> > } >> > + >> > +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op) >> > +{ >> > + XSM_ASSERT_ACTION(XSM_OTHER); >> > + switch ( op ) >> > + { >> > + case XEN_VERSION_version: >> > + case XEN_VERSION_extraversion: >> > + case XEN_VERSION_capabilities: >> > + case XEN_VERSION_platform_parameters: >> > + case XEN_VERSION_get_features: >> > + case XEN_VERSION_pagesize: >> > + case XEN_VERSION_guest_handle: >> > + /* These MUST always be accessible to any guest by default. */ >> > + return xsm_default_action(XSM_HOOK, current->domain, NULL); >> > + default: >> > + return xsm_default_action(XSM_PRIV, current->domain, NULL); >> >> Considering that we seem to have settled on some exceptions here >> for the change adding XSM check to the legacy version op, do you >> really think going with no exception at all here is the right approach? > >> Because if we do, that'll prevent guests getting fully converted over >> to the new interface. Of course, we could also make this conversion >> specifically a non-goal, and omit e.g. XEN_VERSION_VERSION from >> this new interface. > > No no. I think convesion is the right long-term goal. > > However the nice thing about this hypercall is that it can return -EPERM. > > Making it always return an value for XEN_VERSION_version, > XEN_VERSION_platform_parameters, XEN_VERSION_get_features means that > there are some exceptions to this "can return -EPERM" as they will > be guaranteed an postive return value. They can ignore the -EPERM > case. > > And means that guest can still take shortcuts. > > I agree with you that guests need these hypercalls but at the same > time I am not sure what can be done so they don't fall flat on their > faces if they are presented with -EPERM. The Linux xenbus_init can be > modified to deal with this returning -EPERM where it makes some assumptions. > > But in a likelyhood it is the bad assumption! I'm afraid I can't conclude what you mean to say with all of the above. Jan
> >> > --- a/xen/include/xsm/dummy.h > >> > +++ b/xen/include/xsm/dummy.h > >> > @@ -751,3 +751,22 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) > >> > return xsm_default_action(XSM_PRIV, current->domain, NULL); > >> > } > >> > } > >> > + > >> > +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op) > >> > +{ > >> > + XSM_ASSERT_ACTION(XSM_OTHER); > >> > + switch ( op ) > >> > + { > >> > + case XEN_VERSION_version: > >> > + case XEN_VERSION_extraversion: > >> > + case XEN_VERSION_capabilities: > >> > + case XEN_VERSION_platform_parameters: > >> > + case XEN_VERSION_get_features: > >> > + case XEN_VERSION_pagesize: > >> > + case XEN_VERSION_guest_handle: > >> > + /* These MUST always be accessible to any guest by default. */ > >> > + return xsm_default_action(XSM_HOOK, current->domain, NULL); > >> > + default: > >> > + return xsm_default_action(XSM_PRIV, current->domain, NULL); > >> > >> Considering that we seem to have settled on some exceptions here > >> for the change adding XSM check to the legacy version op, do you > >> really think going with no exception at all here is the right approach? > > > >> Because if we do, that'll prevent guests getting fully converted over > >> to the new interface. Of course, we could also make this conversion > >> specifically a non-goal, and omit e.g. XEN_VERSION_VERSION from > >> this new interface. > > > > No no. I think convesion is the right long-term goal. > > > > However the nice thing about this hypercall is that it can return -EPERM. > > > > Making it always return an value for XEN_VERSION_version, > > XEN_VERSION_platform_parameters, XEN_VERSION_get_features means that > > there are some exceptions to this "can return -EPERM" as they will > > be guaranteed an postive return value. They can ignore the -EPERM > > case. > > > > And means that guest can still take shortcuts. > > > > I agree with you that guests need these hypercalls but at the same > > time I am not sure what can be done so they don't fall flat on their > > faces if they are presented with -EPERM. The Linux xenbus_init can be > > modified to deal with this returning -EPERM where it makes some assumptions. > > > > But in a likelyhood it is the bad assumption! > > I'm afraid I can't conclude what you mean to say with all of the > above. That I am waffling. Andrew, what is your opinion? > > Jan >
On 22/03/16 18:57, Konrad Rzeszutek Wilk wrote: >>>>> --- a/xen/include/xsm/dummy.h >>>>> +++ b/xen/include/xsm/dummy.h >>>>> @@ -751,3 +751,22 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) >>>>> return xsm_default_action(XSM_PRIV, current->domain, NULL); >>>>> } >>>>> } >>>>> + >>>>> +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op) >>>>> +{ >>>>> + XSM_ASSERT_ACTION(XSM_OTHER); >>>>> + switch ( op ) >>>>> + { >>>>> + case XEN_VERSION_version: >>>>> + case XEN_VERSION_extraversion: >>>>> + case XEN_VERSION_capabilities: >>>>> + case XEN_VERSION_platform_parameters: >>>>> + case XEN_VERSION_get_features: >>>>> + case XEN_VERSION_pagesize: >>>>> + case XEN_VERSION_guest_handle: >>>>> + /* These MUST always be accessible to any guest by default. */ >>>>> + return xsm_default_action(XSM_HOOK, current->domain, NULL); >>>>> + default: >>>>> + return xsm_default_action(XSM_PRIV, current->domain, NULL); >>>> Considering that we seem to have settled on some exceptions here >>>> for the change adding XSM check to the legacy version op, do you >>>> really think going with no exception at all here is the right approach? >>>> Because if we do, that'll prevent guests getting fully converted over >>>> to the new interface. Of course, we could also make this conversion >>>> specifically a non-goal, and omit e.g. XEN_VERSION_VERSION from >>>> this new interface. >>> No no. I think convesion is the right long-term goal. >>> >>> However the nice thing about this hypercall is that it can return -EPERM. >>> >>> Making it always return an value for XEN_VERSION_version, >>> XEN_VERSION_platform_parameters, XEN_VERSION_get_features means that >>> there are some exceptions to this "can return -EPERM" as they will >>> be guaranteed an postive return value. They can ignore the -EPERM >>> case. >>> >>> And means that guest can still take shortcuts. >>> >>> I agree with you that guests need these hypercalls but at the same >>> time I am not sure what can be done so they don't fall flat on their >>> faces if they are presented with -EPERM. The Linux xenbus_init can be >>> modified to deal with this returning -EPERM where it makes some assumptions. >>> >>> But in a likelyhood it is the bad assumption! >> I'm afraid I can't conclude what you mean to say with all of the >> above. > That I am waffling. > > Andrew, what is your opinion? Nothing good can come from failing a XEN_VERSION_version hypercall. There are a number easy ways for a guest to infer such information. XEN_VERSION_platform_parameters is only useful for 32bit PV guests, and the toolstack. Given that it is returning a fixed number in the ABI, nothing good can come of failing this either. get_features can effectively be failed for permission reasons by returning 0. As such, explicitly failing with -EPERM is similarly pointless. ~Andrew
diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te index 18f49b5..d61e697 100644 --- a/tools/flask/policy/policy/modules/xen/xen.te +++ b/tools/flask/policy/policy/modules/xen/xen.te @@ -74,11 +74,13 @@ allow dom0_t xen_t:xen2 { get_symbol }; -# Allow dom0 to use all XENVER_ subops that have checks. +# Allow dom0 to use all XENVER_ subops that have checks and VERSION subops # Note that dom0 is part of domain_type so this has duplicates. allow dom0_t xen_t:version { xen_extraversion xen_compile_info xen_capabilities xen_changeset xen_pagesize xen_guest_handle xen_commandline + version extraversion capabilities changeset platform_parameters + get_features pagesize guest_handle commandline }; allow dom0_t xen_t:mmu memorymap; @@ -145,10 +147,13 @@ if (guest_writeconsole) { # pmu_ctrl is for) allow domain_type xen_t:xen2 pmu_use; -# For normal guests all possible except XENVER_commandline. +# For normal guests all possible except XENVER_commandline, VERSION_changeset, +# and VERSION_commandline allow domain_type xen_t:version { xen_extraversion xen_compile_info xen_capabilities xen_changeset xen_pagesize xen_guest_handle + version extraversion capabilities platform_parameters + get_features pagesize guest_handle }; ############################################################################### diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 83744e8..31d2115 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1235,6 +1235,7 @@ static arm_hypercall_t arm_hypercall_table[] = { HYPERCALL(multicall, 2), HYPERCALL(platform_op, 1), HYPERCALL_ARM(vcpu_op, 3), + HYPERCALL(version_op, 3), }; #ifndef NDEBUG diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 80d59ff..f16b590 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -5322,6 +5322,7 @@ static const struct { COMPAT_CALL(platform_op), COMPAT_CALL(mmuext_op), HYPERCALL(xenpmu_op), + HYPERCALL(version_op), HYPERCALL(arch_1) }; diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S index 33e2c12..fd25e84 100644 --- a/xen/arch/x86/x86_64/compat/entry.S +++ b/xen/arch/x86/x86_64/compat/entry.S @@ -394,6 +394,7 @@ ENTRY(compat_hypercall_table) .quad do_tmem_op .quad do_ni_hypercall /* reserved for XenClient */ .quad do_xenpmu_op /* 40 */ + .quad do_version_op .rept __HYPERVISOR_arch_0-((.-compat_hypercall_table)/8) .quad compat_ni_hypercall .endr @@ -445,6 +446,7 @@ ENTRY(compat_hypercall_args_table) .byte 1 /* do_tmem_op */ .byte 0 /* reserved for XenClient */ .byte 2 /* do_xenpmu_op */ /* 40 */ + .byte 3 /* do_version_op */ .rept __HYPERVISOR_arch_0-(.-compat_hypercall_args_table) .byte 0 /* compat_ni_hypercall */ .endr diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S index 07ef096..b0e7257 100644 --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -730,6 +730,7 @@ ENTRY(hypercall_table) .quad do_tmem_op .quad do_ni_hypercall /* reserved for XenClient */ .quad do_xenpmu_op /* 40 */ + .quad do_version_op .rept __HYPERVISOR_arch_0-((.-hypercall_table)/8) .quad do_ni_hypercall .endr @@ -781,6 +782,7 @@ ENTRY(hypercall_args_table) .byte 1 /* do_tmem_op */ .byte 0 /* reserved for XenClient */ .byte 2 /* do_xenpmu_op */ /* 40 */ + .byte 3 /* do_version_op */ .rept __HYPERVISOR_arch_0-(.-hypercall_args_table) .byte 0 /* do_ni_hypercall */ .endr diff --git a/xen/common/compat/kernel.c b/xen/common/compat/kernel.c index df93fdd..7a7ca53 100644 --- a/xen/common/compat/kernel.c +++ b/xen/common/compat/kernel.c @@ -39,6 +39,8 @@ CHECK_TYPE(capabilities_info); CHECK_TYPE(domain_handle); +CHECK_TYPE(version_op_val); + #define xennmi_callback compat_nmi_callback #define xennmi_callback_t compat_nmi_callback_t diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 06ecf26..3ea35cc 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -221,6 +221,47 @@ void __init do_initcalls(void) #endif +static int get_features(struct domain *d, xen_feature_info_t *fi) +{ + switch ( fi->submap_idx ) + { + case 0: + fi->submap = (1U << XENFEAT_memory_op_vnode_supported); + if ( paging_mode_translate(d) ) + fi->submap |= + (1U << XENFEAT_writable_page_tables) | + (1U << XENFEAT_auto_translated_physmap); + if ( is_hardware_domain(d) ) + fi->submap |= 1U << XENFEAT_dom0; +#ifdef CONFIG_X86 + if ( VM_ASSIST(d, pae_extended_cr3) ) + fi->submap |= (1U << XENFEAT_pae_pgdir_above_4gb); + switch ( d->guest_type ) + { + case guest_type_pv: + fi->submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) | + (1U << XENFEAT_highmem_assist) | + (1U << XENFEAT_gnttab_map_avail_bits); + break; + case guest_type_pvh: + fi->submap |= (1U << XENFEAT_hvm_safe_pvclock) | + (1U << XENFEAT_supervisor_mode_kernel) | + (1U << XENFEAT_hvm_callback_vector); + break; + case guest_type_hvm: + fi->submap |= (1U << XENFEAT_hvm_safe_pvclock) | + (1U << XENFEAT_hvm_callback_vector) | + (1U << XENFEAT_hvm_pirqs); + break; + } +#endif + break; + default: + return -EINVAL; + } + return 0; +} + /* * Simple hypercalls. */ @@ -298,47 +339,14 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case XENVER_get_features: { xen_feature_info_t fi; - struct domain *d = current->domain; + int rc; if ( copy_from_guest(&fi, arg, 1) ) return -EFAULT; - switch ( fi.submap_idx ) - { - case 0: - fi.submap = (1U << XENFEAT_memory_op_vnode_supported); - if ( VM_ASSIST(d, pae_extended_cr3) ) - fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb); - if ( paging_mode_translate(d) ) - fi.submap |= - (1U << XENFEAT_writable_page_tables) | - (1U << XENFEAT_auto_translated_physmap); - if ( is_hardware_domain(d) ) - fi.submap |= 1U << XENFEAT_dom0; -#ifdef CONFIG_X86 - switch ( d->guest_type ) - { - case guest_type_pv: - fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) | - (1U << XENFEAT_highmem_assist) | - (1U << XENFEAT_gnttab_map_avail_bits); - break; - case guest_type_pvh: - fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) | - (1U << XENFEAT_supervisor_mode_kernel) | - (1U << XENFEAT_hvm_callback_vector); - break; - case guest_type_hvm: - fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) | - (1U << XENFEAT_hvm_callback_vector) | - (1U << XENFEAT_hvm_pirqs); - break; - } -#endif - break; - default: - return -EINVAL; - } + rc = get_features(current->domain, &fi); + if ( rc ) + return rc; if ( __copy_to_guest(arg, &fi, 1) ) return -EFAULT; @@ -380,6 +388,133 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) return -ENOSYS; } +static const char *capabilities_info(unsigned int *len) +{ + static xen_capabilities_info_t __read_mostly cached_cap; + static unsigned int __read_mostly cached_cap_len; + static bool_t cached; + + if ( unlikely(!cached) ) + { + arch_get_xen_caps(&cached_cap); + cached_cap_len = strlen(cached_cap) + 1; + cached = 1; + } + + *len = cached_cap_len; + return cached_cap; +} + +/* + * Similar to HYPERVISOR_xen_version but with a sane interface + * (has a length, one can probe for the length) and with one less sub-ops: + * missing XENVER_compile_info. + */ +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg, + unsigned int len) +{ + union { + xen_version_op_val_t val; + xen_feature_info_t fi; + } u = {}; + unsigned int sz = 0; + const void *ptr = NULL; + int rc = xsm_version_op(XSM_OTHER, cmd); + + /* We can safely return -EPERM! */ + if ( rc ) + return rc; + + /* + * The HYPERVISOR_xen_version differs in that some return the value, + * and some copy it on back on argument. We follow the same rule for all + * sub-ops: return 0 on success, positive value of bytes returned, and + * always copy the result in arg. Yeey sanity! + */ + switch ( cmd ) + { + case XEN_VERSION_version: + sz = sizeof(xen_version_op_val_t); + u.val = (xen_major_version() << 16) | xen_minor_version(); + break; + + case XEN_VERSION_extraversion: + sz = strlen(xen_extra_version()) + 1; + ptr = xen_extra_version(); + break; + + case XEN_VERSION_capabilities: + ptr = capabilities_info(&sz); + break; + + case XEN_VERSION_changeset: + sz = strlen(xen_changeset()) + 1; + ptr = xen_changeset(); + break; + + case XEN_VERSION_platform_parameters: + sz = sizeof(xen_version_op_val_t); + u.val = HYPERVISOR_VIRT_START; + break; + + case XEN_VERSION_get_features: + if ( copy_from_guest(&u.fi, arg, 1) ) + { + rc = -EFAULT; + break; + } + sz = sizeof(xen_feature_info_t); + rc = get_features(current->domain, &u.fi); + break; + + case XEN_VERSION_pagesize: + sz = sizeof(xen_version_op_val_t); + u.val = PAGE_SIZE; + break; + + case XEN_VERSION_guest_handle: + sz = ARRAY_SIZE(current->domain->handle); + ptr = current->domain->handle; + break; + + case XEN_VERSION_commandline: + sz = strlen(saved_cmdline) + 1; + ptr = saved_cmdline; + break; + + default: + rc = -ENOSYS; + } + + if ( rc ) + return rc; + + /* + * This hypercall also allows the client to probe. If it provides + * a NULL arg we will return the size of the space it has to + * allocate for the specific sub-op. + */ + ASSERT(sz); + if ( guest_handle_is_null(arg) ) + return sz; + + if ( !rc ) + { + unsigned int bytes = min(sz, len); + + if ( copy_to_guest(arg, ptr ? : &u, bytes) ) + rc = -EFAULT; + + /* + * We return len (truncate) worth of data even if we fail. + */ + if ( !rc && sz > len ) + rc = -ENOBUFS; + } + + return rc == 0 ? sz : rc; +} + DO(nmi_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct xennmi_callback cb; diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 870bc3b..c9ae315 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -128,6 +128,9 @@ * * VCPUOP_register_vcpu_info * * VCPUOP_register_runstate_memory_area * + * HYPERVISOR_version_op + * All generic sub-operations + * * * Other notes on the ARM ABI: * diff --git a/xen/include/public/version.h b/xen/include/public/version.h index 24a582f..ebb0664 100644 --- a/xen/include/public/version.h +++ b/xen/include/public/version.h @@ -30,7 +30,15 @@ #include "xen.h" -/* NB. All ops return zero on success, except XENVER_{version,pagesize} */ +/* + * There are two hypercalls mentioned in here. The XENVER_ are for + * HYPERCALL_xen_version (17), while VERSION_ are for the + * HYPERCALL_version_op (41). + * + * The subops are very similar except that the later hypercall has a + * sane interface. + */ + /* arg == NULL; returns major:minor (16:16). */ #define XENVER_version 0 @@ -87,6 +95,66 @@ typedef struct xen_feature_info xen_feature_info_t; #define XENVER_commandline 9 typedef char xen_commandline_t[1024]; + + +/* + * The HYPERCALL_version_op has a set of sub-ops which mirror the + * sub-ops of HYPERCALL_xen_version. However this hypercall differs + * radically from the former: + * - It returns the amount of bytes returned. + * - It will return -XEN_EPERM if the guest is not permitted. + * - It will return the requested data in arg. + * - It requires an third argument (len) for the length of the + * arg. Naturally the arg has to fit the requested data otherwise + * -XEN_ENOBUFS is returned. + * + * It also offers an mechanism to probe for the amount of bytes an + * sub-op will require. Having the arg have an NULL handle will + * return the number of bytes requested for the operation. Or an + * negative value if an error is encountered. + */ + +typedef uint64_t xen_version_op_val_t; +DEFINE_XEN_GUEST_HANDLE(xen_version_op_val_t); + +/* + * arg == xen_version_op_val_t. Encoded as major:minor (31..16:15..0), while + * 63..32 are zero. + */ +#define XEN_VERSION_version 0 + +/* arg == char. Contains NUL terminated utf-8 string. */ +#define XEN_VERSION_extraversion 1 + +/* arg == char. Contains NUL terminated utf-8 string. */ +#define XEN_VERSION_capabilities 3 + +/* arg == char. Contains NUL terminated utf-8 string. */ +#define XEN_VERSION_changeset 4 + +/* arg == xen_version_op_val_t. */ +#define XEN_VERSION_platform_parameters 5 + +/* + * arg = xen_feature_info_t - shares the same structure + * as the XENVER_get_features. + */ +#define XEN_VERSION_get_features 6 + +/* arg == xen_version_op_val_t. */ +#define XEN_VERSION_pagesize 7 + +/* + * arg == char. + * + * The toolstack fills it out for guest consumption. It is intended to hold + * the UUID of the guest. + */ +#define XEN_VERSION_guest_handle 8 + +/* arg = char. Contains NUL terminated utf-8 string. */ +#define XEN_VERSION_commandline 9 + #endif /* __XEN_PUBLIC_VERSION_H__ */ /* diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index 64ba7ab..1a99929 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -115,6 +115,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); #define __HYPERVISOR_tmem_op 38 #define __HYPERVISOR_xc_reserved_op 39 /* reserved for XenClient */ #define __HYPERVISOR_xenpmu_op 40 +#define __HYPERVISOR_version_op 41 /* supersedes xen_version (17) */ /* Architecture-specific hypercall definitions. */ #define __HYPERVISOR_arch_0 48 diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h index 0c8ae0e..e8d2b81 100644 --- a/xen/include/xen/hypercall.h +++ b/xen/include/xen/hypercall.h @@ -147,6 +147,10 @@ do_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg); extern long do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg); +extern long +do_version_op(unsigned int cmd, + XEN_GUEST_HANDLE_PARAM(void) arg, unsigned int len); + #ifdef CONFIG_COMPAT extern int diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 87be9e5..71d9509 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -751,3 +751,22 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op) return xsm_default_action(XSM_PRIV, current->domain, NULL); } } + +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op) +{ + XSM_ASSERT_ACTION(XSM_OTHER); + switch ( op ) + { + case XEN_VERSION_version: + case XEN_VERSION_extraversion: + case XEN_VERSION_capabilities: + case XEN_VERSION_platform_parameters: + case XEN_VERSION_get_features: + case XEN_VERSION_pagesize: + case XEN_VERSION_guest_handle: + /* These MUST always be accessible to any guest by default. */ + return xsm_default_action(XSM_HOOK, current->domain, NULL); + default: + return xsm_default_action(XSM_PRIV, current->domain, NULL); + } +} diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index db440f6..ac80472 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -194,6 +194,7 @@ struct xsm_operations { int (*pmu_op) (struct domain *d, unsigned int op); #endif int (*xen_version) (uint32_t cmd); + int (*version_op) (uint32_t cmd); }; #ifdef CONFIG_XSM @@ -736,6 +737,12 @@ static inline int xsm_xen_version (xsm_default_t def, uint32_t op) { return xsm_ops->xen_version(op); } + +static inline int xsm_version_op (xsm_default_t def, uint32_t op) +{ + return xsm_ops->version_op(op); +} + #endif /* XSM_NO_WRAPPERS */ #ifdef CONFIG_MULTIBOOT diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index 9791ad4..776dd09 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -163,4 +163,5 @@ void xsm_fixup_ops (struct xsm_operations *ops) set_to_dummy_if_null(ops, pmu_op); #endif set_to_dummy_if_null(ops, xen_version); + set_to_dummy_if_null(ops, version_op); } diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 1a95689..7569086 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -1658,6 +1658,44 @@ static int flask_xen_version (uint32_t op) } } +static int flask_version_op (uint32_t op) +{ + u32 dsid = domain_sid(current->domain); + + switch ( op ) + { + case XEN_VERSION_version: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__VERSION, NULL); + case XEN_VERSION_extraversion: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__EXTRAVERSION, NULL); + case XEN_VERSION_capabilities: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__CAPABILITIES, NULL); + case XEN_VERSION_changeset: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__CHANGESET, NULL); + case XEN_VERSION_platform_parameters: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__PLATFORM_PARAMETERS, NULL); + case XEN_VERSION_get_features: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__GET_FEATURES, NULL); + case XEN_VERSION_pagesize: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__PAGESIZE, NULL); + case XEN_VERSION_guest_handle: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__GUEST_HANDLE, NULL); + case XEN_VERSION_commandline: + return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_VERSION, + VERSION__COMMANDLINE, NULL); + default: + return -EPERM; + } +} + long do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op); int compat_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op); @@ -1797,6 +1835,7 @@ static struct xsm_operations flask_ops = { .pmu_op = flask_pmu_op, #endif .xen_version = flask_xen_version, + .version_op = flask_version_op, }; static __init void flask_init(void) diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors index badcf1c..7b098db 100644 --- a/xen/xsm/flask/policy/access_vectors +++ b/xen/xsm/flask/policy/access_vectors @@ -496,9 +496,10 @@ class security del_ocontext } -# Class version is used to describe the XENVER_ hypercall. +# Class version is used to describe the XENVER_ and VERSION hypercall. # Almost all sub-ops are described here - in the default case all of them should -# be allowed except the XENVER_commandline. +# be allowed except the XENVER_commandline, VERSION_commandline, and +# VERSION_changeset. # # The ones that are omitted are XENVER_version, XENVER_platform_parameters, # and XENVER_get_features - as they MUST always be returned to a guest. @@ -519,4 +520,23 @@ class version xen_guest_handle # Xen command line. xen_commandline +# --- VERSION hypercall --- +# Often called by PV kernels to force an callback. + version +# Extra informations (-unstable). + extraversion +# Such as "xen-3.0-x86_64 xen-3.0-x86_32p hvm-3.0-x86_32 hvm-3.0-x86_32p hvm-3.0-x86_64". + capabilities +# Such as the virtual address of where the hypervisor resides. + platform_parameters +# Source code changeset. + changeset +# The features the hypervisor supports. + get_features +# Page size the hypervisor uses. + pagesize +# An value that the control stack can choose. + guest_handle +# Xen command line. + commandline }