| Message ID | 20220621150902.46126-6-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMM emulation and interrupt shadow fixes | expand |
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. 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 >
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 > >
On Thu, Jul 21, 2022, Maxim Levitsky wrote: > 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. Toggling CR0.PG when EFER.LME=1 (and CR4.PAE=1) switches the CPU in and out of long mode. That's why I mentioned the EFER.LMA thing below. It's also notable in that the only reason we don't have to handle CR4 here is because clearing CR4.PAE while long is active causes a #GP. > 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.
On Thu, 2022-07-21 at 14:11 +0000, Sean Christopherson wrote: > On Thu, Jul 21, 2022, Maxim Levitsky wrote: > > 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. > > Toggling CR0.PG when EFER.LME=1 (and CR4.PAE=1) switches the CPU in and out of > long mode. That's why I mentioned the EFER.LMA thing below. It's also notable > in that the only reason we don't have to handle CR4 here is because clearing > CR4.PAE while long is active causes a #GP. I had a distinct feeling that this is related to LMA/LME which I always learn and then forget Now I do, and I wrote a short summary for myself to refresh my memory when I forget about this again :-) I'll update the comment again in v3. Thanks a lot, Best regards, Maxim Levitsky > > > 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.
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 */ + r = update_emulation_mode(ctxt); + if (r != X86EMUL_CONTINUE) + return r; + } + return X86EMUL_CONTINUE; }
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(-)