[PATCH v2 0/2] common: XSA-327 follow-up

[PATCH v2 0/2] common: XSA-327 follow-up

[Jan Beulich]

There are a few largely cosmetic things that were discussed in the
context of the XSA, but which weren't really XSA material.

1: common: map_vcpu_info() cosmetics
2: evtchn/fifo: don't enforce higher than necessary alignment

I realize both changes are controversial. For the first one
discussion was about the choice of error code. Neither EINVAL nor
EFAULT represent the fact that it is a choice of implementation
to not support mis-aligned structures. If ENXIO isn't liked, the
best I can suggest are EOPNOTSUPP or (as previously suggested)
EPERM. I think it ought to be possible to settle on an error
code everyone can live with.

For the second one the question was whether the relaxation is
useful to do. The original reason for wanting to make the change
remains: The original code here should not be used as an excuse
to introduce similar over-alignment requirements elsewhere. I
can live with the change getting rejected, but if so I'd like to
request that some alternative be submitted to ensure that the
immediate goal can still be reached.

Jan

[PATCH v2 1/2] common: map_vcpu_info() cosmetics

[Jan Beulich]

Use ENXIO instead of EINVAL to cover the two cases of the address not
satisfying the requirements. This will make an issue here better stand
out at the call site.

Also add a missing compat-mode related size check: If the sizes
differed, other code in the function would need changing. Accompany this
by a change to the initial sizeof() expression, tying it to the type of
the variable we're actually after (matching e.g. the alignof() added by
XSA-327).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1241,17 +1241,18 @@ int map_vcpu_info(struct vcpu *v, unsign
     struct page_info *page;
     unsigned int align;
 
-    if ( offset > (PAGE_SIZE - sizeof(vcpu_info_t)) )
-        return -EINVAL;
+    if ( offset > (PAGE_SIZE - sizeof(*new_info)) )
+        return -ENXIO;
 
 #ifdef CONFIG_COMPAT
+    BUILD_BUG_ON(sizeof(*new_info) != sizeof(new_info->compat));
     if ( has_32bit_shinfo(d) )
         align = alignof(new_info->compat);
     else
 #endif
         align = alignof(*new_info);
     if ( offset & (align - 1) )
-        return -EINVAL;
+        return -ENXIO;
 
     if ( !mfn_eq(v->vcpu_info_mfn, INVALID_MFN) )
         return -EINVAL;

Re: [PATCH v2 1/2] common: map_vcpu_info() cosmetics

[Julien Grall]

Hi Jan,

On 22/12/2020 08:14, Jan Beulich wrote:
> Use ENXIO instead of EINVAL to cover the two cases of the address not
> satisfying the requirements. This will make an issue here better stand
> out at the call site.
> 
> Also add a missing compat-mode related size check: If the sizes
> differed, other code in the function would need changing. Accompany this
> by a change to the initial sizeof() expression, tying it to the type of
> the variable we're actually after (matching e.g. the alignof() added by
> XSA-327).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

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>
---
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.14 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

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

[Jan Beulich]

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

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

[Julien Grall]

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

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

[Jan Beulich]

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,
> 

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

[Julien Grall]

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