| Message ID | 20240619163100.2556555-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/guest_access: Fix accessors for 32bit builds of Xen | expand |
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
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)); \ })
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(-) base-commit: 43d5c5d5f70b3f5419e7ef30399d23adf6ddfa8e