xen: avoid UB in guest handle arithmetic

At least XENMEM_memory_exchange can have huge values passed in the
nr_extents and nr_exchanged fields. Adding such values to pointers can
overflow, resulting in UB. Cast respective pointers to "unsigned long"
while at the same time making the necessary multiplication explicit.
Remaining arithmetic is, despite there possibly being mathematical
overflow, okay as per the C99 spec: "A computation involving unsigned
operands can never overflow, because a result that cannot be represented
by the resulting unsigned integer type is reduced modulo the number that
is one greater than the largest value that can be represented by the
resulting type." The overflow that we need to guard against is checked
for in array_access_ok().

Note that in / down from array_access_ok() the address value is only
ever cast to "unsigned long" anyway, which is why in the invocation from
guest_handle_subrange_okay() the value doesn't need casting back to
pointer type.

In compat grant table code change two guest_handle_add_offset() to avoid
passing in negative offsets.

Since {,__}clear_guest_offset() need touching anyway, also deal with
another (latent) issue there: They were losing the handle type, i.e. the
size of the individual objects accessed. Luckily the few users we
presently have all pass char or uint8 handles.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-Acked-By: Oleksii Kurochko <oleksii.kurochko@gmail.com>

diff --git a/xen/arch/x86/include/asm/guest_access.h b/xen/arch/x86/include/asm/guest_access.h
index dbf789fa587ec7933f60c4f31982879afc231147..69716c8b41bbc2251d48c40a9d8a3cd9969e7f57 100644 (file)
--- a/xen/arch/x86/include/asm/guest_access.h
+++ b/xen/arch/x86/include/asm/guest_access.h
@@ -43,7 +43,7 @@
      array_access_ok((hnd).p, (nr), sizeof(*(hnd).p)))
 #define guest_handle_subrange_okay(hnd, first, last)    \
     (paging_mode_external(current->domain) ||           \
-     array_access_ok((hnd).p + (first),                 \
+     array_access_ok((unsigned long)(hnd).p + (first) * sizeof(*(hnd).p), \
                      (last)-(first)+1,                  \
                      sizeof(*(hnd).p)))
 

diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index af98eade17c9ea2549a9a3ae1cb743febaa7e66f..c495c375eb2b25541a810ee7cc3d3c41b36e92fc 100644 (file)
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -232,7 +232,7 @@ int compat_grant_table_op(
                 cnt_uop = guest_handle_cast(xfer, void);
                 while ( n-- )
                 {
-                    guest_handle_add_offset(xfer, -1);
+                    guest_handle_subtract_offset(xfer, 1);
                     if ( __copy_field_to_guest(xfer, nat.xfer + n, status) )
                         rc = -EFAULT;
                 }
@@ -277,7 +277,7 @@ int compat_grant_table_op(
                 cnt_uop = guest_handle_cast(copy, void);
                 while ( n-- )
                 {
-                    guest_handle_add_offset(copy, -1);
+                    guest_handle_subtract_offset(copy, 1);
                     if ( __copy_field_to_guest(copy, nat.copy + n, status) )
                         rc = -EFAULT;
                 }

diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
index af33ae3ab65267ddf32055a9790eaa2bc211a32a..96dbef2e0251a97ce6f433e06266f723852a426c 100644 (file)
--- a/xen/include/xen/guest_access.h
+++ b/xen/include/xen/guest_access.h
@@ -15,8 +15,10 @@
 #define guest_handle_is_null(hnd)        ((hnd).p == NULL)
 
 /* Offset the given guest handle into the array it refers to. */
-#define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
-#define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
+#define guest_handle_add_offset(hnd, nr) ((hnd).p = \
+    (typeof((hnd).p))((unsigned long)(hnd).p + (nr) * sizeof(*(hnd).p)))
+#define guest_handle_subtract_offset(hnd, nr) ((hnd).p = \
+    (typeof((hnd).p))((unsigned long)(hnd).p - (nr) * sizeof(*(hnd).p)))
 
 /*
  * Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
@@ -59,20 +61,22 @@
  */
 #define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
     const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    unsigned long d_ = (unsigned long)(hnd).p;          \
     /* Check that the handle is not for a const type */ \
     void *__maybe_unused _t = (hnd).p;                  \
     (void)((hnd).p == _s);                              \
-    raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
+    raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
+                      _s, (nr) * sizeof(*_s));          \
 })
 
 /*
  * Clear an array of objects in guest context via a guest handle,
  * specifying an offset into the guest array.
  */
-#define clear_guest_offset(hnd, off, nr) ({    \
-    void *_d = (hnd).p;                        \
-    raw_clear_guest(_d+(off), nr);             \
+#define clear_guest_offset(hnd, off, nr) ({             \
+    unsigned long d_ = (unsigned long)(hnd).p;          \
+    raw_clear_guest((void *)(d_ + (off) * sizeof(*(hnd).p)), \
+                    (nr) * sizeof(*(hnd).p));           \
 })
 
 /*
@@ -80,9 +84,11 @@
  * specifying an offset into the guest array.
  */
 #define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
+    unsigned long s_ = (unsigned long)(hnd).p;          \
     typeof(*(ptr)) *_d = (ptr);                         \
-    raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
+    raw_copy_from_guest(_d,                             \
+                        (const void *)(s_ + (off) * sizeof(*_d)), \
+                        (nr) * sizeof(*_d));            \
 })
 
 /* Copy sub-field of a structure to guest context via a guest handle. */
@@ -117,22 +123,26 @@
 
 #define __copy_to_guest_offset(hnd, off, ptr, nr) ({        \
     const typeof(*(ptr)) *_s = (ptr);                       \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;              \
+    unsigned long d_ = (unsigned long)(hnd).p;              \
     /* Check that the handle is not for a const type */     \
     void *__maybe_unused _t = (hnd).p;                      \
     (void)((hnd).p == _s);                                  \
-    __raw_copy_to_guest(_d + (off), _s, sizeof(*_s) * (nr));\
+    __raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
+                      _s, (nr) * sizeof(*_s));              \
 })
 
-#define __clear_guest_offset(hnd, off, nr) ({    \
-    void *_d = (hnd).p;                          \
-    __raw_clear_guest(_d + (off), nr);           \
+#define __clear_guest_offset(hnd, off, nr) ({               \
+    unsigned long d_ = (unsigned long)(hnd).p;              \
+    __raw_clear_guest((void *)(d_ + (off) * sizeof(*(hnd).p)), \
+                      (nr) * sizeof(*(hnd).p));             \
 })
 
 #define __copy_from_guest_offset(ptr, hnd, off, nr) ({          \
-    const typeof(*(ptr)) *_s = (hnd).p;                         \
+    unsigned long s_ = (unsigned long)(hnd).p;                  \
     typeof(*(ptr)) *_d = (ptr);                                 \
-    __raw_copy_from_guest(_d, _s + (off), sizeof (*_d) * (nr)); \
+    __raw_copy_from_guest(_d,                                   \
+                          (const void *)(s_ + (off) * sizeof(*_d)), \
+                          (nr) * sizeof(*_d));                  \
 })
 
 #define __copy_field_to_guest(hnd, ptr, field) ({       \