From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 2/6] KVM: x86: Wrong error code on limit violation during emulation Date: Mon, 27 Oct 2014 15:37:11 +0100 Message-ID: <544E5897.4080507@redhat.com> References: <1412099359-5316-1-git-send-email-namit@cs.technion.ac.il> <1412099359-5316-3-git-send-email-namit@cs.technion.ac.il> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, nadav.amit@gmail.com To: Nadav Amit Return-path: Received: from mail-la0-f43.google.com ([209.85.215.43]:60302 "EHLO mail-la0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751554AbaJ0OhR (ORCPT ); Mon, 27 Oct 2014 10:37:17 -0400 Received: by mail-la0-f43.google.com with SMTP id ge10so1883834lab.30 for ; Mon, 27 Oct 2014 07:37:15 -0700 (PDT) In-Reply-To: <1412099359-5316-3-git-send-email-namit@cs.technion.ac.il> Sender: kvm-owner@vger.kernel.org List-ID: On 09/30/2014 07:49 PM, Nadav Amit wrote: > GP and SS exceptions deliver as error-code the segment selector if the > exception occurred when the segment is loaded. However, if the exception > occurs during the memory access itself, due to limit violations, the error-code > should be zero. Fix it. > > Signed-off-by: Nadav Amit > --- > arch/x86/kvm/emulate.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index a46207a..13a1c76 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -621,7 +621,7 @@ static int __linearize(struct x86_emulate_ctxt *ctxt, > bool usable; > ulong la; > u32 lim; > - u16 sel; > + u16 sel, error_code = 0; > unsigned cpl; > > la = seg_base(ctxt, addr.seg) + addr.ea; > @@ -634,14 +634,14 @@ static int __linearize(struct x86_emulate_ctxt *ctxt, > usable = ctxt->ops->get_segment(ctxt, &sel, &desc, NULL, > addr.seg); > if (!usable) > - goto bad; > + goto bad_sel; This can only happen because of a NULL selector, which means the error code is zero anyway. > /* code segment in protected mode or read-only data segment */ > if ((((ctxt->mode != X86EMUL_MODE_REAL) && (desc.type & 8)) > || !(desc.type & 2)) && write) > - goto bad; > + goto bad_sel; This is not "detected while loading a segment descriptor", so the error code should be zero. > /* unreadable code segment */ > if (!fetch && (desc.type & 8) && !(desc.type & 2)) > - goto bad; > + goto bad_sel; Same here. > lim = desc_limit_scaled(&desc); > if ((ctxt->mode == X86EMUL_MODE_REAL) && !fetch && > (ctxt->d & NoBigReal)) { > @@ -664,15 +664,15 @@ static int __linearize(struct x86_emulate_ctxt *ctxt, > if (!(desc.type & 8)) { > /* data segment */ > if (cpl > desc.dpl) > - goto bad; > + goto bad_sel; > } else if ((desc.type & 8) && !(desc.type & 4)) { > /* nonconforming code segment */ > if (cpl != desc.dpl) > - goto bad; > + goto bad_sel; > } else if ((desc.type & 8) && (desc.type & 4)) { > /* conforming code segment */ > if (cpl < desc.dpl) > - goto bad; > + goto bad_sel; These three should be deleted, as you pointed out in patch 5. So I've dropped this patch, and posted a simpler alternative that just uses error code 0 in __linearize. Can you look at it? Paolo > } > break; > } > @@ -682,11 +682,13 @@ static int __linearize(struct x86_emulate_ctxt *ctxt, > return emulate_gp(ctxt, 0); > *linear = la; > return X86EMUL_CONTINUE; > +bad_sel: > + error_code = sel; > bad: > if (addr.seg == VCPU_SREG_SS) > - return emulate_ss(ctxt, sel); > + return emulate_ss(ctxt, error_code); > else > - return emulate_gp(ctxt, sel); > + return emulate_gp(ctxt, error_code); > } > > static int linearize(struct x86_emulate_ctxt *ctxt, >