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