[Qemu-devel] [PATCH] xen: additionally restrict xenforeignmemory operations

[Qemu-devel] [PATCH] xen: additionally restrict xenforeignmemory operations

[Paul Durrant]

Commit f0f272baf3a7 "xen: use libxendevice model to restrict operations"
added a command-line option (-xen-domid-restrict) to limit operations
using the libxendevicemodel API to a specified domid. The commit also
noted that the restriction would be extended to cover operations issued
via other xen libraries by subsequent patches.

My recent Xen patch [1] added a call to the xenforeignmemory API to allow
it to be restricted. This patch now makes use of that new call when the
-xen-domid-restrict option is passed.

[1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=5823d6eb

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
 include/hw/xen/xen_common.h | 52 +++++++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 4f3bd35..6f13e8c 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -213,19 +213,6 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn,
     return xendevicemodel_modified_memory(xen_dmod, domid, first_pfn, nr);
 }
 
-static inline int xen_restrict(domid_t domid)
-{
-    int rc = xendevicemodel_restrict(xen_dmod, domid);
-
-    trace_xen_domid_restrict(errno);
-
-    if (errno == ENOTTY) {
-        return 0;
-    }
-
-    return rc;
-}
-
 /* Xen 4.2 through 4.6 */
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
 
@@ -276,8 +263,47 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
 
 #endif
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
+
+static inline int xenforeignmemory_restrict(
+    xenforeignmemory_handle *fmem, domid_t domid)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
+#endif
+
 extern xenforeignmemory_handle *xen_fmem;
 
+static inline int xen_restrict(domid_t domid)
+{
+    int rc;
+
+    /* Attempt to restrict devicemodel operations */
+    rc = xendevicemodel_restrict(xen_dmod, domid);
+    trace_xen_domid_restrict(rc ? errno : 0);
+
+    if (rc < 0) {
+        /*
+         * If errno is ENOTTY then restriction is not implemented so
+         * there's no point in trying to restrict other types of
+         * operation, but it should not be treated as a failure.
+         */
+        if (errno == ENOTTY) {
+            return 0;
+        }
+
+        return rc;
+    }
+
+    /* Restrict foreignmemory operations */
+    rc = xenforeignmemory_restrict(xen_fmem, domid);
+    trace_xen_domid_restrict(rc ? errno : 0);
+
+    return rc;
+}
+
 void destroy_hvm_domain(bool reboot);
 
 /* shutdown/destroy current domain because of an error */
-- 
2.1.4

Re: [Qemu-devel] [PATCH] xen: additionally restrict xenforeignmemory operations

[Stefano Stabellini]

On Fri, 24 Mar 2017, Paul Durrant wrote:
> Commit f0f272baf3a7 "xen: use libxendevice model to restrict operations"
> added a command-line option (-xen-domid-restrict) to limit operations
> using the libxendevicemodel API to a specified domid. The commit also
> noted that the restriction would be extended to cover operations issued
> via other xen libraries by subsequent patches.
> 
> My recent Xen patch [1] added a call to the xenforeignmemory API to allow
> it to be restricted. This patch now makes use of that new call when the
> -xen-domid-restrict option is passed.
> 
> [1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=5823d6eb
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> ---
>  include/hw/xen/xen_common.h | 52 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 4f3bd35..6f13e8c 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -213,19 +213,6 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn,
>      return xendevicemodel_modified_memory(xen_dmod, domid, first_pfn, nr);
>  }
>  
> -static inline int xen_restrict(domid_t domid)
> -{
> -    int rc = xendevicemodel_restrict(xen_dmod, domid);
> -
> -    trace_xen_domid_restrict(errno);
> -
> -    if (errno == ENOTTY) {
> -        return 0;
> -    }
> -
> -    return rc;
> -}
> -
>  /* Xen 4.2 through 4.6 */
>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
>  
> @@ -276,8 +263,47 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
>  
>  #endif
>  
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
> +
> +static inline int xenforeignmemory_restrict(
> +    xenforeignmemory_handle *fmem, domid_t domid)
> +{
> +    errno = ENOTTY;
> +    return -1;
> +}
> +
> +#endif
> +
>  extern xenforeignmemory_handle *xen_fmem;
>  
> +static inline int xen_restrict(domid_t domid)
> +{
> +    int rc;
> +
> +    /* Attempt to restrict devicemodel operations */
> +    rc = xendevicemodel_restrict(xen_dmod, domid);
> +    trace_xen_domid_restrict(rc ? errno : 0);
> +
> +    if (rc < 0) {
> +        /*
> +         * If errno is ENOTTY then restriction is not implemented so
> +         * there's no point in trying to restrict other types of
> +         * operation, but it should not be treated as a failure.
> +         */
> +        if (errno == ENOTTY) {
> +            return 0;
> +        }
> +
> +        return rc;
> +    }
> +
> +    /* Restrict foreignmemory operations */
> +    rc = xenforeignmemory_restrict(xen_fmem, domid);
> +    trace_xen_domid_restrict(rc ? errno : 0);
> +
> +    return rc;
> +}
> +
>  void destroy_hvm_domain(bool reboot);
>  
>  /* shutdown/destroy current domain because of an error */

This is OK but the file is growing too entangled. What do you think of
the following, which moves the if CONFIG_XEN_CTRL_INTERFACE_VERSION <
40701 at the top? This way we don't have to add yet another ifdef.

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 4f3bd35..624fb86 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -27,6 +27,58 @@ extern xc_interface *xen_xc;
  * We don't support Xen prior to 4.2.0.
  */
 
+/* Xen 4.2 through 4.6 */
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
+
+typedef xc_interface xenforeignmemory_handle;
+typedef xc_evtchn xenevtchn_handle;
+typedef xc_gnttab xengnttab_handle;
+
+#define xenevtchn_open(l, f) xc_evtchn_open(l, f);
+#define xenevtchn_close(h) xc_evtchn_close(h)
+#define xenevtchn_fd(h) xc_evtchn_fd(h)
+#define xenevtchn_pending(h) xc_evtchn_pending(h)
+#define xenevtchn_notify(h, p) xc_evtchn_notify(h, p)
+#define xenevtchn_bind_interdomain(h, d, p) xc_evtchn_bind_interdomain(h, d, p)
+#define xenevtchn_unmask(h, p) xc_evtchn_unmask(h, p)
+#define xenevtchn_unbind(h, p) xc_evtchn_unbind(h, p)
+
+#define xengnttab_open(l, f) xc_gnttab_open(l, f)
+#define xengnttab_close(h) xc_gnttab_close(h)
+#define xengnttab_set_max_grants(h, n) xc_gnttab_set_max_grants(h, n)
+#define xengnttab_map_grant_ref(h, d, r, p) xc_gnttab_map_grant_ref(h, d, r, p)
+#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n)
+#define xengnttab_map_grant_refs(h, c, d, r, p) \
+    xc_gnttab_map_grant_refs(h, c, d, r, p)
+#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \
+    xc_gnttab_map_domain_grant_refs(h, c, d, r, p)
+
+#define xenforeignmemory_open(l, f) xen_xc
+#define xenforeignmemory_close(h)
+
+static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
+                                         int prot, size_t pages,
+                                         const xen_pfn_t arr[/*pages*/],
+                                         int err[/*pages*/])
+{
+    if (err)
+        return xc_map_foreign_bulk(h, dom, prot, arr, err, pages);
+    else
+        return xc_map_foreign_pages(h, dom, prot, arr, pages);
+}
+
+#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE)
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
+
+#include <xenevtchn.h>
+#include <xengnttab.h>
+#include <xenforeignmemory.h>
+
+#endif
+
+extern xenforeignmemory_handle *xen_fmem;
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
 
 typedef xc_interface xendevicemodel_handle;
@@ -159,6 +211,13 @@ static inline int xendevicemodel_restrict(
     return -1;
 }
 
+static inline int xenforeignmemory_restrict(
+    xenforeignmemory_handle *fmem, domid_t domid)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
 
 #include <xendevicemodel.h>
@@ -215,69 +274,32 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn,
 
 static inline int xen_restrict(domid_t domid)
 {
-    int rc = xendevicemodel_restrict(xen_dmod, domid);
+    int rc;
 
-    trace_xen_domid_restrict(errno);
+    /* Attempt to restrict devicemodel operations */
+    rc = xendevicemodel_restrict(xen_dmod, domid);
+    trace_xen_domid_restrict(rc ? errno : 0);
 
-    if (errno == ENOTTY) {
-        return 0;
+    if (rc < 0) {
+        /*
+         * If errno is ENOTTY then restriction is not implemented so
+         * there's no point in trying to restrict other types of
+         * operation, but it should not be treated as a failure.
+         */
+        if (errno == ENOTTY) {
+            return 0;
+        }
+
+        return rc;
     }
 
-    return rc;
-}
-
-/* Xen 4.2 through 4.6 */
-#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
-
-typedef xc_interface xenforeignmemory_handle;
-typedef xc_evtchn xenevtchn_handle;
-typedef xc_gnttab xengnttab_handle;
-
-#define xenevtchn_open(l, f) xc_evtchn_open(l, f);
-#define xenevtchn_close(h) xc_evtchn_close(h)
-#define xenevtchn_fd(h) xc_evtchn_fd(h)
-#define xenevtchn_pending(h) xc_evtchn_pending(h)
-#define xenevtchn_notify(h, p) xc_evtchn_notify(h, p)
-#define xenevtchn_bind_interdomain(h, d, p) xc_evtchn_bind_interdomain(h, d, p)
-#define xenevtchn_unmask(h, p) xc_evtchn_unmask(h, p)
-#define xenevtchn_unbind(h, p) xc_evtchn_unbind(h, p)
-
-#define xengnttab_open(l, f) xc_gnttab_open(l, f)
-#define xengnttab_close(h) xc_gnttab_close(h)
-#define xengnttab_set_max_grants(h, n) xc_gnttab_set_max_grants(h, n)
-#define xengnttab_map_grant_ref(h, d, r, p) xc_gnttab_map_grant_ref(h, d, r, p)
-#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n)
-#define xengnttab_map_grant_refs(h, c, d, r, p) \
-    xc_gnttab_map_grant_refs(h, c, d, r, p)
-#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \
-    xc_gnttab_map_domain_grant_refs(h, c, d, r, p)
-
-#define xenforeignmemory_open(l, f) xen_xc
-#define xenforeignmemory_close(h)
+    /* Restrict foreignmemory operations */
+    rc = xenforeignmemory_restrict(xen_fmem, domid);
+    trace_xen_domid_restrict(rc ? errno : 0);
 
-static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
-                                         int prot, size_t pages,
-                                         const xen_pfn_t arr[/*pages*/],
-                                         int err[/*pages*/])
-{
-    if (err)
-        return xc_map_foreign_bulk(h, dom, prot, arr, err, pages);
-    else
-        return xc_map_foreign_pages(h, dom, prot, arr, pages);
+    return rc;
 }
 
-#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE)
-
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
-
-#include <xenevtchn.h>
-#include <xengnttab.h>
-#include <xenforeignmemory.h>
-
-#endif
-
-extern xenforeignmemory_handle *xen_fmem;
-
 void destroy_hvm_domain(bool reboot);
 
 /* shutdown/destroy current domain because of an error */

Re: [Qemu-devel] [PATCH] xen: additionally restrict xenforeignmemory operations

[Paul Durrant]

> -----Original Message-----
[snip]
> 
> This is OK but the file is growing too entangled. What do you think of
> the following, which moves the if CONFIG_XEN_CTRL_INTERFACE_VERSION
> <
> 40701 at the top? This way we don't have to add yet another ifdef.
> 

Yes, this looks better and appears to DTRT. I assume you will just apply this version and don't want be to resubmit?

Cheers,

  Paul

> diff --git a/include/hw/xen/xen_common.h
> b/include/hw/xen/xen_common.h
> index 4f3bd35..624fb86 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -27,6 +27,58 @@ extern xc_interface *xen_xc;
>   * We don't support Xen prior to 4.2.0.
>   */
> 
> +/* Xen 4.2 through 4.6 */
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
> +
> +typedef xc_interface xenforeignmemory_handle;
> +typedef xc_evtchn xenevtchn_handle;
> +typedef xc_gnttab xengnttab_handle;
> +
> +#define xenevtchn_open(l, f) xc_evtchn_open(l, f);
> +#define xenevtchn_close(h) xc_evtchn_close(h)
> +#define xenevtchn_fd(h) xc_evtchn_fd(h)
> +#define xenevtchn_pending(h) xc_evtchn_pending(h)
> +#define xenevtchn_notify(h, p) xc_evtchn_notify(h, p)
> +#define xenevtchn_bind_interdomain(h, d, p)
> xc_evtchn_bind_interdomain(h, d, p)
> +#define xenevtchn_unmask(h, p) xc_evtchn_unmask(h, p)
> +#define xenevtchn_unbind(h, p) xc_evtchn_unbind(h, p)
> +
> +#define xengnttab_open(l, f) xc_gnttab_open(l, f)
> +#define xengnttab_close(h) xc_gnttab_close(h)
> +#define xengnttab_set_max_grants(h, n) xc_gnttab_set_max_grants(h, n)
> +#define xengnttab_map_grant_ref(h, d, r, p) xc_gnttab_map_grant_ref(h,
> d, r, p)
> +#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n)
> +#define xengnttab_map_grant_refs(h, c, d, r, p) \
> +    xc_gnttab_map_grant_refs(h, c, d, r, p)
> +#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \
> +    xc_gnttab_map_domain_grant_refs(h, c, d, r, p)
> +
> +#define xenforeignmemory_open(l, f) xen_xc
> +#define xenforeignmemory_close(h)
> +
> +static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
> +                                         int prot, size_t pages,
> +                                         const xen_pfn_t arr[/*pages*/],
> +                                         int err[/*pages*/])
> +{
> +    if (err)
> +        return xc_map_foreign_bulk(h, dom, prot, arr, err, pages);
> +    else
> +        return xc_map_foreign_pages(h, dom, prot, arr, pages);
> +}
> +
> +#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE)
> +
> +#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
> +
> +#include <xenevtchn.h>
> +#include <xengnttab.h>
> +#include <xenforeignmemory.h>
> +
> +#endif
> +
> +extern xenforeignmemory_handle *xen_fmem;
> +
>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
> 
>  typedef xc_interface xendevicemodel_handle;
> @@ -159,6 +211,13 @@ static inline int xendevicemodel_restrict(
>      return -1;
>  }
> 
> +static inline int xenforeignmemory_restrict(
> +    xenforeignmemory_handle *fmem, domid_t domid)
> +{
> +    errno = ENOTTY;
> +    return -1;
> +}
> +
>  #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
> 
>  #include <xendevicemodel.h>
> @@ -215,69 +274,32 @@ static inline int xen_modified_memory(domid_t
> domid, uint64_t first_pfn,
> 
>  static inline int xen_restrict(domid_t domid)
>  {
> -    int rc = xendevicemodel_restrict(xen_dmod, domid);
> +    int rc;
> 
> -    trace_xen_domid_restrict(errno);
> +    /* Attempt to restrict devicemodel operations */
> +    rc = xendevicemodel_restrict(xen_dmod, domid);
> +    trace_xen_domid_restrict(rc ? errno : 0);
> 
> -    if (errno == ENOTTY) {
> -        return 0;
> +    if (rc < 0) {
> +        /*
> +         * If errno is ENOTTY then restriction is not implemented so
> +         * there's no point in trying to restrict other types of
> +         * operation, but it should not be treated as a failure.
> +         */
> +        if (errno == ENOTTY) {
> +            return 0;
> +        }
> +
> +        return rc;
>      }
> 
> -    return rc;
> -}
> -
> -/* Xen 4.2 through 4.6 */
> -#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
> -
> -typedef xc_interface xenforeignmemory_handle;
> -typedef xc_evtchn xenevtchn_handle;
> -typedef xc_gnttab xengnttab_handle;
> -
> -#define xenevtchn_open(l, f) xc_evtchn_open(l, f);
> -#define xenevtchn_close(h) xc_evtchn_close(h)
> -#define xenevtchn_fd(h) xc_evtchn_fd(h)
> -#define xenevtchn_pending(h) xc_evtchn_pending(h)
> -#define xenevtchn_notify(h, p) xc_evtchn_notify(h, p)
> -#define xenevtchn_bind_interdomain(h, d, p)
> xc_evtchn_bind_interdomain(h, d, p)
> -#define xenevtchn_unmask(h, p) xc_evtchn_unmask(h, p)
> -#define xenevtchn_unbind(h, p) xc_evtchn_unbind(h, p)
> -
> -#define xengnttab_open(l, f) xc_gnttab_open(l, f)
> -#define xengnttab_close(h) xc_gnttab_close(h)
> -#define xengnttab_set_max_grants(h, n) xc_gnttab_set_max_grants(h, n)
> -#define xengnttab_map_grant_ref(h, d, r, p) xc_gnttab_map_grant_ref(h,
> d, r, p)
> -#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n)
> -#define xengnttab_map_grant_refs(h, c, d, r, p) \
> -    xc_gnttab_map_grant_refs(h, c, d, r, p)
> -#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \
> -    xc_gnttab_map_domain_grant_refs(h, c, d, r, p)
> -
> -#define xenforeignmemory_open(l, f) xen_xc
> -#define xenforeignmemory_close(h)
> +    /* Restrict foreignmemory operations */
> +    rc = xenforeignmemory_restrict(xen_fmem, domid);
> +    trace_xen_domid_restrict(rc ? errno : 0);
> 
> -static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
> -                                         int prot, size_t pages,
> -                                         const xen_pfn_t arr[/*pages*/],
> -                                         int err[/*pages*/])
> -{
> -    if (err)
> -        return xc_map_foreign_bulk(h, dom, prot, arr, err, pages);
> -    else
> -        return xc_map_foreign_pages(h, dom, prot, arr, pages);
> +    return rc;
>  }
> 
> -#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE)
> -
> -#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
> -
> -#include <xenevtchn.h>
> -#include <xengnttab.h>
> -#include <xenforeignmemory.h>
> -
> -#endif
> -
> -extern xenforeignmemory_handle *xen_fmem;
> -
>  void destroy_hvm_domain(bool reboot);
> 
>  /* shutdown/destroy current domain because of an error */

Re: [Qemu-devel] [PATCH] xen: additionally restrict xenforeignmemory operations

[Stefano Stabellini]

On Mon, 27 Mar 2017, Paul Durrant wrote:
> > -----Original Message-----
> [snip]
> > 
> > This is OK but the file is growing too entangled. What do you think of
> > the following, which moves the if CONFIG_XEN_CTRL_INTERFACE_VERSION
> > <
> > 40701 at the top? This way we don't have to add yet another ifdef.
> > 
> 
> Yes, this looks better and appears to DTRT. I assume you will just apply this version and don't want be to resubmit?

Actually, I would prefer if you could test it properly and resubmit the
patch. Thanks!


> 
> > diff --git a/include/hw/xen/xen_common.h
> > b/include/hw/xen/xen_common.h
> > index 4f3bd35..624fb86 100644
> > --- a/include/hw/xen/xen_common.h
> > +++ b/include/hw/xen/xen_common.h
> > @@ -27,6 +27,58 @@ extern xc_interface *xen_xc;
> >   * We don't support Xen prior to 4.2.0.
> >   */
> > 
> > +/* Xen 4.2 through 4.6 */
> > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
> > +
> > +typedef xc_interface xenforeignmemory_handle;
> > +typedef xc_evtchn xenevtchn_handle;
> > +typedef xc_gnttab xengnttab_handle;
> > +
> > +#define xenevtchn_open(l, f) xc_evtchn_open(l, f);
> > +#define xenevtchn_close(h) xc_evtchn_close(h)
> > +#define xenevtchn_fd(h) xc_evtchn_fd(h)
> > +#define xenevtchn_pending(h) xc_evtchn_pending(h)
> > +#define xenevtchn_notify(h, p) xc_evtchn_notify(h, p)
> > +#define xenevtchn_bind_interdomain(h, d, p)
> > xc_evtchn_bind_interdomain(h, d, p)
> > +#define xenevtchn_unmask(h, p) xc_evtchn_unmask(h, p)
> > +#define xenevtchn_unbind(h, p) xc_evtchn_unbind(h, p)
> > +
> > +#define xengnttab_open(l, f) xc_gnttab_open(l, f)
> > +#define xengnttab_close(h) xc_gnttab_close(h)
> > +#define xengnttab_set_max_grants(h, n) xc_gnttab_set_max_grants(h, n)
> > +#define xengnttab_map_grant_ref(h, d, r, p) xc_gnttab_map_grant_ref(h,
> > d, r, p)
> > +#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n)
> > +#define xengnttab_map_grant_refs(h, c, d, r, p) \
> > +    xc_gnttab_map_grant_refs(h, c, d, r, p)
> > +#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \
> > +    xc_gnttab_map_domain_grant_refs(h, c, d, r, p)
> > +
> > +#define xenforeignmemory_open(l, f) xen_xc
> > +#define xenforeignmemory_close(h)
> > +
> > +static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
> > +                                         int prot, size_t pages,
> > +                                         const xen_pfn_t arr[/*pages*/],
> > +                                         int err[/*pages*/])
> > +{
> > +    if (err)
> > +        return xc_map_foreign_bulk(h, dom, prot, arr, err, pages);
> > +    else
> > +        return xc_map_foreign_pages(h, dom, prot, arr, pages);
> > +}
> > +
> > +#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE)
> > +
> > +#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
> > +
> > +#include <xenevtchn.h>
> > +#include <xengnttab.h>
> > +#include <xenforeignmemory.h>
> > +
> > +#endif
> > +
> > +extern xenforeignmemory_handle *xen_fmem;
> > +
> >  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
> > 
> >  typedef xc_interface xendevicemodel_handle;
> > @@ -159,6 +211,13 @@ static inline int xendevicemodel_restrict(
> >      return -1;
> >  }
> > 
> > +static inline int xenforeignmemory_restrict(
> > +    xenforeignmemory_handle *fmem, domid_t domid)
> > +{
> > +    errno = ENOTTY;
> > +    return -1;
> > +}
> > +
> >  #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
> > 
> >  #include <xendevicemodel.h>
> > @@ -215,69 +274,32 @@ static inline int xen_modified_memory(domid_t
> > domid, uint64_t first_pfn,
> > 
> >  static inline int xen_restrict(domid_t domid)
> >  {
> > -    int rc = xendevicemodel_restrict(xen_dmod, domid);
> > +    int rc;
> > 
> > -    trace_xen_domid_restrict(errno);
> > +    /* Attempt to restrict devicemodel operations */
> > +    rc = xendevicemodel_restrict(xen_dmod, domid);
> > +    trace_xen_domid_restrict(rc ? errno : 0);
> > 
> > -    if (errno == ENOTTY) {
> > -        return 0;
> > +    if (rc < 0) {
> > +        /*
> > +         * If errno is ENOTTY then restriction is not implemented so
> > +         * there's no point in trying to restrict other types of
> > +         * operation, but it should not be treated as a failure.
> > +         */
> > +        if (errno == ENOTTY) {
> > +            return 0;
> > +        }
> > +
> > +        return rc;
> >      }
> > 
> > -    return rc;
> > -}
> > -
> > -/* Xen 4.2 through 4.6 */
> > -#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
> > -
> > -typedef xc_interface xenforeignmemory_handle;
> > -typedef xc_evtchn xenevtchn_handle;
> > -typedef xc_gnttab xengnttab_handle;
> > -
> > -#define xenevtchn_open(l, f) xc_evtchn_open(l, f);
> > -#define xenevtchn_close(h) xc_evtchn_close(h)
> > -#define xenevtchn_fd(h) xc_evtchn_fd(h)
> > -#define xenevtchn_pending(h) xc_evtchn_pending(h)
> > -#define xenevtchn_notify(h, p) xc_evtchn_notify(h, p)
> > -#define xenevtchn_bind_interdomain(h, d, p)
> > xc_evtchn_bind_interdomain(h, d, p)
> > -#define xenevtchn_unmask(h, p) xc_evtchn_unmask(h, p)
> > -#define xenevtchn_unbind(h, p) xc_evtchn_unbind(h, p)
> > -
> > -#define xengnttab_open(l, f) xc_gnttab_open(l, f)
> > -#define xengnttab_close(h) xc_gnttab_close(h)
> > -#define xengnttab_set_max_grants(h, n) xc_gnttab_set_max_grants(h, n)
> > -#define xengnttab_map_grant_ref(h, d, r, p) xc_gnttab_map_grant_ref(h,
> > d, r, p)
> > -#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n)
> > -#define xengnttab_map_grant_refs(h, c, d, r, p) \
> > -    xc_gnttab_map_grant_refs(h, c, d, r, p)
> > -#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \
> > -    xc_gnttab_map_domain_grant_refs(h, c, d, r, p)
> > -
> > -#define xenforeignmemory_open(l, f) xen_xc
> > -#define xenforeignmemory_close(h)
> > +    /* Restrict foreignmemory operations */
> > +    rc = xenforeignmemory_restrict(xen_fmem, domid);
> > +    trace_xen_domid_restrict(rc ? errno : 0);
> > 
> > -static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
> > -                                         int prot, size_t pages,
> > -                                         const xen_pfn_t arr[/*pages*/],
> > -                                         int err[/*pages*/])
> > -{
> > -    if (err)
> > -        return xc_map_foreign_bulk(h, dom, prot, arr, err, pages);
> > -    else
> > -        return xc_map_foreign_pages(h, dom, prot, arr, pages);
> > +    return rc;
> >  }
> > 
> > -#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE)
> > -
> > -#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
> > -
> > -#include <xenevtchn.h>
> > -#include <xengnttab.h>
> > -#include <xenforeignmemory.h>
> > -
> > -#endif
> > -
> > -extern xenforeignmemory_handle *xen_fmem;
> > -
> >  void destroy_hvm_domain(bool reboot);
> > 
> >  /* shutdown/destroy current domain because of an error */
> 

Re: [Qemu-devel] [PATCH] xen: additionally restrict xenforeignmemory operations

[Paul Durrant]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 27 March 2017 18:46
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: 'Stefano Stabellini' <sstabellini@kernel.org>; qemu-devel@nongnu.org;
> xen-devel@lists.xenproject.org; Anthony Perard
> <anthony.perard@citrix.com>
> Subject: RE: [PATCH] xen: additionally restrict xenforeignmemory operations
> 
> On Mon, 27 Mar 2017, Paul Durrant wrote:
> > > -----Original Message-----
> > [snip]
> > >
> > > This is OK but the file is growing too entangled. What do you think of
> > > the following, which moves the if
> CONFIG_XEN_CTRL_INTERFACE_VERSION
> > > <
> > > 40701 at the top? This way we don't have to add yet another ifdef.
> > >
> >
> > Yes, this looks better and appears to DTRT. I assume you will just apply this
> version and don't want be to resubmit?
> 
> Actually, I would prefer if you could test it properly and resubmit the
> patch. Thanks!
> 

Ok, sure. I gave it a quick try yesterday but I'll send it as v2 later.

  Paul

> 
> >
> > > diff --git a/include/hw/xen/xen_common.h
> > > b/include/hw/xen/xen_common.h
> > > index 4f3bd35..624fb86 100644
> > > --- a/include/hw/xen/xen_common.h
> > > +++ b/include/hw/xen/xen_common.h
> > > @@ -27,6 +27,58 @@ extern xc_interface *xen_xc;
> > >   * We don't support Xen prior to 4.2.0.
> > >   */
> > >
> > > +/* Xen 4.2 through 4.6 */
> > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
> > > +
> > > +typedef xc_interface xenforeignmemory_handle;
> > > +typedef xc_evtchn xenevtchn_handle;
> > > +typedef xc_gnttab xengnttab_handle;
> > > +
> > > +#define xenevtchn_open(l, f) xc_evtchn_open(l, f);
> > > +#define xenevtchn_close(h) xc_evtchn_close(h)
> > > +#define xenevtchn_fd(h) xc_evtchn_fd(h)
> > > +#define xenevtchn_pending(h) xc_evtchn_pending(h)
> > > +#define xenevtchn_notify(h, p) xc_evtchn_notify(h, p)
> > > +#define xenevtchn_bind_interdomain(h, d, p)
> > > xc_evtchn_bind_interdomain(h, d, p)
> > > +#define xenevtchn_unmask(h, p) xc_evtchn_unmask(h, p)
> > > +#define xenevtchn_unbind(h, p) xc_evtchn_unbind(h, p)
> > > +
> > > +#define xengnttab_open(l, f) xc_gnttab_open(l, f)
> > > +#define xengnttab_close(h) xc_gnttab_close(h)
> > > +#define xengnttab_set_max_grants(h, n)
> xc_gnttab_set_max_grants(h, n)
> > > +#define xengnttab_map_grant_ref(h, d, r, p)
> xc_gnttab_map_grant_ref(h,
> > > d, r, p)
> > > +#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n)
> > > +#define xengnttab_map_grant_refs(h, c, d, r, p) \
> > > +    xc_gnttab_map_grant_refs(h, c, d, r, p)
> > > +#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \
> > > +    xc_gnttab_map_domain_grant_refs(h, c, d, r, p)
> > > +
> > > +#define xenforeignmemory_open(l, f) xen_xc
> > > +#define xenforeignmemory_close(h)
> > > +
> > > +static inline void *xenforeignmemory_map(xc_interface *h, uint32_t
> dom,
> > > +                                         int prot, size_t pages,
> > > +                                         const xen_pfn_t arr[/*pages*/],
> > > +                                         int err[/*pages*/])
> > > +{
> > > +    if (err)
> > > +        return xc_map_foreign_bulk(h, dom, prot, arr, err, pages);
> > > +    else
> > > +        return xc_map_foreign_pages(h, dom, prot, arr, pages);
> > > +}
> > > +
> > > +#define xenforeignmemory_unmap(h, p, s) munmap(p, s *
> XC_PAGE_SIZE)
> > > +
> > > +#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
> > > +
> > > +#include <xenevtchn.h>
> > > +#include <xengnttab.h>
> > > +#include <xenforeignmemory.h>
> > > +
> > > +#endif
> > > +
> > > +extern xenforeignmemory_handle *xen_fmem;
> > > +
> > >  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
> > >
> > >  typedef xc_interface xendevicemodel_handle;
> > > @@ -159,6 +211,13 @@ static inline int xendevicemodel_restrict(
> > >      return -1;
> > >  }
> > >
> > > +static inline int xenforeignmemory_restrict(
> > > +    xenforeignmemory_handle *fmem, domid_t domid)
> > > +{
> > > +    errno = ENOTTY;
> > > +    return -1;
> > > +}
> > > +
> > >  #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
> > >
> > >  #include <xendevicemodel.h>
> > > @@ -215,69 +274,32 @@ static inline int xen_modified_memory(domid_t
> > > domid, uint64_t first_pfn,
> > >
> > >  static inline int xen_restrict(domid_t domid)
> > >  {
> > > -    int rc = xendevicemodel_restrict(xen_dmod, domid);
> > > +    int rc;
> > >
> > > -    trace_xen_domid_restrict(errno);
> > > +    /* Attempt to restrict devicemodel operations */
> > > +    rc = xendevicemodel_restrict(xen_dmod, domid);
> > > +    trace_xen_domid_restrict(rc ? errno : 0);
> > >
> > > -    if (errno == ENOTTY) {
> > > -        return 0;
> > > +    if (rc < 0) {
> > > +        /*
> > > +         * If errno is ENOTTY then restriction is not implemented so
> > > +         * there's no point in trying to restrict other types of
> > > +         * operation, but it should not be treated as a failure.
> > > +         */
> > > +        if (errno == ENOTTY) {
> > > +            return 0;
> > > +        }
> > > +
> > > +        return rc;
> > >      }
> > >
> > > -    return rc;
> > > -}
> > > -
> > > -/* Xen 4.2 through 4.6 */
> > > -#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
> > > -
> > > -typedef xc_interface xenforeignmemory_handle;
> > > -typedef xc_evtchn xenevtchn_handle;
> > > -typedef xc_gnttab xengnttab_handle;
> > > -
> > > -#define xenevtchn_open(l, f) xc_evtchn_open(l, f);
> > > -#define xenevtchn_close(h) xc_evtchn_close(h)
> > > -#define xenevtchn_fd(h) xc_evtchn_fd(h)
> > > -#define xenevtchn_pending(h) xc_evtchn_pending(h)
> > > -#define xenevtchn_notify(h, p) xc_evtchn_notify(h, p)
> > > -#define xenevtchn_bind_interdomain(h, d, p)
> > > xc_evtchn_bind_interdomain(h, d, p)
> > > -#define xenevtchn_unmask(h, p) xc_evtchn_unmask(h, p)
> > > -#define xenevtchn_unbind(h, p) xc_evtchn_unbind(h, p)
> > > -
> > > -#define xengnttab_open(l, f) xc_gnttab_open(l, f)
> > > -#define xengnttab_close(h) xc_gnttab_close(h)
> > > -#define xengnttab_set_max_grants(h, n) xc_gnttab_set_max_grants(h,
> n)
> > > -#define xengnttab_map_grant_ref(h, d, r, p)
> xc_gnttab_map_grant_ref(h,
> > > d, r, p)
> > > -#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n)
> > > -#define xengnttab_map_grant_refs(h, c, d, r, p) \
> > > -    xc_gnttab_map_grant_refs(h, c, d, r, p)
> > > -#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \
> > > -    xc_gnttab_map_domain_grant_refs(h, c, d, r, p)
> > > -
> > > -#define xenforeignmemory_open(l, f) xen_xc
> > > -#define xenforeignmemory_close(h)
> > > +    /* Restrict foreignmemory operations */
> > > +    rc = xenforeignmemory_restrict(xen_fmem, domid);
> > > +    trace_xen_domid_restrict(rc ? errno : 0);
> > >
> > > -static inline void *xenforeignmemory_map(xc_interface *h, uint32_t
> dom,
> > > -                                         int prot, size_t pages,
> > > -                                         const xen_pfn_t arr[/*pages*/],
> > > -                                         int err[/*pages*/])
> > > -{
> > > -    if (err)
> > > -        return xc_map_foreign_bulk(h, dom, prot, arr, err, pages);
> > > -    else
> > > -        return xc_map_foreign_pages(h, dom, prot, arr, pages);
> > > +    return rc;
> > >  }
> > >
> > > -#define xenforeignmemory_unmap(h, p, s) munmap(p, s *
> XC_PAGE_SIZE)
> > > -
> > > -#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
> > > -
> > > -#include <xenevtchn.h>
> > > -#include <xengnttab.h>
> > > -#include <xenforeignmemory.h>
> > > -
> > > -#endif
> > > -
> > > -extern xenforeignmemory_handle *xen_fmem;
> > > -
> > >  void destroy_hvm_domain(bool reboot);
> > >
> > >  /* shutdown/destroy current domain because of an error */
> >