Re: [PATCH 2/2] evtchn/fifo: don't enforce higher than necessary alignment

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Wed, 15 Jul 2020, 12:17 Jan Beulich, <jbeulich@xxxxxxxx> wrote:

Neither the code nor the original commit provide any justification for
the need to 8-byte align the struct in all cases. Enforce just as much
alignment as the structure actually needs - 4 bytes - by using alignof()
instead of a literal number.

Take the opportunity and also
- add so far missing validation that native and compat mode layouts of
  the structures actually match,
- tie sizeof() expressions to the types of the fields we're actually
  after, rather than specifying the type explicitly (which in the
  general case risks a disconnect, even if there's close to zero risk in
  this particular case),
- use ENXIO instead of EINVAL for the two cases of the address not
  satisfying the requirements, which will make an issue here better
  stand out at the call site.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
I question the need for the array_index_nospec() here. Or else I'd
expect map_vcpu_info() would also need the same.

--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -504,6 +504,16 @@ static void setup_ports(struct domain *d
     }
 }

+#ifdef CONFIG_COMPAT
+
+#include <compat/event_channel.h>
+
+#define xen_evtchn_fifo_control_block evtchn_fifo_control_block
+CHECK_evtchn_fifo_control_block;
+#undef xen_evtchn_fifo_control_block
+
+#endif
+
 int evtchn_fifo_init_control(struct evtchn_init_control *init_control)
 {
     struct domain *d = current->domain;
@@ -523,19 +533,20 @@ int evtchn_fifo_init_control(struct evtc
         return -ENOENT;

     /* Must not cross page boundary. */
-    if ( offset > (PAGE_SIZE - sizeof(evtchn_fifo_control_block_t)) )
-        return -EINVAL;
+    if ( offset > (PAGE_SIZE - sizeof(*v->evtchn_fifo->control_block)) )
+        return -ENXIO;

     /*
      * Make sure the guest controlled value offset is bounded even during
      * speculative execution.
      */
     offset = array_index_nospec(offset,
-                           PAGE_SIZE - sizeof(evtchn_fifo_control_block_t) + 1);
+                                PAGE_SIZE -
+                                sizeof(*v->evtchn_fifo->control_block) + 1);

-    /* Must be 8-bytes aligned. */
-    if ( offset & (8 - 1) )
-        return -EINVAL;
+    /* Must be suitably aligned. */
+    if ( offset & (alignof(*v->evtchn_fifo->control_block) - 1) )
+        return -ENXIO;

A guest relying on this new alignment wouldn't work on older version of Xen. So I don't think a guest would ever be able to use it.

Therefore is it really worth the change?

     spin_lock(&d->event_lock);

--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -46,6 +46,7 @@
 ?      evtchn_bind_vcpu                event_channel.h
 ?      evtchn_bind_virq                event_channel.h
 ?      evtchn_close                    event_channel.h
+?      evtchn_fifo_control_block       event_channel.h
 ?      evtchn_op                       event_channel.h
 ?      evtchn_send                     event_channel.h
 ?      evtchn_status                   event_channel.h