Re: [Xen-devel] [PATCH v2] domctl: relax getdomaininfo permissions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/08/16 12:20, Jan Beulich wrote:
> Qemu needs access to this for the domain it controls, both due to it
> being used by xc_domain_memory_mapping() (which qemu calls) and the
> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend
> permissions to that of any "ordinary" domctl: A domain controlling the
> targeted domain can invoke this operation for that target domain (which
> is being achieved by no longer passing NULL to xsm_domctl()).
>
> This at once avoids a for_each_domain() loop when the ID of an
> existing domain gets passed in.
>
> Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Add a comment. Clarify description as to what additional permission
>     is being granted.
> ---
> I know there had been an alternative patch suggestion, but that one
> doesn't seem have seen a formal submission so far, so here is my
> original proposal.
>
> I wonder what good the duplication of the returned domain ID does: I'm
> tempted to remove the one in the command-specific structure. Does
> anyone have insight into why it was done that way?

I wonder whether the first incarnation of this hypercall lacked a domid
field in the returned structure?  It seems like the kind of thing which
would be omitted, until the sysctl list version got introduced.

>
> I further wonder why we have XSM_OTHER: The respective conversion into
> other XSM_* values in xsm/dummy.h could as well move into the callers,
> making intentions more obvious when looking at the actual code.
>
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act
>          return 0;
>      case XSM_TARGET:
>          if ( src == target )
> +        {
>              return 0;
> +    case XSM_XS_PRIV:
> +            if ( src->is_xenstore )
> +                return 0;
> +        }
>          /* fall through */
>      case XSM_DM_PRIV:
>          if ( target && src->target == target )
> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act
>          if ( src->is_privileged )
>              return 0;
>          return -EPERM;
> -    case XSM_XS_PRIV:
> -        if ( src->is_xenstore || src->is_privileged )
> -            return 0;
> -        return -EPERM;
>      default:
>          LINKER_BUG_ON(1);
>          return -EPERM;

What is this change in relation to?  I can't see how it is related to
the XSM changes mentioned in the commit, as that is strictly for the use
of XSM_OTHER.

~Andrew

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