* [PATCH] KVM: x86: fix undefined behavior in bit shift for __feature_bit
@ 2022-10-31 11:36 Gaosheng Cui
2022-10-31 17:42 ` Sean Christopherson
0 siblings, 1 reply; 7+ messages in thread
From: Gaosheng Cui @ 2022-10-31 11:36 UTC (permalink / raw)
To: seanjc, pbonzini, tglx, mingo, bp, dave.hansen, x86, hpa, cuigaosheng1
Cc: kvm
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86: fix undefined behavior in bit shift for __feature_bit
2022-10-31 11:36 [PATCH] KVM: x86: fix undefined behavior in bit shift for __feature_bit Gaosheng Cui
@ 2022-10-31 17:42 ` Sean Christopherson
2022-10-31 20:27 ` H. Peter Anvin
2022-10-31 20:54 ` Peter Zijlstra
0 siblings, 2 replies; 7+ messages in thread
From: Sean Christopherson @ 2022-10-31 17:42 UTC (permalink / raw)
To: Gaosheng Cui; +Cc: pbonzini, tglx, mingo, bp, dave.hansen, x86, hpa, kvm
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86: fix undefined behavior in bit shift for __feature_bit
2022-10-31 17:42 ` Sean Christopherson
@ 2022-10-31 20:27 ` H. Peter Anvin
2022-11-01 2:37 ` cuigaosheng
2022-10-31 20:54 ` Peter Zijlstra
1 sibling, 1 reply; 7+ messages in thread
From: H. Peter Anvin @ 2022-10-31 20:27 UTC (permalink / raw)
To: Sean Christopherson, Gaosheng Cui
Cc: pbonzini, tglx, mingo, bp, dave.hansen, x86, kvm
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86: fix undefined behavior in bit shift for __feature_bit
2022-10-31 17:42 ` Sean Christopherson
2022-10-31 20:27 ` H. Peter Anvin
@ 2022-10-31 20:54 ` Peter Zijlstra
2022-11-02 17:54 ` Paolo Bonzini
1 sibling, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2022-10-31 20:54 UTC (permalink / raw)
To: Sean Christopherson
Cc: Gaosheng Cui, pbonzini, tglx, mingo, bp, dave.hansen, x86, hpa, kvm
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86: fix undefined behavior in bit shift for __feature_bit
2022-10-31 20:27 ` H. Peter Anvin
@ 2022-11-01 2:37 ` cuigaosheng
0 siblings, 0 replies; 7+ messages in thread
From: cuigaosheng @ 2022-11-01 2:37 UTC (permalink / raw)
To: H. Peter Anvin, Sean Christopherson, peterz
Cc: pbonzini, tglx, mingo, bp, dave.hansen, x86, kvm
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.
> .
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86: fix undefined behavior in bit shift for __feature_bit
2022-10-31 20:54 ` Peter Zijlstra
@ 2022-11-02 17:54 ` Paolo Bonzini
2022-11-02 20:56 ` Peter Zijlstra
0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2022-11-02 17:54 UTC (permalink / raw)
To: Peter Zijlstra, Sean Christopherson
Cc: Gaosheng Cui, tglx, mingo, bp, dave.hansen, x86, hpa, kvm
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: x86: fix undefined behavior in bit shift for __feature_bit
2022-11-02 17:54 ` Paolo Bonzini
@ 2022-11-02 20:56 ` Peter Zijlstra
0 siblings, 0 replies; 7+ messages in thread
From: Peter Zijlstra @ 2022-11-02 20:56 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Gaosheng Cui, tglx, mingo, bp, dave.hansen,
x86, hpa, kvm
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-02 20:56 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-31 11:36 [PATCH] KVM: x86: fix undefined behavior in bit shift for __feature_bit Gaosheng Cui
2022-10-31 17:42 ` Sean Christopherson
2022-10-31 20:27 ` H. Peter Anvin
2022-11-01 2:37 ` cuigaosheng
2022-10-31 20:54 ` Peter Zijlstra
2022-11-02 17:54 ` Paolo Bonzini
2022-11-02 20:56 ` Peter Zijlstra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.