Re: [Xen-devel] [PATCH] x86/dmop: Fix compat_dm_op() ABI

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 1 Feb 2017, Jan Beulich wrote:
> >>> On 31.01.17 at 20:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> > The parameter to compat_dm_op() is a pointer to an array of
> > compat_dm_op_buf_t's in guest RAM.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with one question (see below).
> 
> > What is the pupose of COMPAT_HANDLE_PARAM()? It is a packed structure of one
> > and a half pointers, so isn't safe at all for use in the hypercall function
> > APIs (depsite its naming making it look deceptively like it is the correct
> > thing to use).
> 
> Stefano, you've added this in e7a527e100 ("xen: replace
> XEN_GUEST_HANDLE with XEN_GUEST_HANDLE_PARAM when
> appropriate"), without any user. I think it should simply be
> removed.

I think I added it for completeness: hypercalls parameters which are
pointers are supposed to be declared using _PARAM macros, because on arm
they have a different size compared to their corresponding
XEN_GUEST_HANDLE type (4 bytes vs 8 bytes). Unfortunately, forgetting to
convert an hypercall parameter type to _PARAM doesn't result in a build
failure, but in a much less obvious runtime failure when the hypercall
is called.

Compat hypercalls are not used on ARM, but for consistency I thought it
would be a good idea to mark their hypercall parameters as "_PARAM".

To do that, I introduced the COMPAT_HANDLE_PARAM macro, whose only
purpose is to be have "_PARAM" in its name. It is not necessary in
compat_dm_op from an ABI point of view, but it properly marks the third
parameter as an hypercall parameter.

I am OK with removing COMPAT_HANDLE_PARAM, but please carefully use the
appropriate XEN_GUEST_HANDLE_PARAM type (avoid directly using
XEN_GUEST_HANDLE) in its stead.


> > On a more serious note, why do we have all this macro infrastrucutre in the
> > first place?  Having spent rather longer debugging this than I to admit
> > (almost mainly from the userspace side) I have concluded that it is actively
> > dangerous to use; all it does is hide what is going on.
> > 
> > What does it actually give us that the Linux route of a real C pointers and 
> > a
> > __user attribute doesn't, other than obfuscating the code on the hypercall
> > boundary?
> 
> I think you refer to the whole handle machinery here, the main goal
> of which is to avoid anyone mistakenly directly de-referencing a
> pointer coming from a guest. Linux'es __user attribute doesn't
> achieve this during the normal compilation stage, afaik.

Additionally, XEN_GUEST_HANDLE_PARAM and XEN_GUEST_HANDLE differ on ARM.


> > @@ -525,7 +525,7 @@ CHECK_dm_op_inject_msi;
> >  
> >  int compat_dm_op(domid_t domid,
> >                   unsigned int nr_bufs,
> > -                 COMPAT_HANDLE_PARAM(compat_dm_op_buf_t) bufs)
> > +                 XEN_GUEST_HANDLE_PARAM(void) bufs)
> 
> Why the change to void? I'd prefer if we kept correct types,
> even if that's just for documentation purposes.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel