avoid UB in guest handle arithmetic

Commit Message

Jan Beulich March 19, 2024, 1:26 p.m. UTC

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 <[e-mail]>
Signed-off-by: Jan Beulich <[e-mail]>

Comments

Stewart Hildebrand March 22, 2024, 1:39 p.m. UTC | #1

On 3/19/24 09:26, Jan Beulich wrote:
> 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"

Why not uintptr_t?

Jan Beulich March 22, 2024, 2:14 p.m. UTC | #2

On 22.03.2024 14:39, Stewart Hildebrand wrote:
> On 3/19/24 09:26, Jan Beulich wrote:
>> 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"
> 
> Why not uintptr_t?

Because, like Linux, so far we've been using unsigned long in preference.
That might change ...

Jan

Patch

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