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.

While relaxation of the requirements is intended here, the primary goal
is to simply get rid of the hard coded number as well its lack of
connection to the structure that is is meant to apply to.

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@suse.com>
---
v3: Adjust version in public header comment.
v2: Add comment to public header. Re-base.
---
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
@@ -567,6 +567,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;
@@ -586,19 +596,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;
 
     spin_lock(&d->event_lock);
 
--- a/xen/include/public/event_channel.h
+++ b/xen/include/public/event_channel.h
@@ -368,6 +368,11 @@ typedef uint32_t event_word_t;
 
 #define EVTCHN_FIFO_NR_CHANNELS (1 << EVTCHN_FIFO_LINK_BITS)
 
+/*
+ * While this structure only requires 4-byte alignment, Xen versions 4.15 and
+ * earlier reject offset values (in struct evtchn_init_control) that aren't a
+ * multiple of 8.
+ */
 struct evtchn_fifo_control_block {
     uint32_t ready;
     uint32_t _rsvd;
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -67,6 +67,7 @@
 ?	evtchn_bind_virq		event_channel.h
 ?	evtchn_close			event_channel.h
 ?	evtchn_expand_array		event_channel.h
+?	evtchn_fifo_control_block	event_channel.h
 ?	evtchn_init_control		event_channel.h
 ?	evtchn_op			event_channel.h
 ?	evtchn_reset			event_channel.h

Hi,

On 21/04/2021 15:36, Jan Beulich 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.

I had another fresh look today at this patch. The 32-bit padding is 
right after the field 'ready'.

I can't for sure tell how the second half is going to ever be used and how.

However, one possibility would be to extend the field 'ready' to 64-bit. 
With the current code, we could easily make a single 64-bit access 
without having to know whether the guest is able to interpret the top half.

With your approach, we may need to have different path depending on the 
padding and ensure the new extension cannot be enabled if the padding is 
4-byte. Otherwise, the atomicity would be broken.

> While relaxation of the requirements is intended here, the primary goal
> is to simply get rid of the hard coded number as well its lack of
> connection to the structure that is is meant to apply to.

Based on what I wrote above, I think the relaxation should not be done 
to give us more flexibility about possible extension to the structure.

Although, I would be worth documenting the reasoning in the code.

Cheers,

-- 
Julien Grall

On 21.04.2021 21:52, Julien Grall wrote:
> Hi,
> 
> On 21/04/2021 15:36, Jan Beulich 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.
> 
> I had another fresh look today at this patch. The 32-bit padding is 
> right after the field 'ready'.
> 
> I can't for sure tell how the second half is going to ever be used and how.
> 
> However, one possibility would be to extend the field 'ready' to 64-bit. 
> With the current code, we could easily make a single 64-bit access 
> without having to know whether the guest is able to interpret the top half.

I don't think extending field sizes is generally to be considered ABI-
compatible. I also don't think we can re-use the field at all, as I
couldn't find any checking of it being zero (input) or it getting set
to zero (output). struct evtchn_init_control, which in principle could
be a way to convey respective controlling flags, similarly has no room
for extension, as its _pad[] also doesn't look to get checked anywhere.

Jan

> With your approach, we may need to have different path depending on the 
> padding and ensure the new extension cannot be enabled if the padding is 
> 4-byte. Otherwise, the atomicity would be broken.
> 
>> While relaxation of the requirements is intended here, the primary goal
>> is to simply get rid of the hard coded number as well its lack of
>> connection to the structure that is is meant to apply to.
> 
> Based on what I wrote above, I think the relaxation should not be done 
> to give us more flexibility about possible extension to the structure.
> 
> Although, I would be worth documenting the reasoning in the code.
> 
> Cheers,
> 

Hi Jan,

On 22/04/2021 10:19, Jan Beulich wrote:
> On 21.04.2021 21:52, Julien Grall wrote:
>> Hi,
>>
>> On 21/04/2021 15:36, Jan Beulich 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.
>>
>> I had another fresh look today at this patch. The 32-bit padding is
>> right after the field 'ready'.
>>
>> I can't for sure tell how the second half is going to ever be used and how.
>>
>> However, one possibility would be to extend the field 'ready' to 64-bit.
>> With the current code, we could easily make a single 64-bit access
>> without having to know whether the guest is able to interpret the top half.
> 
> I don't think extending field sizes is generally to be considered ABI-
> compatible. I also don't think we can re-use the field at all, as I
> couldn't find any checking of it being zero (input) or it getting set
> to zero (output). 

That's would be fine so long we have a flag to control it. We can still 
write unconditionally because a guest can't rely on the pad...

> struct evtchn_init_control, which in principle could
> be a way to convey respective controlling flags, similarly has no room
> for extension, as its _pad[] also doesn't look to get checked anywhere.
Right, we would need to have a different way to convey. Yet, I am still 
unconvinced of the benefits change offer in this patch.

I am not going to Nack. So another maintainer in "THE REST" can express 
support for your patch and ack it.

Cheers,

-- 
Julien Grall