diff mbox

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

Message ID 1412906870-4322-1-git-send-email-namit@cs.technion.ac.il (mailing list archive)
State New, archived
Headers show

Commit Message

Nadav Amit Oct. 10, 2014, 2:07 a.m. UTC
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.

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(-)

Comments

Radim Krčmář Oct. 10, 2014, 3:54 p.m. UTC | #1
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
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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 */