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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 31 January 2017 19:59
> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
> <JBeulich@xxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>
> Subject: [PATCH] x86/dmop: Fix compat_dm_op() ABI
> 
> 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>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> 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).

Indeed, which is why I used it... particularly as, when inspecting the 
auto-generated xen/include/compat/hvm/dm_op.h, the definition of struct 
compat_dm_op_buf is:

struct compat_dm_op_buf {
        COMPAT_HANDLE(void) h;
        compat_ulong_t size;
};
typedef struct compat_dm_op_buf compat_dm_op_buf_t;
DEFINE_COMPAT_HANDLE(compat_dm_op_buf_t);

Thus passing the bufs array as a COMPAT_HANDLE_PARAM must surely be the correct 
thing to, right?

> 
> 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?
> ---
>  xen/arch/x86/hvm/dm.c       | 4 ++--
>  xen/include/xen/hypercall.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 6a722a5..2122c45 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -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)
>  {
>      struct xen_dm_op_buf nat[MAX_NR_BUFS];
>      unsigned int i;
> @@ -538,7 +538,7 @@ int compat_dm_op(domid_t domid,
>      {
>          struct compat_dm_op_buf cmp;
> 
> -        if ( copy_from_compat_offset(&cmp, bufs, i, 1) )
> +        if ( copy_from_guest_offset(&cmp, bufs, i, 1) )
>              return -EFAULT;
> 
>  #define XLAT_dm_op_buf_HNDL_h(_d_, _s_) \
> diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
> index 8d4824f..cc99aea 100644
> --- a/xen/include/xen/hypercall.h
> +++ b/xen/include/xen/hypercall.h
> @@ -203,7 +203,7 @@ extern 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);

Shouldn't we really be fixing COMPAT_HANDLE_PARAM() to DTRT?

  Paul

> 
>  #endif
> 
> --
> 2.1.4


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