Re: [PATCH v1] Add build-id to XENVER hypercall.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Oct 29, 2015 at 02:55:25AM -0600, Jan Beulich wrote:
> >>> On 28.10.15 at 20:00, <konrad.wilk@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.wilk@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
>