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