[PATCH] KVM: x86: Fix potential memory access error

[PATCH] KVM: x86: Fix potential memory access error

[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

Re: [PATCH] KVM: x86: Fix potential memory access error

[Sean Christopherson]

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
> 

Re: [PATCH] KVM: x86: Fix potential memory access error

[Vitaly Kuznetsov]

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

Re: [PATCH] KVM: x86: Fix potential memory access error

[Sean Christopherson]

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. :-)