On Thu, Oct 29, 2015 at 02:55:25AM -0600, Jan Beulich wrote:
> >>> On 28.10.15 at 20:00, <konrad.w...@oracle.com> wrote:
> > On Wed, Oct 28, 2015 at 11:42:41AM -0400, Konrad Rzeszutek Wilk wrote:
> >> Perhaps an another option would be to return success and fill out the
> >> value with an empty string?
> >>
> >> That actually sounds nicer.
>
> I disagree. You still change the ABI this way, the more that ...
>
> > Like this:
> >
> > From f5672c4b72361132798c0ec4bd124c9ddc80bd44 Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> > Date: Mon, 28 Sep 2015 09:00:58 -0400
> > Subject: [PATCH] xsm/libxl/xen_version: Add XSM for the xen_version
> > hypercall.
> >
> > All of XENVER_* have now an XSM check.
> >
> > The XENVER_[compile_info|changeset|commandline] are now
> > guarded by an XSM check for priviliged domains.
>
> ... this matches what the patch does only in the dummy case (the
> full policy case may yield any kind of behavior).
<nods>
>
> Nevertheless a couple of comments on the patch itself:
>
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -5288,6 +5288,8 @@ const libxl_version_info*
> > libxl_get_version_info(libxl_ctx *ctx)
> > info->virt_start = u.p_parms.virt_start;
> >
> > info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
> > + if (!info->pagesize) /* No divide by zero! */
> > + info->pagesize = 1;
>
> I can't see any reason whatsoever to hide the page size from guests.
>
> > DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> > {
> > + int empty_data = xsm_version_op(XSM_HOOK, cmd);
>
> The variable name kind of suggests it to have boolean meaning, and its
> uses below don't help at all making clear that's not the case. Perhaps
> better to make it bool_t and use !! above?
<nodes>
>
> > switch ( cmd )
> > {
> > case XENVER_version:
> > + if ( empty_data )
> > + return 0;
> > return (xen_major_version() << 16) | xen_minor_version();
>
> Another part I can't see a reason to hide. In fact, this may break
> guests which adapt their behavior (use of certain hypercalls)
> depending on hypervisor version.
>
> > @@ -277,6 +286,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void)
> > arg)
> > .virt_start = HYPERVISOR_VIRT_START
> > };
> >
> > + if ( empty_data )
> > + params.virt_start = 0;
>
> This again may break guests (wanting to determine how much of the
> address space to leave untouched). Our kernels use this (albeit with
> proper error checking, so they wouldn't stop working, they just
> would waste address space).
>
> > @@ -302,9 +315,14 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void)
> > arg)
> > if ( copy_from_guest(&fi, arg, 1) )
> > return -EFAULT;
> >
> > + if ( empty_data )
> > + memset(&fi, 0, sizeof(fi));
> > +
> > switch ( fi.submap_idx )
> > {
> > case 0:
> > + if ( empty_data )
> > + break;
> > fi.submap = (1U << XENFEAT_memory_op_vnode_supported);
> > if ( VM_ASSIST(d, pae_extended_cr3) )
> > fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
>
> This one, afaict, is _specifically_ meant to be available to everyone.
OK, so we go back to that some of the subops should _not_ be behind
an XSM check as they are meant to be available to everyone.
Or rather - there is no point of an XSM check at all for those.
>
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -720,4 +720,27 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG
> > struct domain *d, unsigned int
> > }
> > }
> >
> > +#include <public/version.h>
> > +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op)
> > +{
> > + XSM_ASSERT_ACTION(XSM_HOOK);
> > + switch ( op )
> > + {
> > + case XENVER_compile_info:
> > + case XENVER_changeset:
> > + case XENVER_commandline:
>
> I'd expect these three to be replaced by default: - all subops should
> always be accessible to privileged domains.
/me nods.
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
