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 <jul...@xen.org> 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
