Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1948817AbdDYSfI (ORCPT ); Tue, 25 Apr 2017 14:35:08 -0400 Received: from mx2.suse.de ([195.135.220.15]:46344 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1953390AbdDYSen (ORCPT ); Tue, 25 Apr 2017 14:34:43 -0400 Subject: Re: [PATCH] x86/amd: don't set X86_BUG_SYSRET_SS_ATTRS if forced to zero To: Borislav Petkov References: <20170425180014.7533-1-jgross@suse.com> <20170425182443.3ab75tkfosol2yk4@pd.tnic> Cc: linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, x86@kernel.org, boris.ostrovsky@oracle.com, hpa@zytor.com, tglx@linutronix.de, mingo@redhat.com From: Juergen Gross Message-ID: Date: Tue, 25 Apr 2017 20:34:34 +0200 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: <20170425182443.3ab75tkfosol2yk4@pd.tnic> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2237 Lines: 59 On 25/04/17 20:24, Borislav Petkov wrote: > On Tue, Apr 25, 2017 at 08:00:14PM +0200, Juergen Gross wrote: >> When running as Xen pv guest X86_BUG_SYSRET_SS_ATTRS must not be set >> on AMD cpus. Xen will disable this via setup_clear_cpu_cap(), so test >> cpu_caps_cleared to not have disabled this bit. >> >> This bug/feature bit is kind of special as it will be used very early >> when switching threads. Setting the bit and clearing it a little bit >> later leaves a critical window where things can go wrong. This time >> window has enlarged a little bit by using setup_clear_cpu_cap() instead >> of the hypervisor's set_cpu_features callback. It seems this larger >> window now makes it rather easy to hit the problem. >> >> The proper solution is to never set the bit in case of Xen. >> >> Signed-off-by: Juergen Gross >> --- >> arch/x86/kernel/cpu/amd.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c >> index c36140d788fe..f659b6f534b7 100644 >> --- a/arch/x86/kernel/cpu/amd.c >> +++ b/arch/x86/kernel/cpu/amd.c >> @@ -800,7 +800,9 @@ static void init_amd(struct cpuinfo_x86 *c) >> set_cpu_cap(c, X86_FEATURE_3DNOWPREFETCH); >> >> /* AMD CPUs don't reset SS attributes on SYSRET */ >> - set_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS); >> + if (!test_bit(X86_BUG_SYSRET_SS_ATTRS, >> + (unsigned long *)cpu_caps_cleared)) >> + set_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS); >> } > > Hold on, AFAICT we have this order: > > c_init() -> init_amd() sets X86_BUG_SYSRET_SS_ATTRS 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. > init_hypervisor->x86_hyper->set_cpu_features-> clear_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS); And now (with setup_clear_cpu_cap()) the bit is cleared a little bit later. And all should be good. But it isn't. > > and all is good. > > And I remember seeing a patchset doing some xen cpuid cleanup so I'm > assuming you're doing setup_clear_cpu_cap() now? And now we have to wag > the dog? No, now the time window with X86_BUG_SYSRET_SS_ATTRS set is so long we actually see the problem happening. Juergen