[PATCH] xsm/evtchn: Never pretend to have successfully created a Xen event channel

[PATCH] xsm/evtchn: Never pretend to have successfully created a Xen event channel

[Andrew Cooper]

Xen event channels are not internal resources.  They still have one end in a
domain, and are created at the request of privileged domains.  This logic
which "successfully" creates a Xen event channel opens up undesirable failure
cases with ill-specified XSM policies.

If a domain is permitted to create ioreq servers or memevent listeners, but
not to create event channels, the ioreq/memevent creation will succeed but
attempting to bind the returned event channel will fail without any indication
of a permission error.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

I have tested this with and without XSM compiled into Xen, with an
all-powerful dom0.  I have not experimented with a semi-privileged domain
builder or stub qemu domain.
---
 xen/common/event_channel.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 7d6de54..eece46b 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1155,21 +1155,25 @@ int alloc_unbound_xen_event_channel(
 
     spin_lock(&d->event_lock);
 
-    if ( (port = get_free_port(d)) < 0 )
+    rc = get_free_port(d);
+    if ( rc < 0 )
         goto out;
+    port = rc;
     chn = evtchn_from_port(d, port);
 
     rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, remote_domid);
+    if ( rc )
+        goto out;
 
     chn->state = ECS_UNBOUND;
     chn->xen_consumer = get_xen_consumer(notification_fn);
     chn->notify_vcpu_id = local_vcpu->vcpu_id;
-    chn->u.unbound.remote_domid = !rc ? remote_domid : DOMID_INVALID;
+    chn->u.unbound.remote_domid = remote_domid;
 
  out:
     spin_unlock(&d->event_lock);
 
-    return port;
+    return rc < 0 ? rc : port;
 }
 
 
-- 
1.7.10.4

Re: [PATCH] xsm/evtchn: Never pretend to have successfully created a Xen event channel

[Jan Beulich]

>>> On 19.01.15 at 11:45, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1155,21 +1155,25 @@ int alloc_unbound_xen_event_channel(
>  
>      spin_lock(&d->event_lock);
>  
> -    if ( (port = get_free_port(d)) < 0 )
> +    rc = get_free_port(d);
> +    if ( rc < 0 )
>          goto out;
> +    port = rc;

While this is kind of unrelated to the real change done in this patch,
I'm fine with it being here, but I wonder whether this shouldn't be
accompanied by a type change of "port" (to evtchn_port_t).

Jan

Re: [PATCH] xsm/evtchn: Never pretend to have successfully created a Xen event channel

[Andrew Cooper]

On 19/01/15 11:37, Jan Beulich wrote:
>>>> On 19.01.15 at 11:45, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -1155,21 +1155,25 @@ int alloc_unbound_xen_event_channel(
>>  
>>      spin_lock(&d->event_lock);
>>  
>> -    if ( (port = get_free_port(d)) < 0 )
>> +    rc = get_free_port(d);
>> +    if ( rc < 0 )
>>          goto out;
>> +    port = rc;
> While this is kind of unrelated to the real change done in this patch,
> I'm fine with it being here, but I wonder whether this shouldn't be
> accompanied by a type change of "port" (to evtchn_port_t).

There are a number of other places which could also do with similar
typechanges for port (evtchn_from_port(), port_is_valid() etc).  I would
suggest that any such change be a separate patch.

~Andrew

Re: [PATCH] xsm/evtchn: Never pretend to have successfully created a Xen event channel

[Jan Beulich]

>>> On 19.01.15 at 12:41, <andrew.cooper3@citrix.com> wrote:
> On 19/01/15 11:37, Jan Beulich wrote:
>>>>> On 19.01.15 at 11:45, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -1155,21 +1155,25 @@ int alloc_unbound_xen_event_channel(
>>>  
>>>      spin_lock(&d->event_lock);
>>>  
>>> -    if ( (port = get_free_port(d)) < 0 )
>>> +    rc = get_free_port(d);
>>> +    if ( rc < 0 )
>>>          goto out;
>>> +    port = rc;
>> While this is kind of unrelated to the real change done in this patch,
>> I'm fine with it being here, but I wonder whether this shouldn't be
>> accompanied by a type change of "port" (to evtchn_port_t).
> 
> There are a number of other places which could also do with similar
> typechanges for port (evtchn_from_port(), port_is_valid() etc).  I would
> suggest that any such change be a separate patch.

One can view it both ways - you actively decouple rc and port here,
so this could be a good opportunity to change that type. I know
there are many other places (in fact the file doesn't appear to be
making much - if any - use of the correct type at all), but those
could all be fixed when the respective code gets eventually touched
anyway. But of course, a single cleanup patch is also going to be
welcome.

Jan