Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <532c71cbca049004bd6860508fdc056ae118ab1f.camel@redhat.com>
Subject: Re: [PATCH v2 05/11] KVM: x86: emulator: update the emulation mode
 after CR0 write
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, x86@kernel.org,
        Kees Cook <keescook@chromium.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        linux-kernel@vger.kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
        Borislav Petkov <bp@alien8.de>, Joerg Roedel <joro@8bytes.org>,
        Ingo Molnar <mingo@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>
Date:   Thu, 21 Jul 2022 14:53:43 +0300
In-Reply-To: <YtiUq7jm2Z1NTRv3@google.com>
References: <20220621150902.46126-1-mlevitsk@redhat.com>
         <20220621150902.46126-6-mlevitsk@redhat.com> <YtiUq7jm2Z1NTRv3@google.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2022-07-20 at 23:50 +0000, Sean Christopherson wrote:
> On Tue, Jun 21, 2022, Maxim Levitsky wrote:
> > CR0.PE toggles real/protected mode, thus its update
> > should update the emulation mode.
> > 
> > This is likely a benign bug because there is no writeback
> > of state, other than the RIP increment, and when toggling
> > CR0.PE, the CPU has to execute code from a very low memory address.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  arch/x86/kvm/emulate.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > index 6f4632babc4cd8..002687d17f9364 100644
> > --- a/arch/x86/kvm/emulate.c
> > +++ b/arch/x86/kvm/emulate.c
> > @@ -3659,11 +3659,22 @@ static int em_movbe(struct x86_emulate_ctxt *ctxt)
> >  
> >  static int em_cr_write(struct x86_emulate_ctxt *ctxt)
> >  {
> > -	if (ctxt->ops->set_cr(ctxt, ctxt->modrm_reg, ctxt->src.val))
> > +	int cr_num = ctxt->modrm_reg;
> > +	int r;
> > +
> > +	if (ctxt->ops->set_cr(ctxt, cr_num, ctxt->src.val))
> >  		return emulate_gp(ctxt, 0);
> >  
> >  	/* Disable writeback. */
> >  	ctxt->dst.type = OP_NONE;
> > +
> > +	if (cr_num == 0) {
> > +		/* CR0 write might have updated CR0.PE */
> 
> Or toggled CR0.PG.  

I thought about it but paging actually does not affect the CPU mode.

E.g if you are in protected mode, instructions execute the same regardless
if you have paging or not.

(There are probably some exceptions but you understand what I mean).

Best regards,
	Maxim Levitsky

> It's probably also worth noting that ->set_cr() handles side
> effects to other registers, e.g. the lack of an EFER.LMA update makes this look
> suspicious at first glance.

> 
> > +		r = update_emulation_mode(ctxt);
> > +		if (r != X86EMUL_CONTINUE)
> > +			return r;
> > +	}
> > +
> >  	return X86EMUL_CONTINUE;
> >  }
> >  
> > -- 
> > 2.26.3
> >