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

[Borislav Petkov]

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)
--