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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/08/16 17:07, Jan Beulich wrote:
>>>> On 04.08.16 at 17:55, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 04/08/16 16:44, Jan Beulich wrote:
>>>>>> On 04.08.16 at 17:22, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 04/08/16 09:41, Jan Beulich wrote:
>>>>> @@ -817,14 +816,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>>>>  
>>>>>      case XEN_DOMCTL_getdomaininfo:
>>>>>      {
>>>>> -        domid_t dom = op->domain;
>>>>> -
>>>>> -        rcu_read_lock(&domlist_read_lock);
>>>>> +        domid_t dom = DOMID_INVALID;
>>>>>  
>>>>> -        for_each_domain ( d )
>>>>> -            if ( d->domain_id >= dom )
>>>>> +        if ( !d )
>>>>> +        {
>>>>> +            ret = -EINVAL;
>>>>> +            if ( op->domain >= DOMID_FIRST_RESERVED )
>>>>>                  break;
>>>>>  
>>>>> +            rcu_read_lock(&domlist_read_lock);
>>>>> +
>>>>> +            dom = op->domain;
>>>>> +            for_each_domain ( d )
>>>>> +                if ( d->domain_id >= dom )
>>>>> +                    break;
>>>>> +        }
>>>>> +
>>>>>          ret = -ESRCH;
>>>>>          if ( d == NULL )
>>>>>              goto getdomaininfo_out;
>>>>> @@ -839,6 +846,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>>>>          copyback = 1;
>>>>>  
>>>>>      getdomaininfo_out:
>>>>> +        if ( dom == DOMID_INVALID )
>>>>> +            break;
>>>> What is this hunk for?  If you fail the "op->domain >=
>>>> DOMID_FIRST_RESERVED" check we break out of the entire
>>>> XEN_DOMCTL_getdomaininfo case.
>>> If we start out with a non-NULL d, we have to avoid the
>>> rcu_read_unlock() as well as the setting of d to NULL at the
>>> end.
>> Would you mind adding a short comment to that effect?  It is certainly
>> not obvious from just looking at the code.
> How about:
>
> @@ -839,6 +846,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>          copyback = 1;
>  
>      getdomaininfo_out:
> +        /* When d was non-NULL upon entry, no cleanup is needed. */
> +        if ( dom == DOMID_INVALID )
> +            break;
> +
>          rcu_read_unlock(&domlist_read_lock);
>          d = NULL;
>          break;

Looks good.

~Andrew

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