Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.07.2022 19:28, Julien Grall wrote:
> On 11/07/2022 17:08, Rahul Singh wrote:
>>> On 22 Jun 2022, at 3:51 pm, Julien Grall <julien@xxxxxxx> wrote:
>>> On 22/06/2022 15:37, Rahul Singh wrote:
>>>> evtchn_alloc_unbound() always allocates the next available port. Static
>>>> event channel support for dom0less domains requires allocating a
>>>> specified port.
>>>> Modify the evtchn_alloc_unbound() to accept the port number as an
>>>> argument and allocate the specified port if available. If the port
>>>> number argument is zero, the next available port will be allocated.
>>>
>>> I haven't yet fully reviewed this series. But I would like to point out 
>>> that this opening a security hole (which I thought I had mention before) 
>>> that could be exploited by a guest at runtime.
>>>
>>> You would need [1] or similar in order to fix the issue. I am wrote 
>>> "similar" because the patch could potentially be a problem if you allow a 
>>> guest to use FIFO (you may need to allocate a lot of memory to fill the 
>>> hole).
>>>
>>> Cheers,
>>>
>>> [1] 
>>> https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commit;h=2d89486fcf11216331e58a21b367b8a9be1af725
>>
>> Thanks for sharing the patch.  If you are okay I can use this patch in next 
>> version to fix the security hole.
> 
> I am fine with that.
> 
>>
>> For the FIFO issue, we can introduce the new config option to restrict the 
>> maximum number of static
>> port supported in Xen. We can check the user-defined static port when we 
>> parse the device tree and if
>> a user-defined static port is greater than the maximum allowed static port 
>> will return an error to the user.
>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>
>> Let me know your view on this.
>>
>> config MAX_STATIC_PORT
>>      int "Maximum number of static ports”
>>      range 1 4095
>>      help
>>         Controls the build-time maximum number of static port supported
> 
> The problem is not exclusive to the static event channel. So I don't 
> think this is right to introduce MAX_STATIC_PORT to mitigate the issue 
> (even though this is the only user today).
> 
> A few of alternative solutions:
>    1) Handle preemption in alloc_evtchn_bucket()
>    2) Allocate all the buckets when the domain is created (the max 
> numbers event channel is known). We may need to think about preemption
>    3) Tweak is_port_valid() to check if the bucket is valid. This would 
> introduce a couple of extra memory access (might be OK as the bucket 
> would be accessed afterwards) and we would need to update some users.
> 
> At the moment, 3) is appealing me the most. I would be interested to 
> have an opionions from the other maintainers.

Fwiw of the named alternatives I would also prefer 3. Whether things
really need generalizing at this point I'm not sure, though.

Jan