From patchwork Sat Oct 11 09:39:34 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 5068781 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 02104C11AC for ; Sat, 11 Oct 2014 09:39:59 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 248EF201D3 for ; Sat, 11 Oct 2014 09:39:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 30E30201C8 for ; Sat, 11 Oct 2014 09:39:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751801AbaJKJjm (ORCPT ); Sat, 11 Oct 2014 05:39:42 -0400 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=Lq0f15fke5gbLEgS+gfuVH0P8FvnPvvQOBv5E9I7e+E=; b=KAPdC+AcsIbVS+Dchm9zb3tlq4Jo+3BiT6vT5+5sD+EJpnuPhkZfDPsSxfRWLtp/vM c1eRRVtk7XbSQU1qRyKVhQQmZyKLq8CfRQ+8QXCDjNwK19c2a/D7A4iKITVSsQrykEcK MnBnZTiHHCGC2uQHn9iSt06zFR7AUWb+smX9ziGVrAaHkVzNcDwfLJKZtb8Iw5mI26q+ k7LFHhdW9/JyoxT+2vQBfK6XvGeYilzdVlAXWekLp1ayJ2WDhvFE79t/Aq2maPJlnwJ/ xqMYcnlActOt967gKRtGB4PB5gdKnhPhN8oHOmJhYuj1jClPbZDgS+L/nnN1SVqYWCOH sajw== X-Received: by 10.194.240.227 with SMTP id wd3mr9886233wjc.29.1413020380310; Sat, 11 Oct 2014 02:39:40 -0700 (PDT) Received: from yakj.usersys.redhat.com (pd9569ec7.dip0.t-ipconnect.de. [217.86.158.199]) by mx.google.com with ESMTPSA id bi7sm5170599wib.17.2014.10.11.02.39.38 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 11 Oct 2014 02:39:39 -0700 (PDT) Message-ID: <5438FAD6.3010805@redhat.com> Date: Sat, 11 Oct 2014 11:39:34 +0200 From: Paolo Bonzini User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Nadav Amit CC: kvm@vger.kernel.org Subject: Re: [PATCH v2 2/5] KVM: x86: Emulator performs code segment checks on read access References: <20141006203238.GA4989@potion.brq.redhat.com> <1412906870-4322-1-git-send-email-namit@cs.technion.ac.il> <20141010155455.GA17902@potion.brq.redhat.com> In-Reply-To: <20141010155455.GA17902@potion.brq.redhat.com> Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID,T_RP_MATCHES_RCVD,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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? --- 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 --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;