Gitlab CI reports an ARM32 randconfig failure as follows:

  In file included from common/livepatch.c:9:
  common/livepatch.c: In function ‘livepatch_list’:
  ./include/xen/guest_access.h:130:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
    130 |     __raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
        |                         ^
  common/livepatch.c:1283:18: note: in expansion of macro ‘__copy_to_guest_offset’
   1283 |             if ( __copy_to_guest_offset(list->name, name_offset,
        |                  ^~~~~~~~~~~~~~~~~~~~~~
  ./include/xen/guest_access.h:130:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
    130 |     __raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
        |                         ^
  common/livepatch.c:1287:17: note: in expansion of macro ‘__copy_to_guest_offset’
   1287 |                 __copy_to_guest_offset(list->metadata, metadata_offset,
        |                 ^~~~~~~~~~~~~~~~~~~~~~

This isn't specific to ARM32; it's LIVEPATCH on any 32bit build of Xen.

Both name_offset and metadata_offset are uint64_t, meaning that the
expression:

  (d_ + (off) * sizeof(*(hnd).p)

gets promoted to uint64_t, and is too wide to cast back to a pointer in 32bit
builds.  The expression needs casting through (unsigned long) before it can be
cast to (void *).

Xen has the _p() wrapper to do precisely this.

No functional change.

Link: https://gitlab.com/xen-project/xen/-/jobs/7136417308
Fixes: 43d5c5d5f70b ("xen: avoid UB in guest handle arithmetic")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>

For 4.19, as this was found by CI.
---
 xen/include/xen/guest_access.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
index 96dbef2e0251..969813762c25 100644
--- a/xen/include/xen/guest_access.h
+++ b/xen/include/xen/guest_access.h
@@ -65,7 +65,7 @@
     /* Check that the handle is not for a const type */ \
     void *__maybe_unused _t = (hnd).p;                  \
     (void)((hnd).p == _s);                              \
-    raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
+    raw_copy_to_guest(_p(d_ + (off) * sizeof(*_s)),     \
                       _s, (nr) * sizeof(*_s));          \
 })
 
@@ -75,7 +75,7 @@
  */
 #define clear_guest_offset(hnd, off, nr) ({             \
     unsigned long d_ = (unsigned long)(hnd).p;          \
-    raw_clear_guest((void *)(d_ + (off) * sizeof(*(hnd).p)), \
+    raw_clear_guest(_p(d_ + (off) * sizeof(*(hnd).p)),  \
                     (nr) * sizeof(*(hnd).p));           \
 })
 
@@ -87,7 +87,7 @@
     unsigned long s_ = (unsigned long)(hnd).p;          \
     typeof(*(ptr)) *_d = (ptr);                         \
     raw_copy_from_guest(_d,                             \
-                        (const void *)(s_ + (off) * sizeof(*_d)), \
+                        _p(s_ + (off) * sizeof(*_d)),   \
                         (nr) * sizeof(*_d));            \
 })
 
@@ -127,13 +127,13 @@
     /* Check that the handle is not for a const type */     \
     void *__maybe_unused _t = (hnd).p;                      \
     (void)((hnd).p == _s);                                  \
-    __raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
+    __raw_copy_to_guest(_p(d_ + (off) * sizeof(*_s)),       \
                       _s, (nr) * sizeof(*_s));              \
 })
 
 #define __clear_guest_offset(hnd, off, nr) ({               \
     unsigned long d_ = (unsigned long)(hnd).p;              \
-    __raw_clear_guest((void *)(d_ + (off) * sizeof(*(hnd).p)), \
+    __raw_clear_guest(_p(d_ + (off) * sizeof(*(hnd).p)),    \
                       (nr) * sizeof(*(hnd).p));             \
 })
 
@@ -141,7 +141,7 @@
     unsigned long s_ = (unsigned long)(hnd).p;                  \
     typeof(*(ptr)) *_d = (ptr);                                 \
     __raw_copy_from_guest(_d,                                   \
-                          (const void *)(s_ + (off) * sizeof(*_d)), \
+                          _p(s_ + (off) * sizeof(*_d)),         \
                           (nr) * sizeof(*_d));                  \
 })
 

base-commit: 43d5c5d5f70b3f5419e7ef30399d23adf6ddfa8e
-- 
2.39.2

On 19.06.2024 18:31, Andrew Cooper wrote:
> Gitlab CI reports an ARM32 randconfig failure as follows:
> 
>   In file included from common/livepatch.c:9:
>   common/livepatch.c: In function ‘livepatch_list’:
>   ./include/xen/guest_access.h:130:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>     130 |     __raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
>         |                         ^
>   common/livepatch.c:1283:18: note: in expansion of macro ‘__copy_to_guest_offset’
>    1283 |             if ( __copy_to_guest_offset(list->name, name_offset,
>         |                  ^~~~~~~~~~~~~~~~~~~~~~
>   ./include/xen/guest_access.h:130:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>     130 |     __raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
>         |                         ^
>   common/livepatch.c:1287:17: note: in expansion of macro ‘__copy_to_guest_offset’
>    1287 |                 __copy_to_guest_offset(list->metadata, metadata_offset,
>         |                 ^~~~~~~~~~~~~~~~~~~~~~
> 
> This isn't specific to ARM32; it's LIVEPATCH on any 32bit build of Xen.
> 
> Both name_offset and metadata_offset are uint64_t, meaning that the
> expression:
> 
>   (d_ + (off) * sizeof(*(hnd).p)
> 
> gets promoted to uint64_t, and is too wide to cast back to a pointer in 32bit
> builds.  The expression needs casting through (unsigned long) before it can be
> cast to (void *).

I disagree. Instead I'd like to raise the question why these two local variables
are uint64_t in the first place. They accumulate buffer size, and hence ought to
have been size_t from the beginning. I'll make an alternative patch (first making
sure I test livepatch building not only for x86 and arm64).

> @@ -65,7 +65,7 @@
>      /* Check that the handle is not for a const type */ \
>      void *__maybe_unused _t = (hnd).p;                  \
>      (void)((hnd).p == _s);                              \
> -    raw_copy_to_guest((void *)(d_ + (off) * sizeof(*_s)), \
> +    raw_copy_to_guest(_p(d_ + (off) * sizeof(*_s)),     \

It's also from an abstract perspective that I disagree with using _p() like this.
We'd rather keep this as straightforward as possible, to keep down the risk of
hiding bugs by excess casting.

Jan