Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753240AbcKRRdL (ORCPT ); Fri, 18 Nov 2016 12:33:11 -0500 Received: from mail-vk0-f49.google.com ([209.85.213.49]:36365 "EHLO mail-vk0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752140AbcKRRdI (ORCPT ); Fri, 18 Nov 2016 12:33:08 -0500 MIME-Version: 1.0 In-Reply-To: <20161118081444.GC15912@gmail.com> References: <20161117020610.5302-1-khuey@kylehuey.com> <20161117020610.5302-7-khuey@kylehuey.com> <20161118081444.GC15912@gmail.com> From: Andy Lutomirski Date: Fri, 18 Nov 2016 09:32:46 -0800 Message-ID: Subject: Re: [PATCH v12 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID To: Ingo Molnar Cc: Thomas Gleixner , Ingo Molnar , Jeff Dike , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , "open list:KERNEL SELFTEST FRAMEWORK" , Dmitry Safonov , Nadav Amit , Alexander Viro , "Rafael J. Wysocki" , Linux FS Devel , kvm list , "user-mode-linux-devel@lists.sourceforge.net" , X86 ML , "open list:USER-MODE LINUX (UML)" , Paolo Bonzini , Kyle Huey , Dave Hansen , "Robert O'Callahan" , Boris Ostrovsky , Shuah Khan , "linux-kernel@vger.kernel.org" , David Matlack , Borislav Petkov , Len Brown , Richard Weinberger , "H. Peter Anvin" , Peter Zijlstra Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3389 Lines: 81 On Nov 18, 2016 12:14 AM, "Ingo Molnar" wrote: > > > * Kyle Huey wrote: > > > Intel supports faulting on the CPUID instruction beginning with Ivy Bridge. > > When enabled, the processor will fault on attempts to execute the CPUID > > instruction with CPL>0. Exposing this feature to userspace will allow a > > ptracer to trap and emulate the CPUID instruction. > > > > When supported, this feature is controlled by toggling bit 0 of > > MSR_MISC_FEATURES_ENABLES. It is documented in detail in Section 2.3.2 of > > https://bugzilla.kernel.org/attachment.cgi?id=243991 > > > > Implement a new pair of arch_prctls, available on both x86-32 and x86-64. > > > > ARCH_GET_CPUID: Returns the current CPUID faulting state, either > > ARCH_CPUID_ENABLE or ARCH_CPUID_SIGSEGV. arg2 must be 0. > > > > ARCH_SET_CPUID: Set the CPUID faulting state to arg2, which must be either > > ARCH_CPUID_ENABLE or ARCH_CPUID_SIGSEGV. Returns EINVAL if arg2 is > > another value or CPUID faulting is not supported on this system. > > So the interface is: > > > +#define ARCH_GET_CPUID 0x1005 > > +#define ARCH_SET_CPUID 0x1006 > > +#define ARCH_CPUID_ENABLE 1 > > +#define ARCH_CPUID_SIGSEGV 2 > > Which maps to: > > prctl(ARCH_SET_CPUID, 0); /* -EINVAL */ > prctl(ARCH_SET_CPUID, 1); /* enable CPUID [i.e. make it work without faulting] */ > prctl(ARCH_SET_CPUID, 2); /* disable CPUID [i.e. make it fault] */ > > ret = prctl(ARCH_GET_CPUID, 0); /* return current state: 1==on, 2==off */ > > This is a very broken interface that makes very little sense. > > It would be much better to use a more natural interface where 1/0 means on/off and > where ARCH_GET_CPUID returns the current natural state: > > prctl(ARCH_SET_CPUID, 0); /* disable CPUID [i.e. make it fault] */ > prctl(ARCH_SET_CPUID, 1); /* enable CPUID [i.e. make it work without faulting] */ > > ret = prctl(ARCH_GET_CPUID); /* 1==enabled, 0==disabled */ > > See how natural it is? The use of the ARCH_CPUID_SIGSEGV/ENABLED symbols can be > avoided altogether. This will cut down on some of the ugliness in the kernel code > as well - and clean up the argument name as well: instead of naming it 'int arg2' > it can be named the more natural 'int cpuid_enabled'. > > > The state of the CPUID faulting flag is propagated across forks, but reset > > upon exec. > > I don't think this is the natural API for propagating settings across exec(). > We should reset the flag on exec() only if security considerations require it - > i.e. like perf events are cleared. > > If binaries that assume a working CPUID are exec()-ed then CPUID can be enabled > explicitly. I disagree. I'd rather not create more weird state that's carried across exec. We already have the iopl screwup IIRC. I think exec should stay as close to just working as possible. Also, if we keep it disabled across exec, we have to come up with a usable API that respects security considerations. We could use no_new_privs or we could auto-clear it on privilege changes. The former is IMO overcomplicated and the latter is really ugly especially when LSMs are involved. > > Clearing it automatically loses the ability of a pure no-CPUID environment to > exec() a CPUID-safe binary. If we really want this, let's wait until a user appears and add a "sticky" no-CPUID mode that requires no_new_privs to enable. --Andy