Re: [PATCH v2 3/3] x86,arch_prctl Add ARCH_[GET|SET]_CPUID for controlling the CPUID instruction

[Kyle Huey]

On Wed, Sep 14, 2016 at 6:29 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> On Wed, Sep 14, 2016 at 2:01 PM, Kyle Huey <me@xxxxxxxxxxxx> wrote:
>> Intel supports faulting on the CPUID instruction in newer processors. Bit
>> 31 of MSR_PLATFORM_INFO advertises support for this feature. It is
>> documented in detail in Section 2.3.2 of
>> http://www.intel.com/content/dam/www/public/us/en/documents/application-notes/virtualization-technology-flexmigration-application-note.pdf
>>
>> Signed-off-by: Kyle Huey <khuey@xxxxxxxxxxxx>
>> ---
>>  arch/x86/include/asm/msr-index.h          |   1 +
>>  arch/x86/include/asm/thread_info.h        |   4 +-
>>  arch/x86/include/uapi/asm/prctl.h         |   6 +
>>  arch/x86/kernel/process.c                 |  81 +++++++++++
>>  tools/testing/selftests/x86/Makefile      |   2 +-
>>  tools/testing/selftests/x86/cpuid-fault.c | 223 ++++++++++++++++++++++++++++++
>>  6 files changed, 315 insertions(+), 2 deletions(-)
>>  create mode 100644 tools/testing/selftests/x86/cpuid-fault.c
>>
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 83908d5..4aebec2 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -53,6 +53,7 @@
>>  #define MSR_MTRRcap                    0x000000fe
>>  #define MSR_IA32_BBL_CR_CTL            0x00000119
>>  #define MSR_IA32_BBL_CR_CTL3           0x0000011e
>> +#define MSR_MISC_FEATURES_ENABLES      0x00000140
>>
>>  #define MSR_IA32_SYSENTER_CS           0x00000174
>>  #define MSR_IA32_SYSENTER_ESP          0x00000175
>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>> index 8b7c8d8..ec93976 100644
>> --- a/arch/x86/include/asm/thread_info.h
>> +++ b/arch/x86/include/asm/thread_info.h
>> @@ -93,6 +93,7 @@ struct thread_info {
>>  #define TIF_SECCOMP            8       /* secure computing */
>>  #define TIF_USER_RETURN_NOTIFY 11      /* notify kernel of userspace return */
>>  #define TIF_UPROBE             12      /* breakpointed or singlestepping */
>> +#define TIF_NOCPUID            15      /* CPUID is not accessible in userland */
>>  #define TIF_NOTSC              16      /* TSC is not accessible in userland */
>>  #define TIF_IA32               17      /* IA32 compatibility process */
>>  #define TIF_FORK               18      /* ret_from_fork */
>> @@ -117,6 +118,7 @@ struct thread_info {
>>  #define _TIF_SECCOMP           (1 << TIF_SECCOMP)
>>  #define _TIF_USER_RETURN_NOTIFY        (1 << TIF_USER_RETURN_NOTIFY)
>>  #define _TIF_UPROBE            (1 << TIF_UPROBE)
>> +#define _TIF_NOCPUID           (1 << TIF_NOCPUID)
>>  #define _TIF_NOTSC             (1 << TIF_NOTSC)
>>  #define _TIF_IA32              (1 << TIF_IA32)
>>  #define _TIF_FORK              (1 << TIF_FORK)
>> @@ -146,7 +148,7 @@ struct thread_info {
>>
>>  /* flags to check in __switch_to() */
>>  #define _TIF_WORK_CTXSW                                                        \
>> -       (_TIF_IO_BITMAP|_TIF_NOTSC|_TIF_BLOCKSTEP)
>> +       (_TIF_IO_BITMAP|_TIF_NOCPUID|_TIF_NOTSC|_TIF_BLOCKSTEP)
>>
>>  #define _TIF_WORK_CTXSW_PREV (_TIF_WORK_CTXSW|_TIF_USER_RETURN_NOTIFY)
>>  #define _TIF_WORK_CTXSW_NEXT (_TIF_WORK_CTXSW)
>> diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
>> index 3ac5032..c087e55 100644
>> --- a/arch/x86/include/uapi/asm/prctl.h
>> +++ b/arch/x86/include/uapi/asm/prctl.h
>> @@ -6,4 +6,10 @@
>>  #define ARCH_GET_FS 0x1003
>>  #define ARCH_GET_GS 0x1004
>>
>> +/* Get/set the process' ability to use the CPUID instruction */
>> +#define ARCH_GET_CPUID 0x1005
>> +#define ARCH_SET_CPUID 0x1006
>> +# define ARCH_CPUID_ENABLE             1       /* allow the use of the CPUID instruction */
>> +# define ARCH_CPUID_SIGSEGV            2       /* throw a SIGSEGV instead of reading the CPUID */
>> +
>>  #endif /* _ASM_X86_PRCTL_H */
>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>> index 0f857c3..5fc8e9d 100644
>> --- a/arch/x86/kernel/process.c
>> +++ b/arch/x86/kernel/process.c
>> @@ -193,6 +193,69 @@ int set_tsc_mode(unsigned int val)
>>         return 0;
>>  }
>>
>> +static void switch_cpuid_faulting(bool on)
>> +{
>> +       if (on)
>> +               msr_set_bit(MSR_MISC_FEATURES_ENABLES, 0);
>> +       else
>> +               msr_clear_bit(MSR_MISC_FEATURES_ENABLES, 0);
>> +}
>> +
>
> I don't know if we care (yet?), but this is going to be unnecessarily
> slow because of the implicit rdmsr.  You could add a percpu shadow
> copy of MISC_FEATURES_ENABLES, initialized during boot, and avoid the
> rdmsr.
>
>> +
>> +int set_cpuid_mode(struct task_struct *task, unsigned long val)
>> +{
>> +       /* Only disable/enable_cpuid() if it is supported on this hardware. */
>> +       bool cpuid_fault_supported = static_cpu_has(X86_FEATURE_CPUID_FAULT);
>> +
>> +       if (val == ARCH_CPUID_ENABLE && cpuid_fault_supported) {
>> +               if (task_no_new_privs(task) && test_thread_flag(TIF_NOCPUID))
>> +                       return -EACCES;
>
> This check seems confused.  If this flag were preserved on execve,
> it's the SIGSEGV mode that would need the check.

Not sure I follow this one.  no_new_privs should block transitions
from SIGSEGV to ENABLE, right?  That's what this check does.

>> @@ -212,6 +275,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))
>> +                       switch_cpuid_faulting(true);
>> +               else
>> +                       switch_cpuid_faulting(false);
>> +       }
>> +
>
> Off-topic and not needed for this patch, but IMO we should move all of
> this context switch junk out of ti.flags and into thread_struct
> somewhere.

- Kyle