From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v2 2/5] KVM: x86: Emulator performs code segment checks on read access Date: Sat, 11 Oct 2014 11:39:34 +0200 Message-ID: <5438FAD6.3010805@redhat.com> References: <20141006203238.GA4989@potion.brq.redhat.com> <1412906870-4322-1-git-send-email-namit@cs.technion.ac.il> <20141010155455.GA17902@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Nadav Amit Return-path: Received: from mail-wi0-f181.google.com ([209.85.212.181]:58034 "EHLO mail-wi0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751628AbaJKJjl (ORCPT ); Sat, 11 Oct 2014 05:39:41 -0400 Received: by mail-wi0-f181.google.com with SMTP id hi2so3993090wib.2 for ; Sat, 11 Oct 2014 02:39:40 -0700 (PDT) In-Reply-To: <20141010155455.GA17902@potion.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Il 10/10/2014 17:54, Radim Kr=C4=8Dm=C3=A1=C5=99 ha scritto: >> >=20 >> > 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] v= alid because >> > the DPL of the code segment selected by the CS register is the sam= e as the >> > CPL." This is misleading since CS.DPL may be lower (numerically) t= han 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 segmen= ts? >=20 > Method 2 is always valid because the privilege level of a conforming > code segment is effectively the same as the CPL, regardless of its D= PL. Radim is right; we need to skip the check on conforming code segments=20 and, once we do that, checking addr.seg is not necessary anymore. That= =20 is because, for a CS override on a nonconforming code segment, at the=20 time we fetch the instruction we know that cpl =3D=3D desc.dpl. The le= ss=20 restrictive data segment check (cpl <=3D 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 *ctx= t, if ((((ctxt->mode !=3D 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 =3D desc_limit_scaled(&desc); if ((ctxt->mode =3D=3D X86EMUL_MODE_REAL) && !fetch && (ctxt->d & NoBigReal)) { @@ -660,17 +657,40 @@ static int __linearize(struct x86_emulate_ctxt *c= txt, goto bad; } cpl =3D ctxt->ops->cpl(ctxt); - if (!(desc.type & 8)) { - /* data segment */ + if (fetch && (desc.type & 8)) { + if (!(desc.type & 4)) { + /* nonconforming code segment */ + if (cpl !=3D desc.dpl) + goto bad; + break; + } else { + /* conforming code segment */ + if (cpl < desc.dpl) + goto bad; + break; + } + } + + if (likely(!(desc.type & 8) || (desc.type & 6) =3D=3D 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 =3D=3D 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 !=3D 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;