Re: [Xen-devel] [PATCH v7 09/15] argo: implement the sendv op; evtchn: expose send_guest_global_virq

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.01.19 at 05:28, <christopher.w.clark@xxxxxxxxx> wrote:
> @@ -1237,6 +1864,54 @@ compat_argo_op(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg1,
>          break;
>      }
>  
> +    case XEN_ARGO_OP_sendv:
> +    {
> +        xen_argo_send_addr_t send_addr;
> +        xen_argo_iov_t iovs[XEN_ARGO_MAXIOV];
> +        unsigned int i;
> +        unsigned int niov;
> +
> +        XEN_GUEST_HANDLE_PARAM(xen_argo_send_addr_t) send_addr_hnd =
> +            guest_handle_cast(arg1, xen_argo_send_addr_t);
> +        /* arg2: iovs, arg3: niov, arg4: message_type */
> +
> +        rc = copy_from_guest(&send_addr, send_addr_hnd, 1) ? -EFAULT : 0;
> +        if ( rc )
> +            break;
> +
> +        if ( unlikely(arg3 > XEN_ARGO_MAXIOV) )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        niov = array_index_nospec(arg3, XEN_ARGO_MAXIOV + 1);
> +
> +        /*
> +         * Limited scope for compat_iovs array: enables a single 
> copy_from_guest
> +         * call and discards the array from the stack before calling sendv.
> +         */

What makes you think the array gets removed from the stack again
before the call? The typical way of setting up stack frames for a
function is to allocate the full chunk of space needed at the start
of the function, and remove it before returning. Without the
argo_dprintk() after the switch() there would be the potential of
the sendv() carried out as a tail call, but you can't rely on that.

With the current XEN_ARGO_MAXIOV value of 8 the overall frame
size is still tolerable, I would say. But I think you want to add
BUILD_BUG_ON()s here and in the native handler, such that
careless bumping of the value won't go unnoticed (but also see
below).

> --- a/xen/include/public/argo.h
> +++ b/xen/include/public/argo.h
> @@ -43,6 +43,28 @@
>  /* Fixed-width type for "argo port" number. Nothing to do with evtchns. */
>  typedef uint32_t xen_argo_port_t;
>  
> +/*
> + * XEN_ARGO_MAXIOV : maximum number of iovs accepted in a single sendv.
> + * Caution is required if this value is increased: this determines the size 
> of
> + * an array of xen_argo_iov_t structs on the hypervisor stack, so could cause
> + * stack overflow if the value is too large.
> + * The Linux Argo driver never passes more than two iovs.
> + *
> + * This value should also not exceed 128 to ensure that the total amount of 
> data
> + * posted in a single Argo sendv operation cannot exceed 2^31 bytes, to 
> reduce
> + * risk of integer overflow defects:
> + * Each argo iov can hold ~ 2^24 bytes, so XEN_ARGO_MAXIOV <= 2^(31-24),
> + * ie. keep XEN_ARGO_MAXIOV <= 128.
> +*/
> +#define XEN_ARGO_MAXIOV          8U

How does 2^31 come into play here? uint32_t can hold up to 2^32, and
you shouldn't be using signed arithmetic anywhere by this time anymore.
I'm also struggling to see what the "~ 2^24 bytes" refers to - I see nothing
along these lines added to the public header, and ...

> +typedef struct xen_argo_iov
> +{
> +    XEN_GUEST_HANDLE(uint8) iov_hnd;
> +    uint32_t iov_len;

... the field here allows for 2^32-1. Oh, it's XEN_ARGO_MAX_RING_SIZE.
It would help if the comment cross referenced that name.

Btw., neither of these two maximum values look to be architectural limits,
so I wonder whether, before declaring the ABI stable, these constants
shouldn't be purged and replaced by settings the guest is to retrieve via
hypercall.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel