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

[PATCH 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

Jan

[PATCH 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>

--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1229,17 +1229,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 1/2] common: map_vcpu_info() cosmetics

[Roger Pau Monné]

On Wed, Jul 15, 2020 at 12:15:10PM +0200, 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.

Not sure whether I would use EFAULT instead of ENXIO, as the
description of it is 'bad address' which seems more inline with the
error that we are trying to report.

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

Thanks.

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

[Jan Beulich]

On 16.07.2020 13:41, Roger Pau Monné wrote:
> On Wed, Jul 15, 2020 at 12:15:10PM +0200, 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.
> 
> Not sure whether I would use EFAULT instead of ENXIO, as the
> description of it is 'bad address' which seems more inline with the
> error that we are trying to report.

The address isn't bad in the sense of causing a fault, it's just
that we elect to not allow it. Hence I don't think EFAULT is
suitable. I'm open to replacement suggestions for ENXIO, though.

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

Thanks, Jan

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

[Roger Pau Monné]

On Thu, Jul 16, 2020 at 01:48:51PM +0200, Jan Beulich wrote:
> On 16.07.2020 13:41, Roger Pau Monné wrote:
> > On Wed, Jul 15, 2020 at 12:15:10PM +0200, 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.
> > 
> > Not sure whether I would use EFAULT instead of ENXIO, as the
> > description of it is 'bad address' which seems more inline with the
> > error that we are trying to report.
> 
> The address isn't bad in the sense of causing a fault, it's just
> that we elect to not allow it. Hence I don't think EFAULT is
> suitable. I'm open to replacement suggestions for ENXIO, though.

Well, using an address that's not properly aligned to the requirements
of an interface would cause a fault? (in this case it's a software
interface, but the concept applies equally).

Anyway, not something worth arguing about I think, so unless someone
else disagrees I'm fine with using ENXIO.

Thanks.

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

[Jan Beulich]

On 16.07.2020 16:42, Roger Pau Monné wrote:
> On Thu, Jul 16, 2020 at 01:48:51PM +0200, Jan Beulich wrote:
>> On 16.07.2020 13:41, Roger Pau Monné wrote:
>>> On Wed, Jul 15, 2020 at 12:15:10PM +0200, 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.
>>>
>>> Not sure whether I would use EFAULT instead of ENXIO, as the
>>> description of it is 'bad address' which seems more inline with the
>>> error that we are trying to report.
>>
>> The address isn't bad in the sense of causing a fault, it's just
>> that we elect to not allow it. Hence I don't think EFAULT is
>> suitable. I'm open to replacement suggestions for ENXIO, though.
> 
> Well, using an address that's not properly aligned to the requirements
> of an interface would cause a fault? (in this case it's a software
> interface, but the concept applies equally).

Not necessarily, see x86'es behavior. Also even on strict arches
it is typically possible to cover for the misalignment by using
suitable instructions; it's still an implementation choice to not
do so.

> Anyway, not something worth arguing about I think, so unless someone
> else disagrees I'm fine with using ENXIO.

Good, thanks.

Jan

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

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

On Thu, 16 Jul 2020, 17:01 Jan Beulich, <jbeulich@suse.com> wrote:

> On 16.07.2020 16:42, Roger Pau Monné wrote:
> > On Thu, Jul 16, 2020 at 01:48:51PM +0200, Jan Beulich wrote:
> >> On 16.07.2020 13:41, Roger Pau Monné wrote:
> >>> On Wed, Jul 15, 2020 at 12:15:10PM +0200, 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.
> >>>
> >>> Not sure whether I would use EFAULT instead of ENXIO, as the
> >>> description of it is 'bad address' which seems more inline with the
> >>> error that we are trying to report.
> >>
> >> The address isn't bad in the sense of causing a fault, it's just
> >> that we elect to not allow it. Hence I don't think EFAULT is
> >> suitable. I'm open to replacement suggestions for ENXIO, though.
> >
> > Well, using an address that's not properly aligned to the requirements
> > of an interface would cause a fault? (in this case it's a software
> > interface, but the concept applies equally).
>
> Not necessarily, see x86'es behavior. Also even on strict arches

it is typically possible to cover for the misalignment by using
> suitable instructions; it's still an implementation choice to not
> do so.


I am not sure about your argument here... Yes it might be possible, but at
what cost?


> > Anyway, not something worth arguing about I think, so unless someone
> > else disagrees I'm fine with using ENXIO.
>
> Good, thanks.
>

-EFAULT can be described as "Bad address". I think it makes more sense than
-ENXIO here even if it may not strictly result to a fault on some arch.


> Jan
>

[-- Attachment #2: Type: text/html, Size: 2930 bytes --]

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

[Jan Beulich]

On 16.07.2020 18:17, Julien Grall wrote:
> On Thu, 16 Jul 2020, 17:01 Jan Beulich, <jbeulich@suse.com> wrote:
> 
>> On 16.07.2020 16:42, Roger Pau Monné wrote:
>>> On Thu, Jul 16, 2020 at 01:48:51PM +0200, Jan Beulich wrote:
>>>> On 16.07.2020 13:41, Roger Pau Monné wrote:
>>>>> On Wed, Jul 15, 2020 at 12:15:10PM +0200, 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.
>>>>>
>>>>> Not sure whether I would use EFAULT instead of ENXIO, as the
>>>>> description of it is 'bad address' which seems more inline with the
>>>>> error that we are trying to report.
>>>>
>>>> The address isn't bad in the sense of causing a fault, it's just
>>>> that we elect to not allow it. Hence I don't think EFAULT is
>>>> suitable. I'm open to replacement suggestions for ENXIO, though.
>>>
>>> Well, using an address that's not properly aligned to the requirements
>>> of an interface would cause a fault? (in this case it's a software
>>> interface, but the concept applies equally).
>>
>> Not necessarily, see x86'es behavior. Also even on strict arches
> 
> it is typically possible to cover for the misalignment by using
>> suitable instructions; it's still an implementation choice to not
>> do so.
> 
> 
> I am not sure about your argument here... Yes it might be possible, but at
> what cost?

The cost is what influences the decision whether to support it. Nevertheless
it remains an implementation decision rather than a hardware imposed
restriction, and hence I don't consider -EFAULT suitable here.

>>> Anyway, not something worth arguing about I think, so unless someone
>>> else disagrees I'm fine with using ENXIO.
>>
>> Good, thanks.
>>
> 
> -EFAULT can be described as "Bad address". I think it makes more sense than
> -ENXIO here even if it may not strictly result to a fault on some arch.

As said - I don't consider EFAULT applicable here; I also consider EINVAL
as too generic. I'll be happy to see replacement suggestions for my ENXIO
choice, but obviously I'm not overly happy to see options re-suggested
which I did already say I've ruled out.

Jan

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

[Julien Grall]

On 17/07/2020 09:16, Jan Beulich wrote:
> On 16.07.2020 18:17, Julien Grall wrote:
>> On Thu, 16 Jul 2020, 17:01 Jan Beulich, <jbeulich@suse.com> wrote:
>>
>>> On 16.07.2020 16:42, Roger Pau Monné wrote:
>>>> On Thu, Jul 16, 2020 at 01:48:51PM +0200, Jan Beulich wrote:
>>>>> On 16.07.2020 13:41, Roger Pau Monné wrote:
>>>>>> On Wed, Jul 15, 2020 at 12:15:10PM +0200, 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.
>>>>>>
>>>>>> Not sure whether I would use EFAULT instead of ENXIO, as the
>>>>>> description of it is 'bad address' which seems more inline with the
>>>>>> error that we are trying to report.
>>>>>
>>>>> The address isn't bad in the sense of causing a fault, it's just
>>>>> that we elect to not allow it. Hence I don't think EFAULT is
>>>>> suitable. I'm open to replacement suggestions for ENXIO, though.
>>>>
>>>> Well, using an address that's not properly aligned to the requirements
>>>> of an interface would cause a fault? (in this case it's a software
>>>> interface, but the concept applies equally).
>>>
>>> Not necessarily, see x86'es behavior. Also even on strict arches
>>
>> it is typically possible to cover for the misalignment by using
>>> suitable instructions; it's still an implementation choice to not
>>> do so.
>>
>>
>> I am not sure about your argument here... Yes it might be possible, but at
>> what cost?
> 
> The cost is what influences the decision whether to support it. Nevertheless
> it remains an implementation decision rather than a hardware imposed
> restriction, and hence I don't consider -EFAULT suitable here.
> 
>>>> Anyway, not something worth arguing about I think, so unless someone
>>>> else disagrees I'm fine with using ENXIO.
>>>
>>> Good, thanks.
>>>
>>
>> -EFAULT can be described as "Bad address". I think it makes more sense than
>> -ENXIO here even if it may not strictly result to a fault on some arch.
> 
> As said - I don't consider EFAULT applicable here;

AFAICT, you don't consider it because you think that using the address 
means it will always lead to a fault. However, this is just a strict 
interpretation of the error code. A less strict interpretation is it 
could be used for any address that is considered to be invalid.

-ENXIO makes less sense because the address exists. To re-use your 
argument, this is just an implementation details.

> I also consider EINVAL
> as too generic. I'll be happy to see replacement suggestions for my ENXIO
> choice, but obviously I'm not overly happy to see options re-suggested
> which I did already say I've ruled out.

I think I am allowed to express my opinion even if it means this was 
already said... However, I should have been clearer and say that I agree 
with Roger's suggestion about -EFAULT.

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

On 17.07.2020 11:22, Julien Grall wrote:
> 
> 
> On 17/07/2020 09:16, Jan Beulich wrote:
>> On 16.07.2020 18:17, Julien Grall wrote:
>>> On Thu, 16 Jul 2020, 17:01 Jan Beulich, <jbeulich@suse.com> wrote:
>>>
>>>> On 16.07.2020 16:42, Roger Pau Monné wrote:
>>>>> On Thu, Jul 16, 2020 at 01:48:51PM +0200, Jan Beulich wrote:
>>>>>> On 16.07.2020 13:41, Roger Pau Monné wrote:
>>>>>>> On Wed, Jul 15, 2020 at 12:15:10PM +0200, 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.
>>>>>>>
>>>>>>> Not sure whether I would use EFAULT instead of ENXIO, as the
>>>>>>> description of it is 'bad address' which seems more inline with the
>>>>>>> error that we are trying to report.
>>>>>>
>>>>>> The address isn't bad in the sense of causing a fault, it's just
>>>>>> that we elect to not allow it. Hence I don't think EFAULT is
>>>>>> suitable. I'm open to replacement suggestions for ENXIO, though.
>>>>>
>>>>> Well, using an address that's not properly aligned to the requirements
>>>>> of an interface would cause a fault? (in this case it's a software
>>>>> interface, but the concept applies equally).
>>>>
>>>> Not necessarily, see x86'es behavior. Also even on strict arches
>>>
>>> it is typically possible to cover for the misalignment by using
>>>> suitable instructions; it's still an implementation choice to not
>>>> do so.
>>>
>>>
>>> I am not sure about your argument here... Yes it might be possible, but at
>>> what cost?
>>
>> The cost is what influences the decision whether to support it. Nevertheless
>> it remains an implementation decision rather than a hardware imposed
>> restriction, and hence I don't consider -EFAULT suitable here.
>>
>>>>> Anyway, not something worth arguing about I think, so unless someone
>>>>> else disagrees I'm fine with using ENXIO.
>>>>
>>>> Good, thanks.
>>>>
>>>
>>> -EFAULT can be described as "Bad address". I think it makes more sense than
>>> -ENXIO here even if it may not strictly result to a fault on some arch.
>>
>> As said - I don't consider EFAULT applicable here;
> 
> AFAICT, you don't consider it because you think that using the address 
> means it will always lead to a fault. However, this is just a strict 
> interpretation of the error code. A less strict interpretation is it 
> could be used for any address that is considered to be invalid.
> 
> -ENXIO makes less sense because the address exists. To re-use your 
> argument, this is just an implementation details.

To be honest, with all the errno values (and with how we use them in
Xen) I rarely pay attention to the text that's associated with them,
but rather what their symbolic name says. For ENXIO, I don't consider
"No such device or address" any more sensible than "Bad address" for
EFAULT.

Since I'm against EFAULT (and EINVAL) and you're against ENXIO, how
about you suggest a 3rd alternative? EPERM comes to mind, but could
easily be mistaken for yet something else. My goal really is to use
an error code here which makes it immediately clear what the actual
problem was, with no ambiguity to other possible error sources on
this hypercall handling path. I don't really care about which of the
ones we use here that aren't already used anywhere on this path. I'd
even be fine with presumably (at least to some people) absurd ones
like EILSEQ or EXDEV, which we use elsewhere, and hardly in the sense
they were originally meant, but again for the purpose of making the
cause of the error easily identifiable.

For this purpose, to me ENXIO seemed to be a reasonable fit. So again
- I'm open to suggestions, but it has to be a not commonly used error
code.

Jan

[PATCH 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.

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

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

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 3215 bytes --]

On Wed, 15 Jul 2020, 12:17 Jan Beulich, <jbeulich@suse.com> 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@suse.com>
> ---
> 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
>
>

[-- Attachment #2: Type: text/html, Size: 4389 bytes --]

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

[Jan Beulich]

On 15.07.2020 12:46, Julien Grall wrote:
> On Wed, 15 Jul 2020, 12:17 Jan Beulich, <jbeulich@suse.com> 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@suse.com>
>> ---
>> 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?

That's the question. One of your arguments for using a literal number
also for the vCPU info mapping check was that here a literal number
is used. The goal isn't so much relaxation of the interface, but
making the code consistent as well as eliminating a (how I'd call it)
kludge.

Guests not caring to be able to run on older versions could also make
use of the relaxation (which may be more relevant in 10 years time
than it is now).

Jan

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

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 3804 bytes --]

On Wed, 15 Jul 2020, 14:42 Jan Beulich, <jbeulich@suse.com> wrote:

> On 15.07.2020 12:46, Julien Grall wrote:
> > On Wed, 15 Jul 2020, 12:17 Jan Beulich, <jbeulich@suse.com> 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@suse.com>
> >> ---
> >> 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?
>
> That's the question. One of your arguments for using a literal number
> also for the vCPU info mapping check was that here a literal number
> is used. The goal isn't so much relaxation of the interface, but
> making the code consistent as well as eliminating a (how I'd call it)
> kludge.
>

Your commit message lead to think the relaxation is the key motivation to
change the code.



> Guests not caring to be able to run on older versions could also make
> use of the relaxation (which may be more relevant in 10 y ears time
> than it is now).


That makes sense. However, I am a bit concerned that an OS developer may
not notice the alignment problem with older version.

I would suggest to at least document the alignment expected in the public
header.



> Jan
>

[-- Attachment #2: Type: text/html, Size: 5791 bytes --]

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

[Jan Beulich]

On 15.07.2020 16:02, Julien Grall wrote:
> On Wed, 15 Jul 2020, 14:42 Jan Beulich, <jbeulich@suse.com> wrote:
> 
>> On 15.07.2020 12:46, Julien Grall wrote:
>>> On Wed, 15 Jul 2020, 12:17 Jan Beulich, <jbeulich@suse.com> 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@suse.com>
>>>> ---
>>>> 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?
>>
>> That's the question. One of your arguments for using a literal number
>> also for the vCPU info mapping check was that here a literal number
>> is used. The goal isn't so much relaxation of the interface, but
>> making the code consistent as well as eliminating a (how I'd call it)
>> kludge.
>>
> 
> Your commit message lead to think the relaxation is the key motivation to
> change the code.

I've added a clarifying sentence.

>> Guests not caring to be able to run on older versions could also make
>> use of the relaxation (which may be more relevant in 10 y ears time
>> than it is now).
> 
> 
> That makes sense. However, I am a bit concerned that an OS developer may
> not notice the alignment problem with older version.
> 
> I would suggest to at least document the alignment expected in the public
> header.

Done for v2.

Jan