Re: [PATCH v2 2/5] KVM: x86: Emulator performs code segment checks on read access

From: "Radim Krčmář" <rkrcmar@redhat.com>
To: Nadav Amit <namit@cs.technion.ac.il>
Cc: pbonzini@redhat.com, kvm@vger.kernel.org
Subject: Re: [PATCH v2 2/5] KVM: x86: Emulator performs code segment checks on read access
Date: Fri, 10 Oct 2014 17:54:56 +0200	[thread overview]
Message-ID: <20141010155455.GA17902@potion.brq.redhat.com> (raw)
In-Reply-To: <1412906870-4322-1-git-send-email-namit@cs.technion.ac.il>

2014-10-10 05:07+0300, Nadav Amit:
> When read access is performed using a readable code segment, the "conforming"
> and "non-conforming" checks should not be done.  As a result, read using
> non-conforming readable code segment fails.
> 
> This is according to Intel SDM 5.6.1 ("Accessing Data in Code Segments").
> 
> One exception is the case of conforming code segment. The SDM says: "Use a
> code-segment override prefix (CS) to read a readable...  [it is] valid because
> the DPL of the code segment selected by the CS register is the same as the
> CPL." This is misleading since CS.DPL may be lower (numerically) than CPL, and
> CS would still be accessible.  The emulator should avoid privilage level checks
> for data reads using CS.

Ah, after stripping faulty presumptions, I'm not sure this change is
enough ... shouldn't we also skip the check on conforming code segments?

 Method 2 is always valid because the privilege level of a conforming
 code segment is effectively the same as the CPL, regardless of its DPL.

And we couldn't read it from less privileged modes.

> The fix is not to perform the "non-conforming" checks if the access is not a
> fetch, and never to perform the checks for CS.
> 
> ---
> v1->v2: Privilage level checks are always skipped for CS
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index a46207a..0fee0a0 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -661,9 +661,9 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
>  				goto bad;
>  		}
>  		cpl = ctxt->ops->cpl(ctxt);
> -		if (!(desc.type & 8)) {
> -			/* data segment */
> -			if (cpl > desc.dpl)
> +		if (!fetch) {
> +			/* data segment or readable code segment */
> +			if (cpl > desc.dpl && addr.seg != VCPU_SREG_CS)
>  				goto bad;
>  		} else if ((desc.type & 8) && !(desc.type & 4)) {
>  			/* nonconforming code segment */
> -- 
> 1.9.1
> 