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=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 4E256C433E0 for ; Tue, 2 Jun 2020 13:11:31 +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 294E220674 for ; Tue, 2 Jun 2020 13:11:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 294E220674 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jg6hO-0005E7-Fn; Tue, 02 Jun 2020 13:11:22 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jg6hN-0005Dy-RC for xen-devel@lists.xenproject.org; Tue, 02 Jun 2020 13:11:21 +0000 X-Inumbo-ID: 8d468872-a4d2-11ea-9dbe-bc764e2007e4 Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 8d468872-a4d2-11ea-9dbe-bc764e2007e4; Tue, 02 Jun 2020 13:11:21 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id A421CABCE; Tue, 2 Jun 2020 13:11:22 +0000 (UTC) Subject: Re: [PATCH v2 10/14] x86/extable: Adjust extable handling to be shadow stack compatible To: Andrew Cooper References: <20200527191847.17207-1-andrew.cooper3@citrix.com> <20200527191847.17207-11-andrew.cooper3@citrix.com> <0c7f425a-996f-8840-f1e2-79381edb6456@citrix.com> <9d86ecf8-eaaa-7c2c-a3cc-b832d013a225@citrix.com> From: Jan Beulich Message-ID: Date: Tue, 2 Jun 2020 15:11:18 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: <9d86ecf8-eaaa-7c2c-a3cc-b832d013a225@citrix.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Xen-devel , Wei Liu , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On 29.05.2020 23:17, Andrew Cooper wrote: > On 29/05/2020 20:43, Andrew Cooper wrote: >> On 28/05/2020 17:15, Jan Beulich wrote: >>> On 27.05.2020 21:18, Andrew Cooper wrote: >>>> + >>>> + if ( ptr[0] == regs->rip && ptr[1] == regs->cs ) >>>> + { >>>> + asm ( "wrssq %[fix], %[stk]" >>>> + : [stk] "=m" (*ptr) >>> Could this be ptr[0], to match the if()? >>> >>> Considering how important it is that we don't fix up the wrong stack >>> location here, I continue to wonder if we wouldn't better also >>> include the SSP value on the stack in the checking, at the very >>> least by way of an ASSERT() or BUG_ON(). >> Well no, for the reason discussed in point 1. >> >> Its not technically an issue right now, but there is no possible way to >> BUILD_BUG_ON() someone turning an exception into IST, or stopping the >> use of the extable infrastructure on a #DB. >> >> Such a check would lie in wait and either provide an unexpected tangent >> to someone debugging a complicated issue (I do use #DB for a fair bit), >> or become a security vulnerability. > > Also (which I forgot first time around), > > The ptr[1] == regs->cs check is 64 bits wide, and the way to get that on > the shadow stack would be to execute a call instruction ending at at > 0xe008 linear, or from a bad WRSSQ edit, both of which are serious > errors and worthy of hitting the BUG(). Maybe you misunderstood me then: By suggesting more strict checking I'm actually asking for it to become more likely to hit that BUG(), not less likely. (This is despite agreeing that the very limited range of CS values on the stack already makes it practically impossible to mistake the frame found.) Jan