From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E6D8CC433E7 for ; Wed, 14 Oct 2020 13:57:46 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 65B522222A for ; Wed, 14 Oct 2020 13:57:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=citrix.com header.i=@citrix.com header.b="b0z3NeRE" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 65B522222A Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.6755.17767 (Exim 4.92) (envelope-from ) id 1kShHQ-0006W5-NM; Wed, 14 Oct 2020 13:57:24 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 6755.17767; Wed, 14 Oct 2020 13:57:24 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kShHQ-0006Vy-Jv; Wed, 14 Oct 2020 13:57:24 +0000 Received: by outflank-mailman (input) for mailman id 6755; Wed, 14 Oct 2020 13:57:23 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kShHO-0006Vt-Uy for xen-devel@lists.xenproject.org; Wed, 14 Oct 2020 13:57:23 +0000 Received: from esa6.hc3370-68.iphmx.com (unknown [216.71.155.175]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id e3b40573-b13e-4974-87ae-0555cc73369a; Wed, 14 Oct 2020 13:57:21 +0000 (UTC) Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kShHO-0006Vt-Uy for xen-devel@lists.xenproject.org; Wed, 14 Oct 2020 13:57:23 +0000 X-Inumbo-ID: e3b40573-b13e-4974-87ae-0555cc73369a Received: from esa6.hc3370-68.iphmx.com (unknown [216.71.155.175]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id e3b40573-b13e-4974-87ae-0555cc73369a; Wed, 14 Oct 2020 13:57:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=citrix.com; s=securemail; t=1602683841; h=subject:to:cc:references:from:message-id:date: mime-version:in-reply-to:content-transfer-encoding; bh=JJyQ7tUp+FCSQm7uoVm5LWpXqpz9Gw0sQv0caoKYOBI=; b=b0z3NeRE6vzNo3FaArBvdzmsjIywBJ1+VBMS4IB0TmMqDqx2D6R38LyF kft1hDX2ImevcBUPqTLDdvL+ntNu4oPgkBg1CXsoITDP9fOQMwitn3htr SSHDOCQ9iCnC2W+dMQLV0l5haj1sUnSZyDxFJYilSxc3gP+TVxq2orAMO I=; Authentication-Results: esa6.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none IronPort-SDR: hbvgQmFay1opfKYEpyyup9fMjxU35xHRqW4Qt5PkxPJphQD1TdJtu48Wl6TB3wRbc+yCQRfD7T vwAY/m9clL5Nf6Ug0MB4vkzjkiMrEtFHo3IyKgwl9xONa0FsVRiZrYTiLkQX57ZQ+C3FAoaJKr Frblsf46cToQ0FJrHuFJSI9C9ls3Nqi03VUKu2IYUQFpfcGvXMJV4sxCC0lsmHY6rP5ZDmZxVg kxxWIS3a0es9rYT2hbpEeBuMPgpNyDVrkDunK5MF2e3ZZaaCaRWuxUxynUHTQfV+eSIPnr18lA qB8= X-SBRS: 2.5 X-MesageID: 29236849 X-Ironport-Server: esa6.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.77,374,1596513600"; d="scan'208";a="29236849" Subject: Re: [PATCH] x86/vmx: Revert "x86/VMX: sanitize rIP before re-entering guest" To: Jan Beulich CC: Xen-devel , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= , Wei Liu , Jun Nakajima , Kevin Tian References: <20201009150948.31063-1-andrew.cooper3@citrix.com> From: Andrew Cooper Message-ID: <01bb2f27-4e0b-3637-e456-09eb7b9b233e@citrix.com> Date: Wed, 14 Oct 2020 14:57:14 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Content-Language: en-GB X-ClientProxiedBy: AMSPEX02CAS01.citrite.net (10.69.22.112) To FTLPEX02CL05.citrite.net (10.13.108.178) On 13/10/2020 16:58, Jan Beulich wrote: > On 09.10.2020 17:09, Andrew Cooper wrote: >> At the time of XSA-170, the x86 instruction emulator really was broken, and >> would allow arbitrary non-canonical values to be loaded into %rip. This was >> fixed after the embargo by c/s 81d3a0b26c1 "x86emul: limit-check branch >> targets". >> >> However, in a demonstration that off-by-one errors really are one of the >> hardest programming issues we face, everyone involved with XSA-170, myself >> included, mistook the statement in the SDM which says: >> >> If the processor supports N < 64 linear-address bits, bits 63:N must be identical >> >> to mean "must be canonical". A real canonical check is bits 63:N-1. >> >> VMEntries really do tolerate a not-quite-canonical %rip, specifically to cater >> to the boundary condition at 0x0000800000000000. >> >> Now that the emulator has been fixed, revert the XSA-170 change to fix >> architectural behaviour at the boundary case. The XTF test case for XSA-170 >> exercises this corner case, and still passes. >> >> Fixes: ffbbfda377 ("x86/VMX: sanitize rIP before re-entering guest") >> Signed-off-by: Andrew Cooper > But why revert the change rather than fix ... > >> @@ -4280,38 +4280,6 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) >> out: >> if ( nestedhvm_vcpu_in_guestmode(v) ) >> nvmx_idtv_handling(); >> - >> - /* >> - * VM entry will fail (causing the guest to get crashed) if rIP (and >> - * rFLAGS, but we don't have an issue there) doesn't meet certain >> - * criteria. As we must not allow less than fully privileged mode to have >> - * such an effect on the domain, we correct rIP in that case (accepting >> - * this not being architecturally correct behavior, as the injected #GP >> - * fault will then not see the correct [invalid] return address). >> - * And since we know the guest will crash, we crash it right away if it >> - * already is in most privileged mode. >> - */ >> - mode = vmx_guest_x86_mode(v); >> - if ( mode == 8 ? !is_canonical_address(regs->rip) > ... the wrong use of is_canonical_address() here? By reverting > you open up avenues for XSAs in case we get things wrong elsewhere, > including ... > >> - : regs->rip != regs->eip ) > ... for 32-bit guests. Because the only appropriate alternative would be ASSERT_UNREACHABLE() and domain crash. This logic corrupts guest state. Running with corrupt state is every bit an XSA as hitting a VMEntry failure if it can be triggered by userspace, but the latter safer and much more obvious. It was the appropriate security fix (give or take the functional bug in it) at the time, given the complexity of retrofitting zero length instruction fetches to the emulator. However, it is one of a very long list of guest-state-induced VMEntry failures, with non-trivial logic which we assert will pass, on a fastpath, where hardware also performs the same checks and we already have a runtime safe way of dealing with errors.  (Hence not actually using ASSERT_UNREACHABLE() here.) It isn't appropriate for this check to exist on its own (i.e. without other guest state checks), and it isn't appropriate to live in a fastpath.  In principle, some logic like this could live on the vmentry failure path to try a second time, but then you're still creating an XSA situation which is less obvious, and therefore isn't a clever move IMO. ~Andrew