linux-kernel - Re: [Resend Patch] KVM/x86: Fix wrong macro references of X86_CR0_PG_BIT and X86_CR4_PAE_BIT in kvm_valid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180119024104.2q5asztbbp4yd2ca@ubuntu.agxa3e3klouercz13e2hi0kn4g.lx.internal.cloudapp.net>
Date:   Fri, 19 Jan 2018 02:41:04 +0000
From:   Lan Tianyu <lantianyu1986@...il.com>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     Tianyu Lan <Tianyu.Lan@...rosoft.com>, pbonzini@...hat.com,
        rkrcmar@...hat.com, tglx@...utronix.de, mingo@...hat.com,
        hpa@...or.com, x86@...nel.org, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, jeremi.piotrowski@...il.com
Subject: Re: [Resend Patch] KVM/x86: Fix wrong macro references of
 X86_CR0_PG_BIT and X86_CR4_PAE_BIT in kvm_valid_sregs()

Hi Eric:
	Great thanks for your review.
On Thu, Jan 18, 2018 at 10:39:04AM -0800, Eric Biggers wrote:
> On Tue, Jan 16, 2018 at 05:34:07PM +0800, Tianyu Lan wrote:
> > kvm_valid_sregs() should use X86_CR0_PG and X86_CR4_PAE to check bit
> > status rather than X86_CR0_PG_BIT and X86_CR4_PAE_BIT. This patch is
> > to fix it.
> > 
> > Fixes: f29810335965a(KVM/x86: Check input paging mode when cs.l is set)
> > Reported-by: Jeremi Piotrowski <jeremi.piotrowski@...il.com>
> > Cc: Paolo Bonzini <pbonzini@...hat.com>
> > Cc: Radim Krčmář <rkrcmar@...hat.com>
> > Signed-off-by: Tianyu Lan <Tianyu.Lan@...rosoft.com>
> > ---
> > Sorry for noise. Missed kvm maillist.
> > 
> >  arch/x86/kvm/x86.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 1cec2c6..c53298d 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -7496,13 +7496,13 @@ EXPORT_SYMBOL_GPL(kvm_task_switch);
> >  
> >  int kvm_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
> >  {
> > -	if ((sregs->efer & EFER_LME) && (sregs->cr0 & X86_CR0_PG_BIT)) {
> > +	if ((sregs->efer & EFER_LME) && (sregs->cr0 & X86_CR0_PG)) {
> >  		/*
> >  		 * When EFER.LME and CR0.PG are set, the processor is in
> >  		 * 64-bit mode (though maybe in a 32-bit code segment).
> >  		 * CR4.PAE and EFER.LMA must be set.
> >  		 */
> > -		if (!(sregs->cr4 & X86_CR4_PAE_BIT)
> > +		if (!(sregs->cr4 & X86_CR4_PAE)
> >  		    || !(sregs->efer & EFER_LMA))
> >  			return -EINVAL;
> >  	} else {
> > -- 
> > 2.7.4
> > 
> 
> I came across this too and was just about to send the exact same patch.  It
> looks good to me as long as the bits it's supposed to be checking were correct
> in the first place.  Patch title could maybe be shortened a bit, e.g. "KVM/x86:
> Fix references to CR0.PG and CR4.PAE in kvm_valid_sregs()".  The "Fixes:" line
> is also formatted incorrectly.

That will be better and will update.

> 
> Thanks,
> 
> Eric