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

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

[Andrew Cooper]

This is RFC because explicitly changes the logic introduced by c/s b34f2c375
"xsm: label xen-consumer event channels", and is only compile tested.

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>
CC: 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>
---
 xen/common/event_channel.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index cfe4978..89a7d99 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1160,11 +1160,13 @@ int alloc_unbound_xen_event_channel(
     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);
-- 
1.7.10.4

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

[Ian Campbell]

On Mon, 2015-01-12 at 10:03 +0000, Andrew Cooper wrote:
> This is RFC because explicitly changes the logic introduced by c/s b34f2c375
> "xsm: label xen-consumer event channels", and is only compile tested.
> 
> 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>
> CC: 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>
> ---
>  xen/common/event_channel.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index cfe4978..89a7d99 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1160,11 +1160,13 @@ int alloc_unbound_xen_event_channel(
>      chn = evtchn_from_port(d, port);
>  
>      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, remote_domid);
> +    if ( rc )
> +        goto out;

out here appears to return port, not rc so you aren't returning failure,
but an even more half setup port than before.

And I think you need to free the port on failure too.

>  
>      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);

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

[Andrew Cooper]

On 12/01/15 10:22, Ian Campbell wrote:
> On Mon, 2015-01-12 at 10:03 +0000, Andrew Cooper wrote:
>> This is RFC because explicitly changes the logic introduced by c/s b34f2c375
>> "xsm: label xen-consumer event channels", and is only compile tested.
>>
>> 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>
>> CC: 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>
>> ---
>>  xen/common/event_channel.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>> index cfe4978..89a7d99 100644
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -1160,11 +1160,13 @@ int alloc_unbound_xen_event_channel(
>>      chn = evtchn_from_port(d, port);
>>  
>>      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, remote_domid);
>> +    if ( rc )
>> +        goto out;
> out here appears to return port, not rc so you aren't returning failure,
> but an even more half setup port than before.

Ah yes - so I am.  rc was intended.  All callers do correctly check for
< 0 to indicate failure.

>
> And I think you need to free the port on failure too.

At the point that we bail, chn->state is still ECS_FREE so there is
nothing to deallocate.

~Andrew

>
>>  
>>      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);
>

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

[Daniel De Graaf]

On 01/12/2015 05:03 AM, Andrew Cooper wrote:
> This is RFC because explicitly changes the logic introduced by c/s b34f2c375
> "xsm: label xen-consumer event channels", and is only compile tested.
>
> 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>

This feature was originally required in order to support HVM domains which
are created by a (non-dom0) toolstack domain which does not itself provide
any services to the HVM domain.  During creation, the domain would create
the ioreq event channels to the toolstack, which was not permitted by the
XSM policy; masking this error allowed the channels to be created and then
replaced (correctly) when the device model domain was set.

 From my current inspection, this workaround may no longer be needed.  In
any case, I think it is better to expose the error and force the caller
to explicitly request a dummy event channel (or just postpone creation).

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

-- 
Daniel De Graaf
National Security Agency