Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 1 Apr 2021 16:22:59 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Vitaly Kuznetsov <vkuznets@redhat.com>
Cc:     Yang Li <yang.lee@linux.alibaba.com>, pbonzini@redhat.com,
        wanpengli@tencent.com, jmattson@google.com, joro@8bytes.org,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, x86@kernel.org,
        hpa@zytor.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: x86: Fix potential memory access error
Message-ID: <YGXzY5h1eCQj6aU0@google.com>
References: <1617182122-112315-1-git-send-email-yang.lee@linux.alibaba.com>
 <YGS6XS87HYJdVPFQ@google.com>
 <87mtuis77m.fsf@vitty.brq.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87mtuis77m.fsf@vitty.brq.redhat.com>
Precedence: bulk

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