linux-kernel - Re: [PATCH v12 6/7] x86/arch_prctl: Add ARCH_[GET|SET]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALCETrX5SZh7=p1wk3yo8=vdg=veJmLwexMFGE=86bK1G6HkGQ@mail.gmail.com>
Date:   Fri, 18 Nov 2016 09:32:46 -0800
From:   Andy Lutomirski <luto@...capital.net>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Jeff Dike <jdike@...toit.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Dmitry Safonov <dsafonov@...tuozzo.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Linux FS Devel <linux-fsdevel@...r.kernel.org>,
        kvm list <kvm@...r.kernel.org>,
        "user-mode-linux-devel@...ts.sourceforge.net" 
        <user-mode-linux-devel@...ts.sourceforge.net>,
        X86 ML <x86@...nel.org>,
        "open list:USER-MODE LINUX (UML)" 
        <user-mode-linux-user@...ts.sourceforge.net>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Kyle Huey <me@...ehuey.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "Robert O'Callahan" <robert@...llahan.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Shuah Khan <shuah@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        David Matlack <dmatlack@...gle.com>,
        Borislav Petkov <bp@...e.de>, Len Brown <len.brown@...el.com>,
        Richard Weinberger <richard@....at>,
        "H. Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v12 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID

On Nov 18, 2016 12:14 AM, "Ingo Molnar" <mingo@...nel.org> wrote:
>
>
> * Kyle Huey <me@...ehuey.com> 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