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
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;
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.
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
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.
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
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 >
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
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
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
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
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 > >
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
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 >
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
© 2016 - 2024 Red Hat, Inc.