[PATCH] avoid UB in guest handle arithmetic

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- 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)))
 
--- 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;
                 }
--- 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) ({       \