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