[PATCH v2] KVM: x86: fix undefined behavior in bit shift for __feature_bit

[PATCH v2] KVM: x86: fix undefined behavior in bit shift for __feature_bit

[Gaosheng Cui]

Shifting signed 32-bit value by 31 bits is undefined, so we fix it
with the BIT() macro, at the same time, we change the input to
unsigned, and replace "/ 32" with ">> 5".

The UBSAN warning calltrace like below:

UBSAN: shift-out-of-bounds in arch/x86/kvm/reverse_cpuid.h:101:11
left shift of 1 by 31 places cannot be represented in type 'int'
Call Trace:
 <TASK>
 dump_stack_lvl+0x7d/0xa5
 dump_stack+0x15/0x1b
 ubsan_epilogue+0xe/0x4e
 __ubsan_handle_shift_out_of_bounds+0x1e7/0x20c
 kvm_set_cpu_caps+0x15a/0x770 [kvm]
 hardware_setup+0xa6f/0xdfe [kvm_intel]
 kvm_arch_hardware_setup+0x100/0x1e80 [kvm]
 kvm_init+0xdb/0x560 [kvm]
 vmx_init+0x161/0x2b4 [kvm_intel]
 do_one_initcall+0x76/0x430
 do_init_module+0x61/0x28a
 load_module+0x1f82/0x2e50
 __do_sys_finit_module+0xf8/0x190
 __x64_sys_finit_module+0x23/0x30
 do_syscall_64+0x58/0x80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
 </TASK>

Fixes: a7c48c3f56db ("KVM: x86: Expand build-time assertion on reverse CPUID usage")
Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
---
v2:
- Fix the issue by the BIT() macro, change the input to unsigned and
- replace "/ 32" with ">> 5", update the commit msg as well, Thanks!
 arch/x86/kvm/reverse_cpuid.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
index a19d473d0184..f1680344de56 100644
--- a/arch/x86/kvm/reverse_cpuid.h
+++ b/arch/x86/kvm/reverse_cpuid.h
@@ -93,12 +93,12 @@ static __always_inline u32 __feature_leaf(int x86_feature)
  * "word" (stored in bits 31:5).  The word is used to index into arrays of
  * bit masks that hold the per-cpu feature capabilities, e.g. this_cpu_has().
  */
-static __always_inline u32 __feature_bit(int x86_feature)
+static __always_inline u32 __feature_bit(unsigned int x86_feature)
 {
 	x86_feature = __feature_translate(x86_feature);
 
-	reverse_cpuid_check(x86_feature / 32);
-	return 1 << (x86_feature & 31);
+	reverse_cpuid_check(x86_feature >> 5);
+	return BIT(x86_feature & 31);
 }
 
 #define feature_bit(name)  __feature_bit(X86_FEATURE_##name)
-- 
2.25.1

Re: [PATCH v2] KVM: x86: fix undefined behavior in bit shift for __feature_bit

[Peter Zijlstra]

On Tue, Nov 01, 2022 at 10:28:28AM +0800, Gaosheng Cui wrote:
> Shifting signed 32-bit value by 31 bits is undefined, so we fix it
> with the BIT() macro, at the same time, we change the input to
> unsigned, and replace "/ 32" with ">> 5".
> 
> The UBSAN warning calltrace like below:
> 
> UBSAN: shift-out-of-bounds in arch/x86/kvm/reverse_cpuid.h:101:11
> left shift of 1 by 31 places cannot be represented in type 'int'

Again; please go fix your toolchain and don't quote broken crap like
this to change the code.

I'm fine with the changes, but there is no UB here, don't pretend there
is.

Re: [PATCH v2] KVM: x86: fix undefined behavior in bit shift for __feature_bit

[cuigaosheng]

> Again; please go fix your toolchain and don't quote broken crap like
> this to change the code.
>
> I'm fine with the changes, but there is no UB here, don't pretend there
> is.

I have made patch v3 and submitted it, removed the UBSAN warning calltrace,
thanks for taking time review patch!

On 2022/11/1 16:44, Peter Zijlstra wrote:
> On Tue, Nov 01, 2022 at 10:28:28AM +0800, Gaosheng Cui wrote:
>> Shifting signed 32-bit value by 31 bits is undefined, so we fix it
>> with the BIT() macro, at the same time, we change the input to
>> unsigned, and replace "/ 32" with ">> 5".
>>
>> The UBSAN warning calltrace like below:
>>
>> UBSAN: shift-out-of-bounds in arch/x86/kvm/reverse_cpuid.h:101:11
>> left shift of 1 by 31 places cannot be represented in type 'int'
> Again; please go fix your toolchain and don't quote broken crap like
> this to change the code.
>
> I'm fine with the changes, but there is no UB here, don't pretend there
> is.
>
> .