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

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

[Gaosheng Cui]

Shifting signed 32-bit value by 31 bits is undefined, so changing
significant bit to unsigned. 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>
---
 arch/x86/kvm/reverse_cpuid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
index a19d473d0184..ebd6b621d3b8 100644
--- a/arch/x86/kvm/reverse_cpuid.h
+++ b/arch/x86/kvm/reverse_cpuid.h
@@ -98,7 +98,7 @@ static __always_inline u32 __feature_bit(int x86_feature)
 	x86_feature = __feature_translate(x86_feature);
 
 	reverse_cpuid_check(x86_feature / 32);
-	return 1 << (x86_feature & 31);
+	return 1U << (x86_feature & 31);
 }
 
 #define feature_bit(name)  __feature_bit(X86_FEATURE_##name)
-- 
2.25.1

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

[Sean Christopherson]

On Mon, Oct 31, 2022, Gaosheng Cui wrote:
> Shifting signed 32-bit value by 31 bits is undefined, so changing
> significant bit to unsigned. 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'

PeterZ is contending that this isn't actually undefined behavior given how the
kernel is compiled[*].  That said, I would be in favor of replacing the open-coded
shift with BIT() to make the code a bit more self-documenting, and that would
naturally fix this maybe-undefined-behavior issue. 

[*] https://lore.kernel.org/all/Y1%2FAaJOcgIc%2FINtv@hirez.programming.kicks-ass.net

> ---
>  arch/x86/kvm/reverse_cpuid.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
> index a19d473d0184..ebd6b621d3b8 100644
> --- a/arch/x86/kvm/reverse_cpuid.h
> +++ b/arch/x86/kvm/reverse_cpuid.h
> @@ -98,7 +98,7 @@ static __always_inline u32 __feature_bit(int x86_feature)
>  	x86_feature = __feature_translate(x86_feature);
>  
>  	reverse_cpuid_check(x86_feature / 32);
> -	return 1 << (x86_feature & 31);
> +	return 1U << (x86_feature & 31);
>  }
>  
>  #define feature_bit(name)  __feature_bit(X86_FEATURE_##name)
> -- 
> 2.25.1
> 

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

[H. Peter Anvin]

On October 31, 2022 10:42:56 AM PDT, Sean Christopherson <seanjc@google.com> wrote:
>On Mon, Oct 31, 2022, Gaosheng Cui wrote:
>> Shifting signed 32-bit value by 31 bits is undefined, so changing
>> significant bit to unsigned. 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'
>
>PeterZ is contending that this isn't actually undefined behavior given how the
>kernel is compiled[*].  That said, I would be in favor of replacing the open-coded
>shift with BIT() to make the code a bit more self-documenting, and that would
>naturally fix this maybe-undefined-behavior issue. 
>
>[*] https://lore.kernel.org/all/Y1%2FAaJOcgIc%2FINtv@hirez.programming.kicks-ass.net
>
>> ---
>>  arch/x86/kvm/reverse_cpuid.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
>> index a19d473d0184..ebd6b621d3b8 100644
>> --- a/arch/x86/kvm/reverse_cpuid.h
>> +++ b/arch/x86/kvm/reverse_cpuid.h
>> @@ -98,7 +98,7 @@ static __always_inline u32 __feature_bit(int x86_feature)
>>  	x86_feature = __feature_translate(x86_feature);
>>  
>>  	reverse_cpuid_check(x86_feature / 32);
>> -	return 1 << (x86_feature & 31);
>> +	return 1U << (x86_feature & 31);
>>  }
>>  
>>  #define feature_bit(name)  __feature_bit(X86_FEATURE_##name)
>> -- 
>> 2.25.1
>> 

One really ought to change the input to unsigned, though, and I would argue >> 5 would be more idiomatic than / 32; / goes with % whereas >> goes with &; a mishmash is just ugly AF.

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

[Peter Zijlstra]

On Mon, Oct 31, 2022 at 05:42:56PM +0000, Sean Christopherson wrote:
> On Mon, Oct 31, 2022, Gaosheng Cui wrote:
> > Shifting signed 32-bit value by 31 bits is undefined, so changing
> > significant bit to unsigned. 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'
> 
> PeterZ is contending that this isn't actually undefined behavior given how the
> kernel is compiled[*].  That said, I would be in favor of replacing the open-coded
> shift with BIT() to make the code a bit more self-documenting, and that would
> naturally fix this maybe-undefined-behavior issue. 
> 
> [*] https://lore.kernel.org/all/Y1%2FAaJOcgIc%2FINtv@hirez.programming.kicks-ass.net

I'm definitely in favour of updating this code; both your suggestion and
hpa's suggestion look like sane changes. But I do feel that whatever
UBSAN thing generated this warning needs to be fixed too.

I'm fine with the compiler warning about this code -- but it must not
claim undefined behaviour given the compiler flags we use.

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

[cuigaosheng]

Thanks for taking time to review the patch!

> PeterZ is contending that this isn't actually undefined behavior given how the
> kernel is compiled[*].  That said, I would be in favor of replacing the open-coded
> shift with BIT() to make the code a bit more self-documenting, and that would
> naturally fix this maybe-undefined-behavior issue.
>
> [*]https://lore.kernel.org/all/Y1%2FAaJOcgIc%2FINtv@hirez.programming.kicks-ass.net

I have made a patch v2 and submitted it, replacing the open-coded shift with BIT().

> One really ought to change the input to unsigned, though, and I would argue >> 5 would be more idiomatic than / 32; / goes with % whereas >> goes with &; a mishmash is just ugly AF.
> .

I have changed the input to unsigned in patch v2, and replaced "/ 32" with "argue >> 5".

On 2022/11/1 4:27, H. Peter Anvin wrote:
> On October 31, 2022 10:42:56 AM PDT, Sean Christopherson <seanjc@google.com> wrote:
>> On Mon, Oct 31, 2022, Gaosheng Cui wrote:
>>> Shifting signed 32-bit value by 31 bits is undefined, so changing
>>> significant bit to unsigned. 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'
>> PeterZ is contending that this isn't actually undefined behavior given how the
>> kernel is compiled[*].  That said, I would be in favor of replacing the open-coded
>> shift with BIT() to make the code a bit more self-documenting, and that would
>> naturally fix this maybe-undefined-behavior issue.
>>
>> [*] https://lore.kernel.org/all/Y1%2FAaJOcgIc%2FINtv@hirez.programming.kicks-ass.net
>>
>>> ---
>>>   arch/x86/kvm/reverse_cpuid.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h
>>> index a19d473d0184..ebd6b621d3b8 100644
>>> --- a/arch/x86/kvm/reverse_cpuid.h
>>> +++ b/arch/x86/kvm/reverse_cpuid.h
>>> @@ -98,7 +98,7 @@ static __always_inline u32 __feature_bit(int x86_feature)
>>>   	x86_feature = __feature_translate(x86_feature);
>>>   
>>>   	reverse_cpuid_check(x86_feature / 32);
>>> -	return 1 << (x86_feature & 31);
>>> +	return 1U << (x86_feature & 31);
>>>   }
>>>   
>>>   #define feature_bit(name)  __feature_bit(X86_FEATURE_##name)
>>> -- 
>>> 2.25.1
>>>
> One really ought to change the input to unsigned, though, and I would argue >> 5 would be more idiomatic than / 32; / goes with % whereas >> goes with &; a mishmash is just ugly AF.
> .

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

[Paolo Bonzini]

On 10/31/22 21:54, Peter Zijlstra wrote:
>> PeterZ is contending that this isn't actually undefined behavior given how the
>> kernel is compiled[*].  That said, I would be in favor of replacing the open-coded
>> shift with BIT() to make the code a bit more self-documenting, and that would
>> naturally fix this maybe-undefined-behavior issue.
>>
>> [*]https://lore.kernel.org/all/Y1%2FAaJOcgIc%2FINtv@hirez.programming.kicks-ass.net
> I'm definitely in favour of updating this code; both your suggestion and
> hpa's suggestion look like sane changes. But I do feel that whatever
> UBSAN thing generated this warning needs to be fixed too.
> 
> I'm fine with the compiler warning about this code -- but it must not
> claim undefined behaviour given the compiler flags we use.

Yes, the compiler is buggy here (see old bug report for GCC, now fixed, 
at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68418).

I cannot even reproduce the problem with the simple userspace testcase

#include <stdlib.h>
int main(int argc) {
	int i = argc << 31;
	exit(i < 0);
}

on either GCC 12 or clang 15.

Paolo

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

[Peter Zijlstra]

On Wed, Nov 02, 2022 at 06:54:22PM +0100, Paolo Bonzini wrote:
> On 10/31/22 21:54, Peter Zijlstra wrote:
> > > PeterZ is contending that this isn't actually undefined behavior given how the
> > > kernel is compiled[*].  That said, I would be in favor of replacing the open-coded
> > > shift with BIT() to make the code a bit more self-documenting, and that would
> > > naturally fix this maybe-undefined-behavior issue.
> > > 
> > > [*]https://lore.kernel.org/all/Y1%2FAaJOcgIc%2FINtv@hirez.programming.kicks-ass.net
> > I'm definitely in favour of updating this code; both your suggestion and
> > hpa's suggestion look like sane changes. But I do feel that whatever
> > UBSAN thing generated this warning needs to be fixed too.
> > 
> > I'm fine with the compiler warning about this code -- but it must not
> > claim undefined behaviour given the compiler flags we use.
> 
> Yes, the compiler is buggy here (see old bug report for GCC, now fixed, at
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68418).
> 
> I cannot even reproduce the problem with the simple userspace testcase
> 
> #include <stdlib.h>
> int main(int argc) {
> 	int i = argc << 31;
> 	exit(i < 0);
> }
> 
> on either GCC 12 or clang 15.

Perhaps we should have the UBSAN splat include the compiler-version
used... because clearly someone is using ancient crap here.