linux-kernel - Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction.

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]

Date:   Mon, 12 Sep 2016 16:34:13 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Kyle Huey <me@...ehuey.com>
Cc:     Robert O'Callahan <robert@...llahan.org>,
        linux-api@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Len Brown <len.brown@...el.com>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        Huang Rui <ray.huang@....com>,
        Aravind Gopalakrishnan <Aravind.Gopalakrishnan@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>,
        Andy Lutomirski <luto@...nel.org>,
        Juergen Gross <jgross@...e.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Jiri Slaby <jslaby@...e.cz>, Michal Hocko <mhocko@...e.com>,
        Alex Thorlton <athorlton@....com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Mateusz Guzik <mguzik@...hat.com>,
        Ben Segall <bsegall@...gle.com>,
        John Stultz <john.stultz@...aro.org>,
        "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@...r.kernel.org>, kvm ML <kvm@...r.kernel.org>
Subject: Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the
 CPUID instruction.

On Mon, Sep 12, 2016 at 07:15:16AM -0700, Kyle Huey wrote:
> Copied from PR_SET_TSC. Would you prefer something like
> disable_cpuid/disable_cpuid_and_set_flag for
> hard_disable_CPUID/disable_CPUID?

Maybe something like this:

switch_cpuid_faulting(bool on)
{
	if (on)
		msr_set_bit(MSR_MISC_FEATURES_ENABLES, 0);
	else
		msr_clear_bit(MSR_MISC_FEATURES_ENABLES, 0);
}

and call it with the respective argument.

> >> @@ -210,6 +280,15 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
> >>               update_debugctlmsr(debugctl);
> >>       }
> >>
> >> +     if (test_tsk_thread_flag(prev_p, TIF_NOCPUID) ^
> >> +         test_tsk_thread_flag(next_p, TIF_NOCPUID)) {
> >> +             /* prev and next are different */
> >> +             if (test_tsk_thread_flag(next_p, TIF_NOCPUID))
> >> +                     hard_disable_CPUID();
> >> +             else
> >> +                     hard_enable_CPUID();
> >> +     }
> >> +
> >
> > Frankly, I can't say that I'm thrilled by this: if this is a niche
> > feature which has only a very narrow usage for debugging, I'd much
> > prefer if this whole thing were implemented with a static_key which was
> > false on the majority of the systems so that __switch_to() tests it much
> > cheaply.
> >
> > Then and only then if your debugger runs arch_prctl(), it would enable
> > the key and then set_cpuid_mode() can query the MSR directly instead of
> > using another flag in the thread_info flags.
> >
> > This would keep this niche feature out of the way of the hot paths.
> 
> My code is already in the slow path in __switch_to_xtra(), along with
> other debugging features like TIF_BLOCKSTEP and TIF_NOTSC. Adding a
> bit to the mask tested in __switch_to() shouldn't affect performance
> of the hot path.

TIF_BLOCKSTEP maybe but TIF_NOTSC doesn't look like a debugging feature
to me, especially if it is called in seccomp.

And I know it is not on the hot path. But you're using precious TIF bits
for a niche feature. Practically, this is code which will be dead on the
majority of machines out there, because either the hw feature is not
there or because nobody is using it.

Maybe the virtualization aspect would gather more users of this but I
don't know what kvm guys are thinking about faulting CPUID. Let me add
them to CC.

In any case, I'd do the static_key approach because it is simpler and
less obtrusive for your purpose.

But there are plenty more people on CC, this is just me.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--