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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.10.15 at 20:00, <konrad.wilk@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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).

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?

>      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.

> --- 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.

Jan


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