[Xen-devel] Re: [PATCH 2/2] xen/gnt{dev, alloc}: reserve event channels for notify

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 10/25/2011 03:02 PM, Konrad Rzeszutek Wilk wrote:
>>> And cause the event channel refcnt to be set to zero and free it. And then
>>> causing the box to die - as the event channels for the physical IRQ might 
>>> have
>>> gotten free-ed.
>>>
>>
>> Not really. For a given valid event channel E, this will increase the refcnt 
>> by one
>> when i == E, and then decrease refcnt the next time evtchn_get succeeds (for 
>> some
>> other value of i).
> 
> Oh right. Hmm.. I am having this feeling that it still makes sense to 
> seperate the
> events that are allocated by grantdev/grantalloc from the ones that are done
> for in-kernel uses (such as IRQ, MSI, IPI, etc). Basically not trusting the 
> userland
> with its arguments as much as possible.
> 
> And yes, I do understand that you need to be a root user to use /dev/gnt*, but
> I started thinking about QEMU. And Fedora has this concept of making QEMU run 
> in its
> own SELinux container (and own user) - or perhaps I am confusing this with 
> containers..
> Anyhow it runs in one of those quasi-root-but-not-root. My thinking is that 
> it could
> be possible do with QEMU running under Xen too, but then we have to make sure
> that all /dev/gnt* ioctls are secure <hand-waving what secure means>.
> 
> It probably involves more than just what we discussed.

That same SELinux category-based isolation mechanism is also a good solution 
for xen
qemu-dm processes, although moving qemu to a stubdom provides better isolation 
since
SELinux currently cannot talk to XSM to determine what domains a particular 
qemu-dm 
process should be able to manipulate.

Only allowing event channels allocated by userspace to be used in gnt* notify is
a good idea, since there's no reason for userspace to need to manipulate an 
event
channel set up by the kernel.

>>
>>> Hm.. Perhaps the gntalloc and gntdev should keep track of which event 
>>> channels
>>> are OK to refcnt? Something like a whitelist? Granted at that point the 
>>> refcounting
>>> could as well be done by the API that sets up the event channels from the 
>>> userspace.
>>
>> Hmm. Perhaps have a magic value for refcount (-1?) that indicates evtchn_get 
>> is not
>> available. That would become the default value of refcnt, and evtchn.c would 
>> then
>> use evtchn_make_refcounted() to change the refcount to 1 and allow _get/_put 
>> to work.
> 
> How would that work when the IRQ subsystem (so everything is setup in the 
> kernel)
> gets an event? Would the refcount be for that -1.. oh. You would only set
> the refcnt when the _get/_put calls are made and not when in-kernel calls to 
> setup
> IRQs are done?
> 

Right. The reference count would be a dual-purpose field indicating if the event
channel is kernel-internal (value -1) or userspace-visible (reference count > 
0).
New event channels would start out at -1, and evtchn.c would change them to 1.

>>
>>> So the evtchn_ioctl is pretty smart. It uses "get_port_user" to get the list
>>> of events that belong to this user (and have been handed out). I think you
>>> need to use that in the gntalloc to double-check that the event channel is 
>>> not
>>> one of the kernel type.
>>>
>>>> +  }
>>>>  
>>>>    gref->notify.flags = 0;
>>>>  
>>>> @@ -396,6 +398,16 @@ static long gntalloc_ioctl_unmap_notify(struct 
>>>> gntalloc_file_private_data *priv,
>>>>            goto unlock_out;
>>>>    }
>>>>  
>>>> +  if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
>>>> +          if (evtchn_get(op.event_channel_port)) {
>>>> +                  rc = -EINVAL;
>>>> +                  goto unlock_out;
>>>> +          }
>>>> +  }
>>>> +
>>>> +  if (gref->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
>>>> +          evtchn_put(gref->notify.event);
>>>> +
>>>>    gref->notify.flags = op.action;
>>>>    gref->notify.pgoff = pgoff;
>>>>    gref->notify.event = op.event_channel_port;
>>>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
>>>> index f914b26..cfcc890 100644
>>>> --- a/drivers/xen/gntdev.c
>>>> +++ b/drivers/xen/gntdev.c
>>>> @@ -190,6 +190,7 @@ static void gntdev_put_map(struct grant_map *map)
>>>>  
>>>>    if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) {
>>>>            notify_remote_via_evtchn(map->notify.event);
>>>> +          evtchn_put(map->notify.event);
>>>>    }
>>>>  
>>>>    if (map->pages) {
>>>> @@ -596,6 +597,16 @@ static long gntdev_ioctl_notify(struct gntdev_priv 
>>>> *priv, void __user *u)
>>>>            goto unlock_out;
>>>>    }
>>>>  
>>>> +  if (op.action & UNMAP_NOTIFY_SEND_EVENT) {
>>>> +          if (evtchn_get(op.event_channel_port)) {
>>>> +                  rc = -EINVAL;
>>>> +                  goto unlock_out;
>>>> +          }
>>>> +  }
>>>> +
>>>> +  if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT)
>>>
>>> So notify.flags has not been set yet? That looks to be done later?
>>
>> Yep. It's the previous value (zero if we haven't called the ioctl yet).
> 
> OK, can you add a tiny comment so that in a year time the person reading this
> will have a warm fuzzy feeling..

OK

>>
>>> Or is this in case of the user doing
>>>
>>>  uargs.action = UNMAP_NOTIFY_SEND_EVENT;
>>>  ioctl(.., IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
>>>  uargs.action = UNAMP_NOTIFY_CLEAR_BYTE;
>>>  ioctl(.., IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &uarg);
>>>
>>>  and we want to preserve the "old" flags before swapping over to the
>>> new?
>>
>> No. We acquire the new event channel before releasing the old one so that
>> if we happen to be the only one holding a reference to this event channel,
>> a change in the byte-clear portion of the notify does not cause us to drop
>> the event channel.
> 
> Ok.
>>
>>>> +          evtchn_put(map->notify.event);
>>>> +
>>>>    map->notify.flags = op.action;
>>>>    map->notify.addr = op.index - (map->index << PAGE_SHIFT);
>>>>    map->notify.event = op.event_channel_port;
>>>> -- 
>>>> 1.7.6.4

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel