Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1948981AbdDYURY (ORCPT ); Tue, 25 Apr 2017 16:17:24 -0400 Received: from smtp.eu.citrix.com ([185.25.65.24]:3863 "EHLO SMTP.EU.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1948558AbdDYURQ (ORCPT ); Tue, 25 Apr 2017 16:17:16 -0400 X-IronPort-AV: E=Sophos;i="5.37,250,1488844800"; d="scan'208";a="44944935" Subject: Re: [Xen-devel] [PATCH] x86/amd: don't set X86_BUG_SYSRET_SS_ATTRS if forced to zero To: Borislav Petkov , Juergen Gross References: <20170425180014.7533-1-jgross@suse.com> <20170425182443.3ab75tkfosol2yk4@pd.tnic> <20170425191809.uvdt4jimnbvqbyf2@pd.tnic> CC: , , , , , , From: Andrew Cooper Message-ID: <4dcc2a92-5d58-67df-1bd4-8f36cbaa16b4@citrix.com> Date: Tue, 25 Apr 2017 21:17:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170425191809.uvdt4jimnbvqbyf2@pd.tnic> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: AMSPEX02CAS02.citrite.net (10.69.22.113) To AMSPEX02CL02.citrite.net (10.69.22.126) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1122 Lines: 34 On 25/04/17 20:18, Borislav Petkov wrote: > On Tue, Apr 25, 2017 at 08:34:34PM +0200, Juergen Gross wrote: >> And what happens when there is a scheduling event right here? >> __switch_to() will see X86_BUG_SYSRET_SS_ATTRS set and take a wrong >> path. > So the whole thing we're doing right now is wrong: set bit and then > clear bit. > > We should not set the bit at all and there won't be any window to get it > wrong. > > So can we do something like this instead: > > if (!cpu_has(c, X86_FEATURE_XENPV)) > set_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS); > > or is XENPV the wrong thing to test? > X86_BUG_SYSRET_SS_ATTRS only actually causes a problem if you enter the kernel via an IDT vector (forcing %ss to NULL), then exiting the kernel via the optimistic sysret path, which on AMD loads the %ss selector, but apparently doesn't update the segment cache (and %ss.dpl in particular). The problem (for all ring-deprivileged virtuailsation; not just Xen PV), is that savesegment(ss, ss_sel); if (ss_sel != __KERNEL_DS) loadsegment(ss, __KERNEL_DS); tries to load %ss with an RPL of 0, and things blow up. ~Andrew