1. Patchew
  2. Xen
  3. View series

[PATCH] xen/sched: Fix UB shift in compat_set_timer_op()

Andrew Cooper posted 1 patch 2 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240130212706.74303-1-andrew.cooper3@citrix.com
xen/common/sched/compat.c    | 4 ++--
xen/include/hypercall-defs.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
[PATCH] xen/sched: Fix UB shift in compat_set_timer_op()
Posted by Andrew Cooper 2 months, 3 weeks ago 
Tamas reported this UBSAN failure from fuzzing:

  (XEN) ================================================================================
  (XEN) UBSAN: Undefined behaviour in common/sched/compat.c:48:37
  (XEN) left shift of negative value -2147425536
  (XEN) ----[ Xen-4.19-unstable  x86_64  debug=y ubsan=y  Not tainted ]----
  ...
  (XEN) Xen call trace:
  (XEN)    [<ffff82d040307c1c>] R ubsan.c#ubsan_epilogue+0xa/0xd9
  (XEN)    [<ffff82d040308afb>] F __ubsan_handle_shift_out_of_bounds+0x11a/0x1c5
  (XEN)    [<ffff82d040307758>] F compat_set_timer_op+0x41/0x43
  (XEN)    [<ffff82d04040e4cc>] F hvm_do_multicall_call+0x77f/0xa75
  (XEN)    [<ffff82d040519462>] F arch_do_multicall_call+0xec/0xf1
  (XEN)    [<ffff82d040261567>] F do_multicall+0x1dc/0xde3
  (XEN)    [<ffff82d04040d2b3>] F hvm_hypercall+0xa00/0x149a
  (XEN)    [<ffff82d0403cd072>] F vmx_vmexit_handler+0x1596/0x279c
  (XEN)    [<ffff82d0403d909b>] F vmx_asm_vmexit_handler+0xdb/0x200

Left-shifting any negative value is strictly undefined behaviour in C, and
the two parameters here come straight from the guest.

The fuzzer happened to choose lo 0xf, hi 0x8000e300.

Switch everything to be unsigned values, making the shift well defined.

As GCC documents:

  As an extension to the C language, GCC does not use the latitude given in
  C99 and C11 only to treat certain aspects of signed '<<' as undefined.
  However, -fsanitize=shift (and -fsanitize=undefined) will diagnose such
  cases.

this was deemed not to need an XSA.

Fixes: 2942f45e09fb ("Enable compatibility mode operation for HYPERVISOR_sched_op and HYPERVISOR_set_timer_op.")
Reported-by: Tamas K Lengyel <tamas@tklengyel.com>
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: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
---
 xen/common/sched/compat.c    | 4 ++--
 xen/include/hypercall-defs.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/common/sched/compat.c b/xen/common/sched/compat.c
index d718e450d40b..dd97593630ee 100644
--- a/xen/common/sched/compat.c
+++ b/xen/common/sched/compat.c
@@ -43,9 +43,9 @@ static int compat_poll(struct compat_sched_poll *compat)
 
 #include "core.c"
 
-int compat_set_timer_op(uint32_t lo, int32_t hi)
+int compat_set_timer_op(uint32_t lo, uint32_t hi)
 {
-    return do_set_timer_op(((s64)hi << 32) | lo);
+    return do_set_timer_op(((uint64_t)hi << 32) | lo);
 }
 
 #endif /* __COMMON_SCHED_COMPAT_C__ */
diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
index 6d361ddfce1b..47c093acc84d 100644
--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -134,7 +134,7 @@ xenoprof_op(int op, void *arg)
 
 #ifdef CONFIG_COMPAT
 prefix: compat
-set_timer_op(uint32_t lo, int32_t hi)
+set_timer_op(uint32_t lo, uint32_t hi)
 multicall(multicall_entry_compat_t *call_list, uint32_t nr_calls)
 memory_op(unsigned int cmd, void *arg)
 #ifdef CONFIG_IOREQ_SERVER

base-commit: cc6ba68edf6dcd18c3865e7d7c0f1ed822796426
prerequisite-patch-id: de9234b4d0488be5b3be5e2ec23e85789086debc
-- 
2.30.2
Re: [PATCH] xen/sched: Fix UB shift in compat_set_timer_op()
Posted by Jan Beulich 2 months, 3 weeks ago 
On 30.01.2024 22:27, Andrew Cooper wrote:
> Tamas reported this UBSAN failure from fuzzing:
> 
>   (XEN) ================================================================================
>   (XEN) UBSAN: Undefined behaviour in common/sched/compat.c:48:37
>   (XEN) left shift of negative value -2147425536
>   (XEN) ----[ Xen-4.19-unstable  x86_64  debug=y ubsan=y  Not tainted ]----
>   ...
>   (XEN) Xen call trace:
>   (XEN)    [<ffff82d040307c1c>] R ubsan.c#ubsan_epilogue+0xa/0xd9
>   (XEN)    [<ffff82d040308afb>] F __ubsan_handle_shift_out_of_bounds+0x11a/0x1c5
>   (XEN)    [<ffff82d040307758>] F compat_set_timer_op+0x41/0x43
>   (XEN)    [<ffff82d04040e4cc>] F hvm_do_multicall_call+0x77f/0xa75
>   (XEN)    [<ffff82d040519462>] F arch_do_multicall_call+0xec/0xf1
>   (XEN)    [<ffff82d040261567>] F do_multicall+0x1dc/0xde3
>   (XEN)    [<ffff82d04040d2b3>] F hvm_hypercall+0xa00/0x149a
>   (XEN)    [<ffff82d0403cd072>] F vmx_vmexit_handler+0x1596/0x279c
>   (XEN)    [<ffff82d0403d909b>] F vmx_asm_vmexit_handler+0xdb/0x200
> 
> Left-shifting any negative value is strictly undefined behaviour in C, and
> the two parameters here come straight from the guest.
> 
> The fuzzer happened to choose lo 0xf, hi 0x8000e300.
> 
> Switch everything to be unsigned values, making the shift well defined.
> 
> As GCC documents:
> 
>   As an extension to the C language, GCC does not use the latitude given in
>   C99 and C11 only to treat certain aspects of signed '<<' as undefined.
>   However, -fsanitize=shift (and -fsanitize=undefined) will diagnose such
>   cases.
> 
> this was deemed not to need an XSA.
> 
> Fixes: 2942f45e09fb ("Enable compatibility mode operation for HYPERVISOR_sched_op and HYPERVISOR_set_timer_op.")
> Reported-by: Tamas K Lengyel <tamas@tklengyel.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with the additional observation that the subsequent unsigned->signed conversion
then exercises implementation defined behavior, i.e. is also fine given what
gcc doc states in this regard. Not sure whether that's worth mentioning, seeing
that we have such conversions all over the place (albeit I think it would be
nice if we could reduce their amount some).

Jan

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.