| Message ID | 20221101022828.565075-1-cuigaosheng1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: x86: fix undefined behavior in bit shift for __feature_bit | expand |
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.
> 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. > > .
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)
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(-)