Re: [PATCH v2] domctl: tighten XEN_DOMCTL_*_permission

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/08/14 12:02, Andrii Tseglytskyi wrote:
> Hi Andrew,
>
> Thank you for your comment.
>
> On Fri, Aug 15, 2014 at 1:36 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 15/08/14 11:00, Andrii Tseglytskyi wrote:
>>> Hi,
>>>
>>> I see possible issue with this patch. Can someone clarify - did I get
>>> everything correctly?
>>>
>>> On Tue, May 6, 2014 at 4:08 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> @@ -790,7 +790,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>>>
>>>>          if ( pirq >= d->nr_pirqs )
>>>>              ret = -EINVAL;
>>>> -        else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
>>>> +        else if ( !pirq_access_permitted(current->domain, pirq) ||
>>> pirq_access_permitted() checks a range. Range can be added only with
>>> pirq_permit_access() function call. The only place where
>>> pirq_permit_access() is called - is following
>>>  *else if* branch. But it will be never called -
>>> pirq_access_permitted() will return 0 if range does not exist. As
>>> result - it is impossible to add irq, even if XSM allows this.
>>> The same is true for iomem_access_permitted() function call.
>> I questioned the same issue when this patch went in.
>>
>> The argument is that, even with XSM, a domain may only permit access to
>> pirqs for which it also has permissions.
>>
>> This prevents a buggy domain builder accidentally conferring pirq access
>> for a dom0 resource, without dom0 first having conferred access to the
>> domain builder.
>>
> Okay. This sounds reasonable.
> One more question - I see that pirq_access_permitted() calls with
> current->domain pointer, which points to dom0.

In a traditional case, yes.  In an XSM case with semi-privileged
domain-builder domains, current could easily point to a domain other
than dom0.

> So, if resource belongs to dom0 - it must not belong to any other
> domain.

Why so?  Resources are not exclusive to a single domain.  PCIPassthough
is a prime example where the device strictly still belongs to dom0, but
pciback in dom0 ensures it is not being used by dom0 while it is being
used by another domain.

>  But in current implementation pirq_access_permitted() returns
> 0 if resource is not found.

Correct, and that is the point.

If the current domain does not have permissions to use a certain pirq,
it does not have permission to alter the permission of another domain,
with respect to this pirq.  Note that by permitting/denying access to a
target domain, the current domain does not loose its existing permissions.

> In other words - resource does not belong to dom0, but -EPERM error
> will be returned anyway.

The resource may not strictly belong to dom0, but dom0 still retains
permissions to use it, and permissions to revoke access in the future if
it chooses.

~Andrew