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

[Julien Grall <julien@xxxxxxx>]

On 11/07/2022 17:08, Rahul Singh wrote:
> Hi Julien,
Hi Rahul,
> > On 22 Jun 2022, at 3:51 pm, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi,
> >
> > 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.
Cheers,

--
Julien Grall