[PATCH] x86/dmop: Fix compat_dm_op() ABI

[PATCH] x86/dmop: Fix compat_dm_op() ABI

[Andrew Cooper]

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

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

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);
 
 #endif
 
-- 
2.1.4


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

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

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 31 January 2017 19:59
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>
> 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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 31.01.17 at 20:59, <andrew.cooper3@citrix.com> 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@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
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.

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

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

Jan


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

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

[Jan Beulich]

>>> On 31.01.17 at 20:59, <andrew.cooper3@citrix.com> wrote:
> 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).

Btw, where are you taking this "one and a half pointers" from?
It's half a pointer (a compat one) plus a zero sized array.

Jan


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

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

[Andrew Cooper]

On 01/02/17 10:46, Jan Beulich wrote:
>>>> On 31.01.17 at 20:59, <andrew.cooper3@citrix.com> wrote:
>> 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).
> Btw, where are you taking this "one and a half pointers" from?
> It's half a pointer (a compat one) plus a zero sized array.

Hmm.  I had missed the ZLA, but debugging proves that the raw value of
bufs.c was garbage even when passing a NULL handle from userspace.  As a
result, the copy_from_compat_offset() was hitting -EFAULT for every
hypercall.

~Andrew

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

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

[Stefano Stabellini]

On Wed, 1 Feb 2017, Jan Beulich wrote:
> >>> On 31.01.17 at 20:59, <andrew.cooper3@citrix.com> 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@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 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@lists.xen.org
https://lists.xen.org/xen-devel