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

[Ian Campbell <Ian.Campbell@citrix.com>]

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