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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 10/10/2014 17:54, Radim Krčmář ha scritto:
>> > 
>> > 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.

Radim is right; we need to skip the check on conforming code segments 
and, once we do that, checking addr.seg is not necessary anymore.  That 
is because, for a CS override on a nonconforming code segment, at the 
time we fetch the instruction we know that cpl == desc.dpl.  The less 
restrictive data segment check (cpl <= desc.dpl) thus always passes.

Let's put together this check and the readability check, too, since
we are adding another "if (fetch)".

Can you guys think of a way to simplify the following untested patch?

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 03954f7900f5..9f3e33551db9 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -638,9 +638,6 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
 		if ((((ctxt->mode != X86EMUL_MODE_REAL) && (desc.type & 8))
 					|| !(desc.type & 2)) && write)
 			goto bad;
-		/* unreadable code segment */
-		if (!fetch && (desc.type & 8) && !(desc.type & 2))
-			goto bad;
 		lim = desc_limit_scaled(&desc);
 		if ((ctxt->mode == X86EMUL_MODE_REAL) && !fetch &&
 		    (ctxt->d & NoBigReal)) {
@@ -660,17 +657,40 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
 				goto bad;
 		}
 		cpl = ctxt->ops->cpl(ctxt);
-		if (!(desc.type & 8)) {
-			/* data segment */
+		if (fetch && (desc.type & 8)) {
+			if (!(desc.type & 4)) {
+				/* nonconforming code segment */
+				if (cpl != desc.dpl)
+					goto bad;
+				break;
+			} else {
+				/* conforming code segment */
+				if (cpl < desc.dpl)
+					goto bad;
+				break;
+			}
+		}
+
+		if (likely(!(desc.type & 8) || (desc.type & 6) == 2)) {
+			/*
+			 * Data segment or readable, nonconforming code
+			 * segment.  The SDM mentions that access through
+			 * a code-segment override prefix is always valid.
+			 * This really only matters for conforming code
+			 * segments (checked below, and always valid anyway):
+			 * for nonconforming ones, cpl == desc.dpl was checked
+			 * when fetching the instruction, meaning the following
+			 * test will always pass too.
+			 */
 			if (cpl > desc.dpl)
 				goto bad;
-		} else if ((desc.type & 8) && !(desc.type & 4)) {
-			/* nonconforming code segment */
-			if (cpl != desc.dpl)
-				goto bad;
-		} else if ((desc.type & 8) && (desc.type & 4)) {
-			/* conforming code segment */
-			if (cpl < desc.dpl)
+		} else {
+			/*
+			 * These are the (rare) cases that do not behave
+			 * like data segments: nonreadable code segments (bad)
+			 * and readable, conforming code segments (good).
+			 */
+			if (!(desc.type & 2))
 				goto bad;
 		}
 		break;