>>> On 31.01.19 at 05:28, <christopher.w.cl...@gmail.com> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel