| Message ID | 1617182122-112315-1-git-send-email-yang.lee@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Fix potential memory access error | expand |
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 >
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?
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. :-)
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)
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(-)