Re: [PATCH v4 03/34] xsm/xen_version: Add XSM for the xen_version hypercall

["Jan Beulich" <JBeulich@suse.com>]

>>> On 15.03.16 at 18:56, <konrad.wilk@oracle.com> wrote:
> @@ -223,12 +224,15 @@ void __init do_initcalls(void)
>  /*
>   * Simple hypercalls.
>   */
> -
>  DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)

Please retain the blank line, as it relates to more than just this
one function.

>  {
> +    bool_t deny = !!xsm_xen_version(XSM_OTHER, cmd);
> +
>      switch ( cmd )
>      {
>      case XENVER_version:
> +        if ( deny )
> +            return 0;
>          return (xen_major_version() << 16) | xen_minor_version();

To be honest, I'm now rather uncertain about this one: If a guest
can't figure out the hypervisor version, how would it be able to
adjust its behavior accordingly (e.g. use deprecated hypercalls as
needed)? IOW, other than for most/all other stuff here (the
get-features and platform-parameters sub-ops may be considered
similar to this one, see also below), I don't think allowing the
"permitted" default to be overridden makes sense here.

> @@ -274,6 +279,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>              .virt_start = HYPERVISOR_VIRT_START
>          };
>  
> +        if ( deny )
> +            params.virt_start = 0;

Guests may (validly imo) assume to get a valid address here. If you
mean to not expose the non-constant address in the compat mode
case, I could accept that. But you would then need to set the ABI
mandated __HYPERVISOR_COMPAT_VIRT_START (and retain the
constant value in the non-compat case). Our old 32-bit PV guests
would crash extremely early on boot if they got back zero here
(that's for 2.6.30 and later, and I think both you and Citrix had
derived some of their kernels from our 2.6.32 based one).

> @@ -302,6 +310,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          switch ( fi.submap_idx )
>          {
>          case 0:
> +            if ( deny )
> +                break;

I think if to be put here at all, this should go ahead of the switch(),
so that guests wouldn't be able to guess from the valid index values
which features may be available. And of course you should clear
fi.submap if you deny access, instead of leaving in it what has been
there before.

>      case XENVER_guest_handle:
> -        if ( copy_to_guest(arg, current->domain->handle,
> -                           ARRAY_SIZE(current->domain->handle)) )
> +    {
> +        xen_domain_handle_t hdl;
> +        ssize_t len;
> +
> +        if ( deny )
> +        {
> +            len = sizeof(hdl);
> +            memset(&hdl, 0, len);
> +        } else
> +            len = ARRAY_SIZE(current->domain->handle);
> +
> +        if ( copy_to_guest(arg, deny ? hdl : current->domain->handle, len ) )
>              return -EFAULT;
>          return 0;

What is this "len" handling here about? Aren't both the same type
and hence size? Perhaps, if you feel unsure about that, simply add
a respective BUILD_BUG_ON()?

> --- a/xen/include/xen/version.h
> +++ b/xen/include/xen/version.h
> @@ -12,5 +12,5 @@ unsigned int xen_minor_version(void);
>  const char *xen_extra_version(void);
>  const char *xen_changeset(void);
>  const char *xen_banner(void);
> -
> +const char *xen_deny(void);
>  #endif /* __XEN_VERSION_H__ */

Please retain the blank line.

Jan


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