]> xenbits.xensource.com Git - people/andrewcoop/xen.git/commit
xen/sched: Fix UB shift in compat_set_timer_op()
authorAndrew Cooper <andrew.cooper3@citrix.com>
Tue, 30 Jan 2024 20:44:34 +0000 (20:44 +0000)
committerAndrew Cooper <andrew.cooper3@citrix.com>
Thu, 1 Feb 2024 19:52:44 +0000 (19:52 +0000)
commitae6d4fd876765e6d623eec67d14f5d0464be09cb
tree8b23926ade6824e8fc12460faa0f504c21f04610
parent9d701bc9abaf1e9ab458545a8f320d36dc54d686
xen/sched: Fix UB shift in compat_set_timer_op()

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.

Note: The unsigned -> signed conversion for do_set_timer_op()'s s_time_t
parameter is also well defined.  C makes it implementation defined, and GCC
defines it as reduction modulo 2^N to be within range of the new type.

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>
xen/common/sched/compat.c
xen/include/hypercall-defs.c