Xen Security Advisory 125 (CVE-2015-2752) - Long latency MMIO mapping operations are not preemptible

From 98670acc98cad5aee0e0714694a64d3b96675c36 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <[email protected]>
Date: Wed, 19 Nov 2014 12:57:11 -0500
Subject: [PATCH] Limit XEN_DOMCTL_memory_mapping hypercall to only process up
 to 64 GFNs (or less)

Said hypercall for large BARs can take quite a while. As such
we can require that the hypercall MUST break up the request
in smaller values.

Another approach is to add preemption to it - whether we do the
preemption using hypercall_create_continuation or returning
EAGAIN to userspace (and have it re-invocate the call) - either
way the issue we cannot easily solve is that in 'map_mmio_regions'
if we encounter an error we MUST call 'unmap_mmio_regions' for the
whole BAR region.

Since the preemption would re-use input fields such as nr_mfns,
first_gfn, first_mfn - we would lose the original values -
and only undo what was done in the current round (i.e. ignoring
anything that was done prior to earlier preemptions).

Unless we re-used the return value as 'EAGAIN|nr_mfns_done<<10' but
that puts a limit (since the return value is a long) on the amount
of nr_mfns that can provided.

This patch sidesteps this problem by:
 - Setting an hard limit of nr_mfns having to be 64 or less.
 - Toolstack adjusts correspondingly to the nr_mfn limit.
 - If the there is an error when adding the toolstack will call the
   remove operation to remove the whole region.

The need to break this hypercall down is for large BARs can take
more than the guest (initial domain usually) time-slice. This has
the negative result in that the guest is locked out for a long
duration and is unable to act on any pending events.

We also augment the code to return zero if nr_mfns instead
of trying to the hypercall.

Suggested-by: Jan Beulich <[email protected]>
Acked-by: Jan Beulich <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
Acked-by: Ian Campbell <[email protected]>
---
[v50: Simplify loop]
[v51: If max_batch_sz 1 (or less) we would return zero. Fix that]
[v52: Handle nr_mfns being zero]
[v53: Fix up return value]
---
 tools/libxc/xc_domain.c     | 46 +++++++++++++++++++++++++++++++++++++++++----
 xen/common/domctl.c         |  5 +++++
 xen/include/public/domctl.h |  1 +
 3 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 845d1d7..bba7672 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1988,6 +1988,8 @@ int xc_domain_memory_mapping(
 {
     DECLARE_DOMCTL;
     xc_dominfo_t info;
+    int ret = 0, err;
+    unsigned long done = 0, nr, max_batch_sz;
 
     if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 ||
          info.domid != domid )
@@ -1998,14 +2000,50 @@ int xc_domain_memory_mapping(
     if ( !xc_core_arch_auto_translated_physmap(&info) )
         return 0;
 
+    if ( !nr_mfns )
+        return 0;
+
     domctl.cmd = XEN_DOMCTL_memory_mapping;
     domctl.domain = domid;
-    domctl.u.memory_mapping.first_gfn = first_gfn;
-    domctl.u.memory_mapping.first_mfn = first_mfn;
-    domctl.u.memory_mapping.nr_mfns = nr_mfns;
     domctl.u.memory_mapping.add_mapping = add_mapping;
+    max_batch_sz = nr_mfns;
+    do
+    {
+        nr = min(nr_mfns - done, max_batch_sz);
+        domctl.u.memory_mapping.nr_mfns = nr;
+        domctl.u.memory_mapping.first_gfn = first_gfn + done;
+        domctl.u.memory_mapping.first_mfn = first_mfn + done;
+        err = do_domctl(xch, &domctl);
+        if ( err && errno == E2BIG )
+        {
+            if ( max_batch_sz <= 1 )
+                break;
+            max_batch_sz >>= 1;
+            continue;
+        }
+        /* Save the first error... */
+        if ( !ret )
+            ret = err;
+        /* .. and ignore the rest of them when removing. */
+        if ( err && add_mapping != DPCI_REMOVE_MAPPING )
+            break;
 
-    return do_domctl(xch, &domctl);
+        done += nr;
+    } while ( done < nr_mfns );
+
+    /*
+     * Undo what we have done unless unmapping, by unmapping the entire region.
+     * Errors here are ignored.
+     */
+    if ( ret && add_mapping != DPCI_REMOVE_MAPPING )
+        xc_domain_memory_mapping(xch, domid, first_gfn, first_mfn, nr_mfns,
+                                 DPCI_REMOVE_MAPPING);
+
+    /* We might get E2BIG so many times that we never advance. */
+    if ( !done && !ret )
+        ret = -1;
+
+    return ret;
 }
 
 int xc_domain_ioport_mapping(
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index d396cc4..c2e60a7 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1027,6 +1027,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
              (gfn + nr_mfns - 1) < gfn ) /* wrap? */
             break;
 
+        ret = -E2BIG;
+        /* Must break hypercall up as this could take a while. */
+        if ( nr_mfns > 64 )
+            break;
+
         ret = -EPERM;
         if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
              !iomem_access_permitted(d, mfn, mfn_end) )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index ca0e51e..0c9f474 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -543,6 +543,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_bind_pt_irq_t);
 
 
 /* Bind machine I/O address range -> HVM address range. */
+/* If this returns -E2BIG lower nr_mfns value. */
 /* XEN_DOMCTL_memory_mapping */
 #define DPCI_ADD_MAPPING         1
 #define DPCI_REMOVE_MAPPING      0
-- 
2.1.0

Limit XEN_DOMCTL_memory_mapping hypercall to only process up to 64 GFNs (or less)

Said hypercall for large BARs can take quite a while. As such
we can require that the hypercall MUST break up the request
in smaller values.

Another approach is to add preemption to it - whether we do the
preemption using hypercall_create_continuation or returning
EAGAIN to userspace (and have it re-invocate the call) - either
way the issue we cannot easily solve is that in 'map_mmio_regions'
if we encounter an error we MUST call 'unmap_mmio_regions' for the
whole BAR region.

Since the preemption would re-use input fields such as nr_mfns,
first_gfn, first_mfn - we would lose the original values -
and only undo what was done in the current round (i.e. ignoring
anything that was done prior to earlier preemptions).

Unless we re-used the return value as 'EAGAIN|nr_mfns_done<<10' but
that puts a limit (since the return value is a long) on the amount
of nr_mfns that can provided.

This patch sidesteps this problem by:
 - Setting an hard limit of nr_mfns having to be 64 or less.
 - Toolstack adjusts correspondingly to the nr_mfn limit.
 - If the there is an error when adding the toolstack will call the
   remove operation to remove the whole region.

The need to break this hypercall down is for large BARs can take
more than the guest (initial domain usually) time-slice. This has
the negative result in that the guest is locked out for a long
duration and is unable to act on any pending events.

We also augment the code to return zero if nr_mfns instead
of trying to the hypercall.

Suggested-by: Jan Beulich <[email protected]>
Acked-by: Jan Beulich <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
Acked-by: Ian Campbell <[email protected]>

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index d98e68b..60f7137 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1352,6 +1352,13 @@ int xc_domain_bind_pt_isa_irq(
                                   PT_IRQ_TYPE_ISA, 0, 0, 0, machine_irq));
 }
 
+#ifndef min
+#define min(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x < _y) ? _x : _y; })
+#endif
 int xc_domain_memory_mapping(
     xc_interface *xch,
     uint32_t domid,
@@ -1361,17 +1368,55 @@ int xc_domain_memory_mapping(
     uint32_t add_mapping)
 {
     DECLARE_DOMCTL;
+    int ret = 0, err;
+    unsigned long done = 0, nr, max_batch_sz;
+
+    if ( !nr_mfns )
+        return 0;
 
     domctl.cmd = XEN_DOMCTL_memory_mapping;
     domctl.domain = domid;
-    domctl.u.memory_mapping.first_gfn = first_gfn;
-    domctl.u.memory_mapping.first_mfn = first_mfn;
-    domctl.u.memory_mapping.nr_mfns = nr_mfns;
     domctl.u.memory_mapping.add_mapping = add_mapping;
+    max_batch_sz = nr_mfns;
+    do
+    {
+        nr = min(nr_mfns - done, max_batch_sz);
+        domctl.u.memory_mapping.nr_mfns = nr;
+        domctl.u.memory_mapping.first_gfn = first_gfn + done;
+        domctl.u.memory_mapping.first_mfn = first_mfn + done;
+        err = do_domctl(xch, &domctl);
+        if ( err && errno == E2BIG )
+        {
+            if ( max_batch_sz <= 1 )
+                break;
+            max_batch_sz >>= 1;
+            continue;
+        }
+        /* Save the first error... */
+        if ( !ret )
+            ret = err;
+        /* .. and ignore the rest of them when removing. */
+        if ( err && add_mapping != DPCI_REMOVE_MAPPING )
+            break;
 
-    return do_domctl(xch, &domctl);
-}
+        done += nr;
+    } while ( done < nr_mfns );
 
+    /*
+     * Undo what we have done unless unmapping, by unmapping the entire region.
+     * Errors here are ignored.
+     */
+    if ( ret && add_mapping != DPCI_REMOVE_MAPPING )
+        xc_domain_memory_mapping(xch, domid, first_gfn, first_mfn, nr_mfns,
+                                 DPCI_REMOVE_MAPPING);
+
+    /* We might get E2BIG so many times that we never advance. */
+    if ( !done && !ret )
+        ret = -1;
+
+    return ret;
+}
+#undef min
 int xc_domain_ioport_mapping(
     xc_interface *xch,
     uint32_t domid,
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 8bf8f09..c9a97c9 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -867,6 +867,11 @@ long arch_do_domctl(
              (gfn + nr_mfns - 1) < gfn ) /* wrap? */
             break;
 
+        ret = -E2BIG;
+        /* Must break hypercall up as this could take a while. */
+        if ( nr_mfns > 64 )
+            break;
+
         ret = -EPERM;
         if ( !IS_PRIV(current->domain) &&
              !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 240ceb9..b45911e 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -507,6 +507,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_bind_pt_irq_t);
 
 
 /* Bind machine I/O address range -> HVM address range. */
+/* If this returns -E2BIG lower nr_mfns value. */
 /* XEN_DOMCTL_memory_mapping */
 #define DPCI_ADD_MAPPING         1
 #define DPCI_REMOVE_MAPPING      0

Limit XEN_DOMCTL_memory_mapping hypercall to only process up to 64 GFNs (or less)

Said hypercall for large BARs can take quite a while. As such
we can require that the hypercall MUST break up the request
in smaller values.

Another approach is to add preemption to it - whether we do the
preemption using hypercall_create_continuation or returning
EAGAIN to userspace (and have it re-invocate the call) - either
way the issue we cannot easily solve is that in 'map_mmio_regions'
if we encounter an error we MUST call 'unmap_mmio_regions' for the
whole BAR region.

Since the preemption would re-use input fields such as nr_mfns,
first_gfn, first_mfn - we would lose the original values -
and only undo what was done in the current round (i.e. ignoring
anything that was done prior to earlier preemptions).

Unless we re-used the return value as 'EAGAIN|nr_mfns_done<<10' but
that puts a limit (since the return value is a long) on the amount
of nr_mfns that can provided.

This patch sidesteps this problem by:
 - Setting an hard limit of nr_mfns having to be 64 or less.
 - Toolstack adjusts correspondingly to the nr_mfn limit.
 - If the there is an error when adding the toolstack will call the
   remove operation to remove the whole region.

The need to break this hypercall down is for large BARs can take
more than the guest (initial domain usually) time-slice. This has
the negative result in that the guest is locked out for a long
duration and is unable to act on any pending events.

We also augment the code to return zero if nr_mfns instead
of trying to the hypercall.

Suggested-by: Jan Beulich <[email protected]>
Acked-by: Jan Beulich <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
Acked-by: Ian Campbell <[email protected]>

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 3257e2a..7386e58 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1467,6 +1467,13 @@ int xc_domain_bind_pt_isa_irq(
                                   PT_IRQ_TYPE_ISA, 0, 0, 0, machine_irq));
 }
 
+#ifndef min
+#define min(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x < _y) ? _x : _y; })
+#endif
 int xc_domain_memory_mapping(
     xc_interface *xch,
     uint32_t domid,
@@ -1476,17 +1483,55 @@ int xc_domain_memory_mapping(
     uint32_t add_mapping)
 {
     DECLARE_DOMCTL;
+    int ret = 0, err;
+    unsigned long done = 0, nr, max_batch_sz;
+
+    if ( !nr_mfns )
+        return 0;
 
     domctl.cmd = XEN_DOMCTL_memory_mapping;
     domctl.domain = domid;
-    domctl.u.memory_mapping.first_gfn = first_gfn;
-    domctl.u.memory_mapping.first_mfn = first_mfn;
-    domctl.u.memory_mapping.nr_mfns = nr_mfns;
     domctl.u.memory_mapping.add_mapping = add_mapping;
+    max_batch_sz = nr_mfns;
+    do
+    {
+        nr = min(nr_mfns - done, max_batch_sz);
+        domctl.u.memory_mapping.nr_mfns = nr;
+        domctl.u.memory_mapping.first_gfn = first_gfn + done;
+        domctl.u.memory_mapping.first_mfn = first_mfn + done;
+        err = do_domctl(xch, &domctl);
+        if ( err && errno == E2BIG )
+        {
+            if ( max_batch_sz <= 1 )
+                break;
+            max_batch_sz >>= 1;
+            continue;
+        }
+        /* Save the first error... */
+        if ( !ret )
+            ret = err;
+        /* .. and ignore the rest of them when removing. */
+        if ( err && add_mapping != DPCI_REMOVE_MAPPING )
+            break;
 
-    return do_domctl(xch, &domctl);
-}
+        done += nr;
+    } while ( done < nr_mfns );
 
+    /*
+     * Undo what we have done unless unmapping, by unmapping the entire region.
+     * Errors here are ignored.
+     */
+    if ( ret && add_mapping != DPCI_REMOVE_MAPPING )
+        xc_domain_memory_mapping(xch, domid, first_gfn, first_mfn, nr_mfns,
+                                 DPCI_REMOVE_MAPPING);
+
+    /* We might get E2BIG so many times that we never advance. */
+    if ( !done && !ret )
+        ret = -1;
+
+    return ret;
+}
+#undef min
 int xc_domain_ioport_mapping(
     xc_interface *xch,
     uint32_t domid,
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 173bf01..306297a 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -655,6 +655,11 @@ long arch_do_domctl(
              (gfn + nr_mfns - 1) < gfn ) /* wrap? */
             break;
 
+        ret = -E2BIG;
+        /* Must break hypercall up as this could take a while. */
+        if ( nr_mfns > 64 )
+            break;
+
         ret = -EPERM;
         if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
             break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index d381903..8f9d5c0 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -513,6 +513,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_bind_pt_irq_t);
 
 
 /* Bind machine I/O address range -> HVM address range. */
+/* If this returns -E2BIG lower nr_mfns value. */
 /* XEN_DOMCTL_memory_mapping */
 #define DPCI_ADD_MAPPING         1
 #define DPCI_REMOVE_MAPPING      0

From df2922ce672cc35500e2f3ba041441021f44b41c Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <[email protected]>
Date: Wed, 19 Nov 2014 12:57:11 -0500
Subject: [PATCH] Limit XEN_DOMCTL_memory_mapping hypercall to only process up
 to 64 GFNs (or less)

Said hypercall for large BARs can take quite a while. As such
we can require that the hypercall MUST break up the request
in smaller values.

Another approach is to add preemption to it - whether we do the
preemption using hypercall_create_continuation or returning
EAGAIN to userspace (and have it re-invocate the call) - either
way the issue we cannot easily solve is that in 'map_mmio_regions'
if we encounter an error we MUST call 'unmap_mmio_regions' for the
whole BAR region.

Since the preemption would re-use input fields such as nr_mfns,
first_gfn, first_mfn - we would lose the original values -
and only undo what was done in the current round (i.e. ignoring
anything that was done prior to earlier preemptions).

Unless we re-used the return value as 'EAGAIN|nr_mfns_done<<10' but
that puts a limit (since the return value is a long) on the amount
of nr_mfns that can provided.

This patch sidesteps this problem by:
 - Setting an hard limit of nr_mfns having to be 64 or less.
 - Toolstack adjusts correspondingly to the nr_mfn limit.
 - If the there is an error when adding the toolstack will call the
   remove operation to remove the whole region.

The need to break this hypercall down is for large BARs can take
more than the guest (initial domain usually) time-slice. This has
the negative result in that the guest is locked out for a long
duration and is unable to act on any pending events.

We also augment the code to return zero if nr_mfns instead
of trying to the hypercall.

Suggested-by: Jan Beulich <[email protected]>
Acked-by: Jan Beulich <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
Acked-by: Ian Campbell <[email protected]>
---
 tools/libxc/xc_domain.c     | 55 ++++++++++++++++++++++++++++++++++++++++-----
 xen/arch/x86/domctl.c       |  5 +++++
 xen/include/public/domctl.h |  1 +
 3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 369c3f3..40ca771 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1641,6 +1641,13 @@ failed:
     return -1;
 }
 
+#ifndef min
+#define min(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x < _y) ? _x : _y; })
+#endif
 int xc_domain_memory_mapping(
     xc_interface *xch,
     uint32_t domid,
@@ -1650,17 +1657,55 @@ int xc_domain_memory_mapping(
     uint32_t add_mapping)
 {
     DECLARE_DOMCTL;
+    int ret = 0, err;
+    unsigned long done = 0, nr, max_batch_sz;
+
+    if ( !nr_mfns )
+        return 0;
 
     domctl.cmd = XEN_DOMCTL_memory_mapping;
     domctl.domain = domid;
-    domctl.u.memory_mapping.first_gfn = first_gfn;
-    domctl.u.memory_mapping.first_mfn = first_mfn;
-    domctl.u.memory_mapping.nr_mfns = nr_mfns;
     domctl.u.memory_mapping.add_mapping = add_mapping;
+    max_batch_sz = nr_mfns;
+    do
+    {
+        nr = min(nr_mfns - done, max_batch_sz);
+        domctl.u.memory_mapping.nr_mfns = nr;
+        domctl.u.memory_mapping.first_gfn = first_gfn + done;
+        domctl.u.memory_mapping.first_mfn = first_mfn + done;
+        err = do_domctl(xch, &domctl);
+        if ( err && errno == E2BIG )
+        {
+            if ( max_batch_sz <= 1 )
+                break;
+            max_batch_sz >>= 1;
+            continue;
+        }
+        /* Save the first error... */
+        if ( !ret )
+            ret = err;
+        /* .. and ignore the rest of them when removing. */
+        if ( err && add_mapping != DPCI_REMOVE_MAPPING )
+            break;
 
-    return do_domctl(xch, &domctl);
-}
+        done += nr;
+    } while ( done < nr_mfns );
 
+    /*
+     * Undo what we have done unless unmapping, by unmapping the entire region.
+     * Errors here are ignored.
+     */
+    if ( ret && add_mapping != DPCI_REMOVE_MAPPING )
+        xc_domain_memory_mapping(xch, domid, first_gfn, first_mfn, nr_mfns,
+                                 DPCI_REMOVE_MAPPING);
+
+    /* We might get E2BIG so many times that we never advance. */
+    if ( !done && !ret )
+        ret = -1;
+
+    return ret;
+}
+#undef min
 int xc_domain_ioport_mapping(
     xc_interface *xch,
     uint32_t domid,
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a967b65..9b72c22 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -653,6 +653,11 @@ long arch_do_domctl(
              (gfn + nr_mfns - 1) < gfn ) /* wrap? */
             break;
 
+        ret = -E2BIG;
+        /* Must break hypercall up as this could take a while. */
+        if ( nr_mfns > 64 )
+            break;
+
         ret = -EPERM;
         if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
             break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index f22fe2e..c45bc59 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -518,6 +518,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_bind_pt_irq_t);
 
 
 /* Bind machine I/O address range -> HVM address range. */
+/* If this returns -E2BIG lower nr_mfns value. */
 /* XEN_DOMCTL_memory_mapping */
 #define DPCI_ADD_MAPPING         1
 #define DPCI_REMOVE_MAPPING      0
-- 
2.1.0