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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/08/16 07:12, Jan Beulich wrote:
>>>> On 05.08.16 at 19:07, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 05/08/16 14:54, Jan Beulich wrote:
>>>>>> On 05.08.16 at 15:10, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 05/08/16 12:20, Jan Beulich wrote:
>>>>> 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.
>>> Oh, good point - that makes clear why the field can't be dropped:
>>> That sysctl would break then.
>> Which domid were you referring to then?
>>
>> The domid in the xen_domctl_getdomaininfo structure clearly needs to
>> stay, but the domctl "op->domain = op->u.getdomaininfo.domain;"
>> needn't.  OTOH, as we need to copy back the entire domctl structure
>> anyway, it doesn't hurt to keep it.
> The comment was about removal of the field, not just the
> assignment. But as you did make obvious, the sysctl side needs
> it to stay.
>
>>>>> --- 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.
>>> I don't see any XSM changes mentioned in the description, there
>>> was only the XSM_OTHER related question outside the description.
>>> Anyway - the change above is what guarantees the XSM_XS_PRIV
>>> check, as invoked by xsm_domctl()'s XEN_DOMCTL_getdomaininfo
>>> case, to fall through into XSM_DM_PRIV - after all that's what the
>>> whole patch is about.
>> But the patch is about a qemu stubdom, which would be DM_PRIV, not XS_PRIV.
> The point of the patch is to _extend_ permissions of this domctl
> from XS_PRIV to DM_PRIV.

Aah - and this only exists because of the xsm_domctl() bodge with
XSM_OTHER, which actually makes getdomaininfo protected with XS_PRIV.

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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