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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
>> CC: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> CC: Keir Fraser <keir@xxxxxxx>
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Tim Deegan <tim@xxxxxxx>
>> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
>> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
>> ---
>>  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);
>



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