* [PATCH] KVM: x86: Fix potential memory access error @ 2021-03-31 9:15 Yang Li 2021-03-31 18:07 ` Sean Christopherson 0 siblings, 1 reply; 4+ messages in thread From: Yang Li @ 2021-03-31 9:15 UTC (permalink / raw) To: pbonzini Cc: seanjc, vkuznets, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, kvm, linux-kernel, Yang Li Using __set_bit() to set a bit in an integer is not a good idea, since the function expects an unsigned long as argument, which can be 64bit wide. Coverity reports this problem as High:Out-of-bounds access(INCOMPATIBLE_CAST) CWE119: Out-of-bounds access to a scalar Pointer "&vcpu->arch.regs_avail" points to an object whose effective type is "unsigned int" (32 bits, unsigned) but is dereferenced as a wider "unsigned long" (64 bits, unsigned). This may lead to memory corruption. /home/heyuan.shy/git-repo/linux/arch/x86/kvm/kvm_cache_regs.h: kvm_register_is_available Just use BIT instead. Reported-by: Abaci Robot <abaci@linux.alibaba.com> Signed-off-by: Yang Li <yang.lee@linux.alibaba.com> --- arch/x86/kvm/kvm_cache_regs.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h index 2e11da2..cfa45d88 100644 --- a/arch/x86/kvm/kvm_cache_regs.h +++ b/arch/x86/kvm/kvm_cache_regs.h @@ -52,14 +52,14 @@ static inline bool kvm_register_is_dirty(struct kvm_vcpu *vcpu, static inline void kvm_register_mark_available(struct kvm_vcpu *vcpu, enum kvm_reg reg) { - __set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail); + vcpu->arch.regs_avail |= BIT(reg); } static inline void kvm_register_mark_dirty(struct kvm_vcpu *vcpu, enum kvm_reg reg) { - __set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail); - __set_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty); + vcpu->arch.regs_avail |= BIT(reg); + vcpu->arch.regs_dirty |= BIT(reg); } static inline unsigned long kvm_register_read(struct kvm_vcpu *vcpu, int reg) -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: x86: Fix potential memory access error 2021-03-31 9:15 [PATCH] KVM: x86: Fix potential memory access error Yang Li @ 2021-03-31 18:07 ` Sean Christopherson 2021-04-01 9:08 ` Vitaly Kuznetsov 0 siblings, 1 reply; 4+ messages in thread From: Sean Christopherson @ 2021-03-31 18:07 UTC (permalink / raw) To: Yang Li Cc: pbonzini, vkuznets, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, kvm, linux-kernel On Wed, Mar 31, 2021, Yang Li wrote: > Using __set_bit() to set a bit in an integer is not a good idea, since > the function expects an unsigned long as argument, which can be 64bit wide. > Coverity reports this problem as > > High:Out-of-bounds access(INCOMPATIBLE_CAST) > CWE119: Out-of-bounds access to a scalar > Pointer "&vcpu->arch.regs_avail" points to an object whose effective > type is "unsigned int" (32 bits, unsigned) but is dereferenced as a > wider "unsigned long" (64 bits, unsigned). This may lead to memory > corruption. > > /home/heyuan.shy/git-repo/linux/arch/x86/kvm/kvm_cache_regs.h: > kvm_register_is_available > > Just use BIT instead. Meh, we're hosed either way. Using BIT() will either result in undefined behavior due to SHL shifting beyond the size of a u64, or setting random bits if the truncated shift ends up being less than 63. I suppose one could argue that undefined behavior is better than memory corruption, but KVM is very broken if 'reg' is out-of-bounds so IMO it's not worth changing. There are only two call sites that don't use a hardcoded value, and both are guarded by WARN. kvm_register_write() bails without calling kvm_register_mark_dirty(), so that's guaranteed safe. vmx_cache_reg() WARNs after kvm_register_mark_available(), but except for kvm_register_read(), all calls to vmx_cache_reg() use a hardcoded value, and kvm_register_read() also WARNs and bails. Note, all of the above holds true for kvm_register_is_{available,dirty}(), too. So in the current code, it's impossible for this to be a problem. Theoretically future code could introduce bugs, but IMO we should never accept code that uses a non-hardcoded 'reg' and doesn't pre-validate. The number of uops is basically a wash because "BTS <reg>, <mem>" is fairly expensive; depending on the uarch, the difference is ~1-2 uops in favor of BIT(). On the flip side, __set_bit() shaves 8 bytes. Of course, none these flows are anywhere near that senstive. TL;DR: I'm not opposed to using BIT(), I just don't see the point. __set_bit(): 0x00000000000104e6 <+6>: mov %esi,%eax 0x00000000000104e8 <+8>: mov %rdi,%rbp 0x00000000000104eb <+11>: sub $0x8,%rsp 0x00000000000104ef <+15>: bts %rax,0x2a0(%rdi) |= BIT(): 0x0000000000010556 <+6>: mov %esi,%ecx 0x0000000000010558 <+8>: mov $0x1,%eax 0x000000000001055d <+13>: mov %rdi,%rbp 0x0000000000010560 <+16>: sub $0x8,%rsp 0x0000000000010564 <+20>: shl %cl,%rax 0x0000000000010567 <+23>: or %eax,0x2a0(%rdi) > Reported-by: Abaci Robot <abaci@linux.alibaba.com> > Signed-off-by: Yang Li <yang.lee@linux.alibaba.com> > --- > arch/x86/kvm/kvm_cache_regs.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h > index 2e11da2..cfa45d88 100644 > --- a/arch/x86/kvm/kvm_cache_regs.h > +++ b/arch/x86/kvm/kvm_cache_regs.h > @@ -52,14 +52,14 @@ static inline bool kvm_register_is_dirty(struct kvm_vcpu *vcpu, > static inline void kvm_register_mark_available(struct kvm_vcpu *vcpu, > enum kvm_reg reg) > { > - __set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail); > + vcpu->arch.regs_avail |= BIT(reg); > } > > static inline void kvm_register_mark_dirty(struct kvm_vcpu *vcpu, > enum kvm_reg reg) > { > - __set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail); > - __set_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty); > + vcpu->arch.regs_avail |= BIT(reg); > + vcpu->arch.regs_dirty |= BIT(reg); > } > > static inline unsigned long kvm_register_read(struct kvm_vcpu *vcpu, int reg) > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: x86: Fix potential memory access error 2021-03-31 18:07 ` Sean Christopherson @ 2021-04-01 9:08 ` Vitaly Kuznetsov 2021-04-01 16:22 ` Sean Christopherson 0 siblings, 1 reply; 4+ messages in thread From: Vitaly Kuznetsov @ 2021-04-01 9:08 UTC (permalink / raw) To: Sean Christopherson, Yang Li Cc: pbonzini, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, kvm, linux-kernel Sean Christopherson <seanjc@google.com> writes: > On Wed, Mar 31, 2021, Yang Li wrote: >> Using __set_bit() to set a bit in an integer is not a good idea, since >> the function expects an unsigned long as argument, which can be 64bit wide. >> Coverity reports this problem as >> >> High:Out-of-bounds access(INCOMPATIBLE_CAST) >> CWE119: Out-of-bounds access to a scalar >> Pointer "&vcpu->arch.regs_avail" points to an object whose effective >> type is "unsigned int" (32 bits, unsigned) but is dereferenced as a >> wider "unsigned long" (64 bits, unsigned). This may lead to memory >> corruption. >> >> /home/heyuan.shy/git-repo/linux/arch/x86/kvm/kvm_cache_regs.h: >> kvm_register_is_available >> >> Just use BIT instead. > > Meh, we're hosed either way. Using BIT() will either result in undefined > behavior due to SHL shifting beyond the size of a u64, or setting random bits > if the truncated shift ends up being less than 63. > A stupid question: why can't we just make 'regs_avail'/'regs_dirty' 'unsigned long' and drop a bunch of '(unsigned long *)' casts? -- Vitaly ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: x86: Fix potential memory access error 2021-04-01 9:08 ` Vitaly Kuznetsov @ 2021-04-01 16:22 ` Sean Christopherson 0 siblings, 0 replies; 4+ messages in thread From: Sean Christopherson @ 2021-04-01 16:22 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: Yang Li, pbonzini, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, kvm, linux-kernel On Thu, Apr 01, 2021, Vitaly Kuznetsov wrote: > Sean Christopherson <seanjc@google.com> writes: > > > On Wed, Mar 31, 2021, Yang Li wrote: > >> Using __set_bit() to set a bit in an integer is not a good idea, since > >> the function expects an unsigned long as argument, which can be 64bit wide. > >> Coverity reports this problem as > >> > >> High:Out-of-bounds access(INCOMPATIBLE_CAST) > >> CWE119: Out-of-bounds access to a scalar > >> Pointer "&vcpu->arch.regs_avail" points to an object whose effective > >> type is "unsigned int" (32 bits, unsigned) but is dereferenced as a > >> wider "unsigned long" (64 bits, unsigned). This may lead to memory > >> corruption. > >> > >> /home/heyuan.shy/git-repo/linux/arch/x86/kvm/kvm_cache_regs.h: > >> kvm_register_is_available > >> > >> Just use BIT instead. > > > > Meh, we're hosed either way. Using BIT() will either result in undefined > > behavior due to SHL shifting beyond the size of a u64, or setting random bits > > if the truncated shift ends up being less than 63. > > > > A stupid question: why can't we just make 'regs_avail'/'regs_dirty' > 'unsigned long' and drop a bunch of '(unsigned long *)' casts? It wouldn't break anything, but it would create a weird situation where x86-64 has more bits for tracking registers than i386. Obviously not the end of the world, but it's also not clearly an improvement across the board. We could do something like: DECLARE_BITMAP(regs_avail, NR_VCPU_TRACKED_REGS); DECLARE_BITMAP(regs_dirty, NR_VCPU_TRACKED_REGS); but that would complicate the vendor code, e.g. vmx_register_cache_reset(). The casting crud is quite contained, and likely isn't going to expand anytime soon. So, at least for me, this is one of the few cases where I'm content to let sleeping dogs lie. :-) ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-01 18:36 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-31 9:15 [PATCH] KVM: x86: Fix potential memory access error Yang Li 2021-03-31 18:07 ` Sean Christopherson 2021-04-01 9:08 ` Vitaly Kuznetsov 2021-04-01 16:22 ` Sean Christopherson
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.