Re: [PATCH v3 1/3] xsm/xen_version: Add XSM for the xen_version hypercall (v6).

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

>>> On 08.01.16 at 03:25, <konrad.wilk@oracle.com> wrote:
> @@ -226,9 +227,10 @@ void __init do_initcalls(void)
>  /*
>   * Simple hypercalls.
>   */
> -
>  DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> +    bool_t deny = !!xsm_version_op(XSM_OTHER, cmd);
> +
>      switch ( cmd )
>      {
>      case XENVER_version:
> @@ -354,10 +356,17 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          return 0;
>  
>      case XENVER_commandline:
> -        if ( copy_to_guest(arg, saved_cmdline, ARRAY_SIZE(saved_cmdline)) )
> +    {
> +        size_t len = ARRAY_SIZE(saved_cmdline);
> +
> +        if ( deny )
> +            len = strlen(xen_deny()) + 1;
> +
> +        if ( copy_to_guest(arg, deny ? xen_deny() : saved_cmdline, len) )
>              return -EFAULT;
>          return 0;
>      }
> +    }
>  
>      return -ENOSYS;
>  }

As said before, I don't think it is appropriate for "deny" to be
ignored for all other sub-ops when there is a designated policy.

> --- a/xen/common/version.c
> +++ b/xen/common/version.c
> @@ -55,3 +55,8 @@ const char *xen_banner(void)
>  {
>      return XEN_BANNER;
>  }
> +
> +const char *xen_deny(void)
> +{
> +    return "<denied>\0";
> +}

There's still this strange extra NUL character here.

> @@ -1621,6 +1622,28 @@ static int flask_pmu_op (struct domain *d, unsigned int op)
>  }
>  #endif /* CONFIG_X86 */
>  
> +static int flask_version_op (uint32_t op)
> +{
> +    u32 dsid = domain_sid(current->domain);
> +
> +    switch ( op )
> +    {
> +    case XENVER_version:
> +    case XENVER_extraversion:
> +    case XENVER_compile_info:
> +    case XENVER_capabilities:
> +    case XENVER_changeset:
> +    case XENVER_platform_parameters:
> +    case XENVER_get_features:
> +    case XENVER_pagesize:
> +    case XENVER_guest_handle:
> +        return 0; /* These MUST always be accessible to guests. */
> +    default:
> +        return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_XEN2,
> +                            XEN2__VERSION_PRIV, NULL);
> +    }
> +}

And along with the comment above, I don't think there should be
a switch statement here, but instead "op" should be subjected to
policy restrictions.

Jan