[PATCH] xen/spinlock: Fix UBSAN "load of address with insufficient space" in lock_prof_init()

Andrew Cooper posted 1 patch 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20241014145710.3295232-1-andrew.cooper3@citrix.com
xen/common/spinlock.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
[PATCH] xen/spinlock: Fix UBSAN "load of address with insufficient space" in lock_prof_init()
Posted by Andrew Cooper 1 month, 1 week ago
UBSAN complains:

  (XEN) ================================================================================
  (XEN) UBSAN: Undefined behaviour in common/spinlock.c:794:10
  (XEN) load of address ffff82d040ae24c8 with insufficient space
  (XEN) for an object of type 'struct lock_profile *'
  (XEN) ----[ Xen-4.20-unstable  x86_64  debug=y ubsan=y  Tainted:   C    ]----

This shows up with GCC-14, but not with GCC-12.  I have not bisected further.

Either way, the types for __lock_profile_{start,end} are incorrect.

They are an array of struct lock_profile pointers.  Correct the extern's
types, and adjust the loop to match.

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Juergen Gross <jgross@suse.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Found as collatoral damage in
https://github.com/QubesOS/qubes-issues/issues/9501 but it's not related to
the main bug being repoted.

A reported-by tag TBC.
---
 xen/common/spinlock.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 0b877384451d..38caa10a2ea2 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -608,9 +608,6 @@ struct lock_profile_anc {
 typedef void lock_profile_subfunc(struct lock_profile *data, int32_t type,
     int32_t idx, void *par);
 
-extern struct lock_profile *__lock_profile_start;
-extern struct lock_profile *__lock_profile_end;
-
 static s_time_t lock_profile_start;
 static struct lock_profile_anc lock_profile_ancs[] = {
     [LOCKPROF_TYPE_GLOBAL] = { .name = "Global" },
@@ -780,13 +777,16 @@ void _lock_profile_deregister_struct(
     spin_unlock(&lock_profile_lock);
 }
 
+extern struct lock_profile *__lock_profile_start[];
+extern struct lock_profile *__lock_profile_end[];
+
 static int __init cf_check lock_prof_init(void)
 {
     struct lock_profile **q;
 
     BUILD_BUG_ON(ARRAY_SIZE(lock_profile_ancs) != LOCKPROF_TYPE_N);
 
-    for ( q = &__lock_profile_start; q < &__lock_profile_end; q++ )
+    for ( q = __lock_profile_start; q < __lock_profile_end; q++ )
     {
         (*q)->next = lock_profile_glb_q.elem_q;
         lock_profile_glb_q.elem_q = *q;
-- 
2.39.5


Re: [PATCH] xen/spinlock: Fix UBSAN "load of address with insufficient space" in lock_prof_init()
Posted by Jürgen Groß 1 month, 1 week ago
On 14.10.24 16:57, Andrew Cooper wrote:
> UBSAN complains:
> 
>    (XEN) ================================================================================
>    (XEN) UBSAN: Undefined behaviour in common/spinlock.c:794:10
>    (XEN) load of address ffff82d040ae24c8 with insufficient space
>    (XEN) for an object of type 'struct lock_profile *'
>    (XEN) ----[ Xen-4.20-unstable  x86_64  debug=y ubsan=y  Tainted:   C    ]----
> 
> This shows up with GCC-14, but not with GCC-12.  I have not bisected further.
> 
> Either way, the types for __lock_profile_{start,end} are incorrect.
> 
> They are an array of struct lock_profile pointers.  Correct the extern's
> types, and adjust the loop to match.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH] xen/spinlock: Fix UBSAN "load of address with insufficient space" in lock_prof_init()
Posted by Andrew Cooper 1 month, 1 week ago
On 14/10/2024 4:12 pm, Jürgen Groß wrote:
> On 14.10.24 16:57, Andrew Cooper wrote:
>> UBSAN complains:
>>
>>    (XEN)
>> ================================================================================
>>    (XEN) UBSAN: Undefined behaviour in common/spinlock.c:794:10
>>    (XEN) load of address ffff82d040ae24c8 with insufficient space
>>    (XEN) for an object of type 'struct lock_profile *'
>>    (XEN) ----[ Xen-4.20-unstable  x86_64  debug=y ubsan=y  Tainted:  
>> C    ]----
>>
>> This shows up with GCC-14, but not with GCC-12.  I have not bisected
>> further.
>>
>> Either way, the types for __lock_profile_{start,end} are incorrect.
>>
>> They are an array of struct lock_profile pointers.  Correct the extern's
>> types, and adjust the loop to match.
>>
>> No practical change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks, and I've got proper Reported-by tag from the conversation on Matrix.

~Andrew