* [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. @ 2016-09-12 0:29 Kyle Huey [not found] ` <1473640169-24145-1-git-send-email-khuey-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> 0 siblings, 1 reply; 16+ messages in thread From: Kyle Huey @ 2016-09-12 0:29 UTC (permalink / raw) To: Robert O'Callahan Cc: linux-api-u79uwXL29TY76Z2rM5mHXA, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, maintainer:X86 ARCHITECTURE 32-BIT AND 64-BIT, Peter Zijlstra (Intel), Rafael J. Wysocki, Borislav Petkov, Len Brown, Srinivas Pandruvada, Huang Rui, Aravind Gopalakrishnan, Alexander Shishkin, Vladimir Zapolskiy, Andy Lutomirski, Juergen Gross, Fenghua Yu, Luis R. Rodriguez, Denys rr (http://rr-project.org/), a userspace record-and-replay reverse- execution debugger, would like to trap and emulate the CPUID instruction. This would allow us to a) mask away certain hardware features that rr does not support (e.g. RDRAND) and b) enable trace portability across machines by providing constant results. 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. I would like to thank Trevor Saunders <tbsaunde-lLNik5A/BD1g9hUCZPvPmw@public.gmane.org> for drafting an earlier version of this patch. Signed-off-by Kyle Huey <khuey-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> --- arch/x86/include/asm/msr-index.h | 1 + arch/x86/include/asm/processor.h | 7 ++++ arch/x86/include/asm/thread_info.h | 4 +- arch/x86/kernel/process.c | 79 ++++++++++++++++++++++++++++++++++++++ include/uapi/linux/prctl.h | 6 +++ kernel/sys.c | 12 ++++++ 6 files changed, 108 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index 56f4c66..28b0736 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -52,6 +52,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/processor.h b/arch/x86/include/asm/processor.h index 63def95..661c4c1 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -805,6 +805,13 @@ extern void start_thread(struct pt_regs *regs, unsigned long new_ip, extern int get_tsc_mode(unsigned long adr); extern int set_tsc_mode(unsigned int val); +/* Get/set a process' ability to use the CPUID instruction */ +#define GET_CPUID_CTL(adr) get_cpuid_mode((adr)) +#define SET_CPUID_CTL(val) set_cpuid_mode((val)) + +extern int get_cpuid_mode(unsigned long adr); +extern int set_cpuid_mode(unsigned int val); + /* Register/unregister a process' MPX related resource */ #define MPX_ENABLE_MANAGEMENT() mpx_enable_management() #define MPX_DISABLE_MANAGEMENT() mpx_disable_management() 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/kernel/process.c b/arch/x86/kernel/process.c index 62c0b0e..a189516 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -191,6 +191,76 @@ int set_tsc_mode(unsigned int val) return 0; } +static void hard_disable_CPUID(void) +{ + msr_set_bit(MSR_MISC_FEATURES_ENABLES, 0); +} + +static void disable_CPUID(void) +{ + preempt_disable(); + if (!test_and_set_thread_flag(TIF_NOCPUID)) + /* + * Must flip the CPU state synchronously with + * TIF_NOCPUID in the current running context. + */ + hard_disable_CPUID(); + preempt_enable(); +} + +static void hard_enable_CPUID(void) +{ + msr_clear_bit(MSR_MISC_FEATURES_ENABLES, 0); +} + +static void enable_CPUID(void) +{ + preempt_disable(); + if (test_and_clear_thread_flag(TIF_NOCPUID)) + /* + * Must flip the CPU state synchronously with + * TIF_NOCPUID in the current running context. + */ + hard_enable_CPUID(); + preempt_enable(); +} + +static int supports_CPUID_faulting(void) +{ + unsigned int lo, hi; + + rdmsr(MSR_PLATFORM_INFO, lo, hi); + if ((lo & (1 << 31))) + return 1; + else + return 0; +} + +int get_cpuid_mode(unsigned long adr) +{ + unsigned int val; + + if (test_thread_flag(TIF_NOCPUID)) + val = PR_CPUID_SIGSEGV; + else + val = PR_CPUID_ENABLE; + + return put_user(val, (unsigned int __user *)adr); +} + +int set_cpuid_mode(unsigned int val) +{ + // Only disable/enable_CPUID() if it is supported on this hardware. + if (val == PR_CPUID_SIGSEGV && supports_CPUID_faulting()) + disable_CPUID(); + else if (val == PR_CPUID_ENABLE && supports_CPUID_faulting()) + enable_CPUID(); + else + return -EINVAL; + + return 0; +} + void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p, struct tss_struct *tss) { @@ -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(); + } + if (test_tsk_thread_flag(prev_p, TIF_NOTSC) ^ test_tsk_thread_flag(next_p, TIF_NOTSC)) { /* prev and next are different */ diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h index a8d0759..641d12b 100644 --- a/include/uapi/linux/prctl.h +++ b/include/uapi/linux/prctl.h @@ -197,4 +197,10 @@ struct prctl_mm_map { # define PR_CAP_AMBIENT_LOWER 3 # define PR_CAP_AMBIENT_CLEAR_ALL 4 +/* Get/set the process' ability to use the CPUID instruction */ +#define PR_GET_CPUID 48 +#define PR_SET_CPUID 49 +# define PR_CPUID_ENABLE 1 /* allow the use of the CPUID instruction */ +# define PR_CPUID_SIGSEGV 2 /* throw a SIGSEGV instead of reading the CPUID */ + #endif /* _LINUX_PRCTL_H */ diff --git a/kernel/sys.c b/kernel/sys.c index 89d5be4..997c6519 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -91,6 +91,12 @@ #ifndef SET_TSC_CTL # define SET_TSC_CTL(a) (-EINVAL) #endif +#ifndef GET_CPUID_CTL +# define GET_CPUID_CTL(a) (-EINVAL) +#endif +#ifndef SET_CPUID_CTL +# define SET_CPUID_CTL(a) (-EINVAL) +#endif #ifndef MPX_ENABLE_MANAGEMENT # define MPX_ENABLE_MANAGEMENT() (-EINVAL) #endif @@ -2162,6 +2168,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, case PR_SET_TSC: error = SET_TSC_CTL(arg2); break; + case PR_GET_CPUID: + error = GET_CPUID_CTL(arg2); + break; + case PR_SET_CPUID: + error = SET_CPUID_CTL(arg2); + break; case PR_TASK_PERF_EVENTS_DISABLE: error = perf_event_task_disable(); break; -- 2.7.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
[parent not found: <1473640169-24145-1-git-send-email-khuey-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org>]
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. [not found] ` <1473640169-24145-1-git-send-email-khuey-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> @ 2016-09-12 9:07 ` Borislav Petkov 2016-09-12 14:15 ` Kyle Huey 2016-09-12 16:56 ` Andy Lutomirski 1 sibling, 1 reply; 16+ messages in thread From: Borislav Petkov @ 2016-09-12 9:07 UTC (permalink / raw) To: Kyle Huey Cc: Robert O'Callahan, linux-api-u79uwXL29TY76Z2rM5mHXA, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Peter Zijlstra (Intel), Rafael J. Wysocki, Len Brown, Srinivas Pandruvada, Huang Rui, Aravind Gopalakrishnan, Alexander Shishkin, Vladimir Zapolskiy, Andy Lutomirski, Juergen Gross, Fenghua Yu, Luis R. Rodriguez On Sun, Sep 11, 2016 at 05:29:23PM -0700, Kyle Huey wrote: > rr (http://rr-project.org/), a userspace record-and-replay reverse- > execution debugger, would like to trap and emulate the CPUID instruction. > This would allow us to a) mask away certain hardware features that rr does > not support (e.g. RDRAND) and b) enable trace portability across machines > by providing constant results. > > 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. > > I would like to thank Trevor Saunders <tbsaunde-lLNik5A/BD1g9hUCZPvPmw@public.gmane.org> for drafting > an earlier version of this patch. > > Signed-off-by Kyle Huey <khuey-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> > --- > arch/x86/include/asm/msr-index.h | 1 + > arch/x86/include/asm/processor.h | 7 ++++ > arch/x86/include/asm/thread_info.h | 4 +- > arch/x86/kernel/process.c | 79 ++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/prctl.h | 6 +++ > kernel/sys.c | 12 ++++++ > 6 files changed, 108 insertions(+), 1 deletion(-) ... > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > index 62c0b0e..a189516 100644 > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c > @@ -191,6 +191,76 @@ int set_tsc_mode(unsigned int val) > return 0; > } > > +static void hard_disable_CPUID(void) Why hard_disable? I don't see any soft_disable. Also, I can't say that I like all that screaming "CPUID" :-) disable_cpuid() looks just fine to me too. > +{ > + msr_set_bit(MSR_MISC_FEATURES_ENABLES, 0); > +} > + > +static void disable_CPUID(void) > +{ > + preempt_disable(); > + if (!test_and_set_thread_flag(TIF_NOCPUID)) > + /* > + * Must flip the CPU state synchronously with > + * TIF_NOCPUID in the current running context. > + */ > + hard_disable_CPUID(); > + preempt_enable(); > +} > + > +static void hard_enable_CPUID(void) > +{ > + msr_clear_bit(MSR_MISC_FEATURES_ENABLES, 0); > +} > + > +static void enable_CPUID(void) > +{ > + preempt_disable(); > + if (test_and_clear_thread_flag(TIF_NOCPUID)) > + /* > + * Must flip the CPU state synchronously with > + * TIF_NOCPUID in the current running context. > + */ > + hard_enable_CPUID(); > + preempt_enable(); > +} > + > +static int supports_CPUID_faulting(void) > +{ > + unsigned int lo, hi; > + > + rdmsr(MSR_PLATFORM_INFO, lo, hi); rdmsr_safe() > + if ((lo & (1 << 31))) > + return 1; > + else > + return 0; > +} > > +int get_cpuid_mode(unsigned long adr) > +{ > + unsigned int val; > + > + if (test_thread_flag(TIF_NOCPUID)) > + val = PR_CPUID_SIGSEGV; > + else > + val = PR_CPUID_ENABLE; > + > + return put_user(val, (unsigned int __user *)adr); > +} > + > +int set_cpuid_mode(unsigned int val) > +{ > + // Only disable/enable_CPUID() if it is supported on this hardware. Use /* ... */ for comments in the kernel. > + if (val == PR_CPUID_SIGSEGV && supports_CPUID_faulting()) > + disable_CPUID(); > + else if (val == PR_CPUID_ENABLE && supports_CPUID_faulting()) > + enable_CPUID(); > + else > + return -EINVAL; > + > + return 0; > +} > + > void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p, > struct tss_struct *tss) > { > @@ -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. > if (test_tsk_thread_flag(prev_p, TIF_NOTSC) ^ > test_tsk_thread_flag(next_p, TIF_NOTSC)) { > /* prev and next are different */ > diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h > index a8d0759..641d12b 100644 > --- a/include/uapi/linux/prctl.h > +++ b/include/uapi/linux/prctl.h > @@ -197,4 +197,10 @@ struct prctl_mm_map { > # define PR_CAP_AMBIENT_LOWER 3 > # define PR_CAP_AMBIENT_CLEAR_ALL 4 > > +/* Get/set the process' ability to use the CPUID instruction */ > +#define PR_GET_CPUID 48 > +#define PR_SET_CPUID 49 > +# define PR_CPUID_ENABLE 1 /* allow the use of the CPUID instruction */ > +# define PR_CPUID_SIGSEGV 2 /* throw a SIGSEGV instead of reading the CPUID */ > + > #endif /* _LINUX_PRCTL_H */ > diff --git a/kernel/sys.c b/kernel/sys.c > index 89d5be4..997c6519 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -91,6 +91,12 @@ > #ifndef SET_TSC_CTL > # define SET_TSC_CTL(a) (-EINVAL) > #endif > +#ifndef GET_CPUID_CTL > +# define GET_CPUID_CTL(a) (-EINVAL) > +#endif > +#ifndef SET_CPUID_CTL > +# define SET_CPUID_CTL(a) (-EINVAL) > +#endif > #ifndef MPX_ENABLE_MANAGEMENT > # define MPX_ENABLE_MANAGEMENT() (-EINVAL) > #endif > @@ -2162,6 +2168,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > case PR_SET_TSC: > error = SET_TSC_CTL(arg2); > break; > + case PR_GET_CPUID: > + error = GET_CPUID_CTL(arg2); > + break; > + case PR_SET_CPUID: > + error = SET_CPUID_CTL(arg2); > + break; > case PR_TASK_PERF_EVENTS_DISABLE: > error = perf_event_task_disable(); > break; This whole fun should be in arch_prctl() as it is arch-specific. And wherever it ends, it needs documenting in the man page. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. 2016-09-12 9:07 ` Borislav Petkov @ 2016-09-12 14:15 ` Kyle Huey 2016-09-12 14:34 ` Borislav Petkov 2016-09-13 18:42 ` Kyle Huey 0 siblings, 2 replies; 16+ messages in thread From: Kyle Huey @ 2016-09-12 14:15 UTC (permalink / raw) To: Borislav Petkov Cc: Robert O'Callahan, linux-api, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Peter Zijlstra (Intel), Rafael J. Wysocki, Len Brown, Srinivas Pandruvada, Huang Rui, Aravind Gopalakrishnan, Alexander Shishkin, Vladimir Zapolskiy, Andy Lutomirski, Juergen Gross, Fenghua Yu, Luis R. Rodriguez Thanks for the review! On Mon, Sep 12, 2016 at 2:07 AM, Borislav Petkov <bp@suse.de> wrote: > On Sun, Sep 11, 2016 at 05:29:23PM -0700, Kyle Huey wrote: >> rr (http://rr-project.org/), a userspace record-and-replay reverse- >> execution debugger, would like to trap and emulate the CPUID instruction. >> This would allow us to a) mask away certain hardware features that rr does >> not support (e.g. RDRAND) and b) enable trace portability across machines >> by providing constant results. >> >> 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. >> >> I would like to thank Trevor Saunders <tbsaunde@tbsaunde.org> for drafting >> an earlier version of this patch. >> >> Signed-off-by Kyle Huey <khuey@kylehuey.com> >> --- >> arch/x86/include/asm/msr-index.h | 1 + >> arch/x86/include/asm/processor.h | 7 ++++ >> arch/x86/include/asm/thread_info.h | 4 +- >> arch/x86/kernel/process.c | 79 ++++++++++++++++++++++++++++++++++++++ >> include/uapi/linux/prctl.h | 6 +++ >> kernel/sys.c | 12 ++++++ >> 6 files changed, 108 insertions(+), 1 deletion(-) > > ... > >> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c >> index 62c0b0e..a189516 100644 >> --- a/arch/x86/kernel/process.c >> +++ b/arch/x86/kernel/process.c >> @@ -191,6 +191,76 @@ int set_tsc_mode(unsigned int val) >> return 0; >> } >> >> +static void hard_disable_CPUID(void) > > Why hard_disable? I don't see any soft_disable. Copied from PR_SET_TSC. Would you prefer something like disable_cpuid/disable_cpuid_and_set_flag for hard_disable_CPUID/disable_CPUID? > Also, I can't say that I like all that screaming "CPUID" :-) > > disable_cpuid() looks just fine to me too. Ok. >> +{ >> + msr_set_bit(MSR_MISC_FEATURES_ENABLES, 0); >> +} >> + >> +static void disable_CPUID(void) >> +{ >> + preempt_disable(); >> + if (!test_and_set_thread_flag(TIF_NOCPUID)) >> + /* >> + * Must flip the CPU state synchronously with >> + * TIF_NOCPUID in the current running context. >> + */ >> + hard_disable_CPUID(); >> + preempt_enable(); >> +} >> + >> +static void hard_enable_CPUID(void) >> +{ >> + msr_clear_bit(MSR_MISC_FEATURES_ENABLES, 0); >> +} >> + >> +static void enable_CPUID(void) >> +{ >> + preempt_disable(); >> + if (test_and_clear_thread_flag(TIF_NOCPUID)) >> + /* >> + * Must flip the CPU state synchronously with >> + * TIF_NOCPUID in the current running context. >> + */ >> + hard_enable_CPUID(); >> + preempt_enable(); >> +} >> + >> +static int supports_CPUID_faulting(void) >> +{ >> + unsigned int lo, hi; >> + >> + rdmsr(MSR_PLATFORM_INFO, lo, hi); > > rdmsr_safe() Ok. >> + if ((lo & (1 << 31))) >> + return 1; >> + else >> + return 0; >> +} >> >> +int get_cpuid_mode(unsigned long adr) >> +{ >> + unsigned int val; >> + >> + if (test_thread_flag(TIF_NOCPUID)) >> + val = PR_CPUID_SIGSEGV; >> + else >> + val = PR_CPUID_ENABLE; >> + >> + return put_user(val, (unsigned int __user *)adr); >> +} >> + >> +int set_cpuid_mode(unsigned int val) >> +{ >> + // Only disable/enable_CPUID() if it is supported on this hardware. > > Use /* ... */ for comments in the kernel. Ok. >> + if (val == PR_CPUID_SIGSEGV && supports_CPUID_faulting()) >> + disable_CPUID(); >> + else if (val == PR_CPUID_ENABLE && supports_CPUID_faulting()) >> + enable_CPUID(); >> + else >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p, >> struct tss_struct *tss) >> { >> @@ -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. >> if (test_tsk_thread_flag(prev_p, TIF_NOTSC) ^ >> test_tsk_thread_flag(next_p, TIF_NOTSC)) { >> /* prev and next are different */ >> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h >> index a8d0759..641d12b 100644 >> --- a/include/uapi/linux/prctl.h >> +++ b/include/uapi/linux/prctl.h >> @@ -197,4 +197,10 @@ struct prctl_mm_map { >> # define PR_CAP_AMBIENT_LOWER 3 >> # define PR_CAP_AMBIENT_CLEAR_ALL 4 >> >> +/* Get/set the process' ability to use the CPUID instruction */ >> +#define PR_GET_CPUID 48 >> +#define PR_SET_CPUID 49 >> +# define PR_CPUID_ENABLE 1 /* allow the use of the CPUID instruction */ >> +# define PR_CPUID_SIGSEGV 2 /* throw a SIGSEGV instead of reading the CPUID */ >> + >> #endif /* _LINUX_PRCTL_H */ >> diff --git a/kernel/sys.c b/kernel/sys.c >> index 89d5be4..997c6519 100644 >> --- a/kernel/sys.c >> +++ b/kernel/sys.c >> @@ -91,6 +91,12 @@ >> #ifndef SET_TSC_CTL >> # define SET_TSC_CTL(a) (-EINVAL) >> #endif >> +#ifndef GET_CPUID_CTL >> +# define GET_CPUID_CTL(a) (-EINVAL) >> +#endif >> +#ifndef SET_CPUID_CTL >> +# define SET_CPUID_CTL(a) (-EINVAL) >> +#endif >> #ifndef MPX_ENABLE_MANAGEMENT >> # define MPX_ENABLE_MANAGEMENT() (-EINVAL) >> #endif >> @@ -2162,6 +2168,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, >> case PR_SET_TSC: >> error = SET_TSC_CTL(arg2); >> break; >> + case PR_GET_CPUID: >> + error = GET_CPUID_CTL(arg2); >> + break; >> + case PR_SET_CPUID: >> + error = SET_CPUID_CTL(arg2); >> + break; >> case PR_TASK_PERF_EVENTS_DISABLE: >> error = perf_event_task_disable(); >> break; > > This whole fun should be in arch_prctl() as it is arch-specific. Yeah, I was debating about that, and did it this way because of PR_SET_TSC. Will fix. > And wherever it ends, it needs documenting in the man page. Indeed. Would like to reach consensus on the TIF vs. static key thing before updating the patch. Thanks! - Kyle ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. 2016-09-12 14:15 ` Kyle Huey @ 2016-09-12 14:34 ` Borislav Petkov 2016-09-13 18:42 ` Kyle Huey 1 sibling, 0 replies; 16+ messages in thread From: Borislav Petkov @ 2016-09-12 14:34 UTC (permalink / raw) To: Kyle Huey Cc: Robert O'Callahan, linux-api, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Peter Zijlstra (Intel), Rafael J. Wysocki, Len Brown, Srinivas Pandruvada, Huang Rui, Aravind Gopalakrishnan, Alexander Shishkin, Vladimir Zapolskiy, Andy Lutomirski, Juergen Gross, Fenghua Yu, Luis R. Rodriguez 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) -- ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. 2016-09-12 14:15 ` Kyle Huey 2016-09-12 14:34 ` Borislav Petkov @ 2016-09-13 18:42 ` Kyle Huey 1 sibling, 0 replies; 16+ messages in thread From: Kyle Huey @ 2016-09-13 18:42 UTC (permalink / raw) To: Borislav Petkov Cc: Robert O'Callahan, linux-api, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Peter Zijlstra (Intel), Rafael J. Wysocki, Len Brown, Srinivas Pandruvada, Huang Rui, Aravind Gopalakrishnan, Alexander Shishkin, Vladimir Zapolskiy, Andy Lutomirski, Juergen Gross, Fenghua Yu, Luis R. Rodriguez On Mon, Sep 12, 2016 at 7:15 AM, Kyle Huey <me@kylehuey.com> wrote: > On Mon, Sep 12, 2016 at 2:07 AM, Borislav Petkov <bp@suse.de> wrote: >> On Sun, Sep 11, 2016 at 05:29:23PM -0700, Kyle Huey wrote: >>> @@ -2162,6 +2168,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, >>> case PR_SET_TSC: >>> error = SET_TSC_CTL(arg2); >>> break; >>> + case PR_GET_CPUID: >>> + error = GET_CPUID_CTL(arg2); >>> + break; >>> + case PR_SET_CPUID: >>> + error = SET_CPUID_CTL(arg2); >>> + break; >>> case PR_TASK_PERF_EVENTS_DISABLE: >>> error = perf_event_task_disable(); >>> break; >> >> This whole fun should be in arch_prctl() as it is arch-specific. > > Yeah, I was debating about that, and did it this way because of > PR_SET_TSC. Will fix. arch_prctl is not yet exposed on 32 bit x86, so we'll have to add that as well to do this. - Kyle ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. [not found] ` <1473640169-24145-1-git-send-email-khuey-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> 2016-09-12 9:07 ` Borislav Petkov @ 2016-09-12 16:56 ` Andy Lutomirski [not found] ` <CALCETrVJAib918qn9CL4Tvyfok-8eAHimYDabXUF=1Nsxo4-Pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 16+ messages in thread From: Andy Lutomirski @ 2016-09-12 16:56 UTC (permalink / raw) To: Kyle Huey Cc: Thomas Gleixner, John Stultz, Ingo Molnar, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Dmitry Vyukov, Vladimir Zapolskiy, Jiri Slaby, Andrey Ryabinin, Ben Segall, X86 On Sep 11, 2016 5:29 PM, "Kyle Huey" <me-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> wrote: > > rr (http://rr-project.org/), a userspace record-and-replay reverse- > execution debugger, would like to trap and emulate the CPUID instruction. > This would allow us to a) mask away certain hardware features that rr does > not support (e.g. RDRAND) and b) enable trace portability across machines > by providing constant results. > > 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. > This should have a cpufeature bit and show up in /proc/cpuinfo. That should be its own patch. You should explicitly check that, if the feature is set under Xen PV, then the MSR actually works as advertised. This may require talking to the Xen folks to make sure you're testing the right configuration. There should be a selftest. The behavior on fork() should be defined, and that behavior should be tested by the selftest. If this bit is preserved on fork(), then no_new_privs must be checked (or it must be cleared on "unsafe" exec, but that's nasty). --Andy ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <CALCETrVJAib918qn9CL4Tvyfok-8eAHimYDabXUF=1Nsxo4-Pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. [not found] ` <CALCETrVJAib918qn9CL4Tvyfok-8eAHimYDabXUF=1Nsxo4-Pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2016-09-12 17:18 ` Borislav Petkov 2016-09-12 17:56 ` Jann Horn 2016-09-14 6:13 ` Kyle Huey 2 siblings, 0 replies; 16+ messages in thread From: Borislav Petkov @ 2016-09-12 17:18 UTC (permalink / raw) To: Andy Lutomirski Cc: Kyle Huey, Thomas Gleixner, John Stultz, Ingo Molnar, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Dmitry Vyukov, Vladimir Zapolskiy, Jiri Slaby, Andrey Ryabinin, Ben Segall, mainta On Mon, Sep 12, 2016 at 09:56:11AM -0700, Andy Lutomirski wrote: > This should have a cpufeature bit and show up in /proc/cpuinfo. That Haha, test a cpufeature with a faulting CPUID :-) Yeah, we need a synthetic flag and query MSR_PLATFORM_INFO[31] which denotes whether the other MSR - MISC_FEATURES_ENABLES - is there and bit 0 is defined. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. [not found] ` <CALCETrVJAib918qn9CL4Tvyfok-8eAHimYDabXUF=1Nsxo4-Pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-09-12 17:18 ` Borislav Petkov @ 2016-09-12 17:56 ` Jann Horn 2016-09-12 21:07 ` Andy Lutomirski 2016-09-14 6:13 ` Kyle Huey 2 siblings, 1 reply; 16+ messages in thread From: Jann Horn @ 2016-09-12 17:56 UTC (permalink / raw) To: Andy Lutomirski Cc: Kyle Huey, Thomas Gleixner, John Stultz, Ingo Molnar, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Dmitry Vyukov, Vladimir Zapolskiy, Jiri Slaby, Andrey Ryabinin, Ben Segall, mainta On Mon, Sep 12, 2016 at 09:56:11AM -0700, Andy Lutomirski wrote: > On Sep 11, 2016 5:29 PM, "Kyle Huey" <me-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> wrote: > > > > rr (http://rr-project.org/), a userspace record-and-replay reverse- > > execution debugger, would like to trap and emulate the CPUID instruction. > > This would allow us to a) mask away certain hardware features that rr does > > not support (e.g. RDRAND) and b) enable trace portability across machines > > by providing constant results. > > > > 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. [...] > If this bit is preserved on fork(), then no_new_privs must be checked > (or it must be cleared on "unsafe" exec, but that's nasty). I think you mean "preserved on execve()"? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. 2016-09-12 17:56 ` Jann Horn @ 2016-09-12 21:07 ` Andy Lutomirski 0 siblings, 0 replies; 16+ messages in thread From: Andy Lutomirski @ 2016-09-12 21:07 UTC (permalink / raw) To: Jann Horn Cc: Thomas Gleixner, Ingo Molnar, John Stultz, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Linux API, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Vladimir Zapolskiy, Dmitry Vyukov, Jiri Slaby, Andrey Ryabinin, Ben Segall On Sep 12, 2016 10:57 AM, "Jann Horn" <jann@thejh.net> wrote: > > On Mon, Sep 12, 2016 at 09:56:11AM -0700, Andy Lutomirski wrote: > > On Sep 11, 2016 5:29 PM, "Kyle Huey" <me@kylehuey.com> wrote: > > > > > > rr (http://rr-project.org/), a userspace record-and-replay reverse- > > > execution debugger, would like to trap and emulate the CPUID instruction. > > > This would allow us to a) mask away certain hardware features that rr does > > > not support (e.g. RDRAND) and b) enable trace portability across machines > > > by providing constant results. > > > > > > 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. > [...] > > If this bit is preserved on fork(), then no_new_privs must be checked > > (or it must be cleared on "unsafe" exec, but that's nasty). > > I think you mean "preserved on execve()"? Indeed. So it should have defined and tested behavior on fork() and execve(). Maybe fork() should preserve the flag after all. --Andy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. [not found] ` <CALCETrVJAib918qn9CL4Tvyfok-8eAHimYDabXUF=1Nsxo4-Pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-09-12 17:18 ` Borislav Petkov 2016-09-12 17:56 ` Jann Horn @ 2016-09-14 6:13 ` Kyle Huey [not found] ` <CAP045ApLcQXZP44wry8mofLz2ip1vQbvpnhCjQZsT+JxXmVEzg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2 siblings, 1 reply; 16+ messages in thread From: Kyle Huey @ 2016-09-14 6:13 UTC (permalink / raw) To: Andy Lutomirski Cc: Thomas Gleixner, John Stultz, Ingo Molnar, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Dmitry Vyukov, Vladimir Zapolskiy, Jiri Slaby, Andrey Ryabinin, Ben Segall, X86 On Mon, Sep 12, 2016 at 9:56 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote: > You should explicitly check that, if the > feature is set under Xen PV, then the MSR actually works as > advertised. This may require talking to the Xen folks to make sure > you're testing the right configuration. This is interesting. When running under Xen PV the kernel is allowed to read the real value of MSR_PLATFORM_INFO and see that CPUID faulting is supported. But as you suggested, writing to MSR_MISC_FEATURES_ENABLES doesn't actually enable CPUID faulting, at least not in any way that works. It's not obvious to me how to test this, because when this feature works, CPUID only faults in userspace, not in the kernel. Is there existing code somewhere that runs tests like this in userspace? - Kyle ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <CAP045ApLcQXZP44wry8mofLz2ip1vQbvpnhCjQZsT+JxXmVEzg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. [not found] ` <CAP045ApLcQXZP44wry8mofLz2ip1vQbvpnhCjQZsT+JxXmVEzg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2016-09-14 18:52 ` Andy Lutomirski 2016-09-14 19:22 ` Andrew Cooper 2016-09-14 19:23 ` Boris Ostrovsky 0 siblings, 2 replies; 16+ messages in thread From: Andy Lutomirski @ 2016-09-14 18:52 UTC (permalink / raw) To: Kyle Huey, Andrew Cooper, Boris Ostrovsky Cc: Thomas Gleixner, John Stultz, Ingo Molnar, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Dmitry Vyukov, Vladimir Zapolskiy, Jiri Slaby, Andrey Ryabinin, Ben Segall, X86 On Tue, Sep 13, 2016 at 11:13 PM, Kyle Huey <me-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> wrote: > On Mon, Sep 12, 2016 at 9:56 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote: >> You should explicitly check that, if the >> feature is set under Xen PV, then the MSR actually works as >> advertised. This may require talking to the Xen folks to make sure >> you're testing the right configuration. > > This is interesting. When running under Xen PV the kernel is allowed > to read the real value of MSR_PLATFORM_INFO and see that CPUID > faulting is supported. But as you suggested, writing to > MSR_MISC_FEATURES_ENABLES doesn't actually enable CPUID faulting, at > least not in any way that works. > > It's not obvious to me how to test this, because when this feature > works, CPUID only faults in userspace, not in the kernel. Is there > existing code somewhere that runs tests like this in userspace? > Andrew, Boris: should we expect Xen PV to do anything sensible when we write to MSR_PLATFORM_INFO to turn on CPUID faulting? Should the Xen PV rdmsr hooks or perhaps the hypervisor mask out the feature if it isn't going to be supported? --Andy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. 2016-09-14 18:52 ` Andy Lutomirski @ 2016-09-14 19:22 ` Andrew Cooper 2016-09-14 19:23 ` Boris Ostrovsky 1 sibling, 0 replies; 16+ messages in thread From: Andrew Cooper @ 2016-09-14 19:22 UTC (permalink / raw) To: Andy Lutomirski, Kyle Huey, Boris Ostrovsky Cc: Thomas Gleixner, John Stultz, Ingo Molnar, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Dmitry Vyukov, Vladimir Zapolskiy, Jiri Slaby, Andrey Ryabinin, Ben Segall, X86 On 14/09/2016 19:52, Andy Lutomirski wrote: > On Tue, Sep 13, 2016 at 11:13 PM, Kyle Huey <me@kylehuey.com> wrote: >> On Mon, Sep 12, 2016 at 9:56 AM, Andy Lutomirski <luto@amacapital.net> wrote: >>> You should explicitly check that, if the >>> feature is set under Xen PV, then the MSR actually works as >>> advertised. This may require talking to the Xen folks to make sure >>> you're testing the right configuration. >> This is interesting. When running under Xen PV the kernel is allowed >> to read the real value of MSR_PLATFORM_INFO and see that CPUID >> faulting is supported. But as you suggested, writing to >> MSR_MISC_FEATURES_ENABLES doesn't actually enable CPUID faulting, at >> least not in any way that works. >> >> It's not obvious to me how to test this, because when this feature >> works, CPUID only faults in userspace, not in the kernel. Is there >> existing code somewhere that runs tests like this in userspace? >> > Andrew, Boris: should we expect Xen PV to do anything sensible when we > write to MSR_PLATFORM_INFO to turn on CPUID faulting? It will drop the write, so "No" is the answer to your question. > Should the Xen > PV rdmsr hooks or perhaps the hypervisor mask out the feature if it > isn't going to be supported? Yes. Sadly, whomever hacked these things together in the early days decided that the most simple solution to getting guests to boot was to allow all domains default-read access across the CPUID and MSR space, blacklisting out specific areas known to cause problems. I am in the process of sorting this stupidity^W "feature" out, but it is taking a while. What is the purpose of the check? I think it might be easiest to just do the native thing, and raise a bug in general against Xen citing "incorrect behaviour with respect to MSR_PLATFORM_INFO", get it fixed in stable trees and pretend that this breakage never happened. Short of having a small userspace stub check, I can't see any way to actually test this, and I certainly would prefer to avoid workarounds which end up like the OXSAVE detection, which is a complete disaster for both Linux and Xen. ~Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. 2016-09-14 18:52 ` Andy Lutomirski 2016-09-14 19:22 ` Andrew Cooper @ 2016-09-14 19:23 ` Boris Ostrovsky [not found] ` <e7b0b419-ed32-e35b-2f7b-eee71834fafd-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> 1 sibling, 1 reply; 16+ messages in thread From: Boris Ostrovsky @ 2016-09-14 19:23 UTC (permalink / raw) To: Andy Lutomirski, Kyle Huey, Andrew Cooper Cc: Thomas Gleixner, John Stultz, Ingo Molnar, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Dmitry Vyukov, Vladimir Zapolskiy, Jiri Slaby, Andrey Ryabinin, Ben Segall, X86 On 09/14/2016 02:52 PM, Andy Lutomirski wrote: > On Tue, Sep 13, 2016 at 11:13 PM, Kyle Huey <me@kylehuey.com> wrote: >> On Mon, Sep 12, 2016 at 9:56 AM, Andy Lutomirski <luto@amacapital.net> wrote: >>> You should explicitly check that, if the >>> feature is set under Xen PV, then the MSR actually works as >>> advertised. This may require talking to the Xen folks to make sure >>> you're testing the right configuration. >> This is interesting. When running under Xen PV the kernel is allowed >> to read the real value of MSR_PLATFORM_INFO and see that CPUID >> faulting is supported. But as you suggested, writing to >> MSR_MISC_FEATURES_ENABLES doesn't actually enable CPUID faulting, at >> least not in any way that works. >> >> It's not obvious to me how to test this, because when this feature >> works, CPUID only faults in userspace, not in the kernel. Is there >> existing code somewhere that runs tests like this in userspace? >> > Andrew, Boris: should we expect Xen PV to do anything sensible when we > write to MSR_PLATFORM_INFO to turn on CPUID faulting? Should the Xen > PV rdmsr hooks or perhaps the hypervisor mask out the feature if it > isn't going to be supported? The hypervisor uses CPUID faulting so we shouldn't advertise this feature to guests. -boris ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <e7b0b419-ed32-e35b-2f7b-eee71834fafd-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. [not found] ` <e7b0b419-ed32-e35b-2f7b-eee71834fafd-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> @ 2016-09-14 19:28 ` Andrew Cooper 2016-09-14 19:36 ` Andy Lutomirski 0 siblings, 1 reply; 16+ messages in thread From: Andrew Cooper @ 2016-09-14 19:28 UTC (permalink / raw) To: Boris Ostrovsky, Andy Lutomirski, Kyle Huey Cc: Thomas Gleixner, John Stultz, Ingo Molnar, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Dmitry Vyukov, Vladimir Zapolskiy, Jiri Slaby, Andrey Ryabinin, Ben Segall, X86 On 14/09/2016 20:23, Boris Ostrovsky wrote: > On 09/14/2016 02:52 PM, Andy Lutomirski wrote: >> On Tue, Sep 13, 2016 at 11:13 PM, Kyle Huey <me-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> wrote: >>> On Mon, Sep 12, 2016 at 9:56 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote: >>>> You should explicitly check that, if the >>>> feature is set under Xen PV, then the MSR actually works as >>>> advertised. This may require talking to the Xen folks to make sure >>>> you're testing the right configuration. >>> This is interesting. When running under Xen PV the kernel is allowed >>> to read the real value of MSR_PLATFORM_INFO and see that CPUID >>> faulting is supported. But as you suggested, writing to >>> MSR_MISC_FEATURES_ENABLES doesn't actually enable CPUID faulting, at >>> least not in any way that works. >>> >>> It's not obvious to me how to test this, because when this feature >>> works, CPUID only faults in userspace, not in the kernel. Is there >>> existing code somewhere that runs tests like this in userspace? >>> >> Andrew, Boris: should we expect Xen PV to do anything sensible when we >> write to MSR_PLATFORM_INFO to turn on CPUID faulting? Should the Xen >> PV rdmsr hooks or perhaps the hypervisor mask out the feature if it >> isn't going to be supported? > The hypervisor uses CPUID faulting so we shouldn't advertise this > feature to guests. In the case that the hardware has faulting, or for any HVM guest, the extra cost to making the feature available to the guest is a single conditional test in the cpuid path. This is about as close to zero as a feature gets. We really should be offering the feature to guests, and have it actually working. The issue here is that it is leaking when we weren't intending to offer it. ~Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. 2016-09-14 19:28 ` Andrew Cooper @ 2016-09-14 19:36 ` Andy Lutomirski [not found] ` <CALCETrUkmiwLDD0V-b5woDncOzdqZaS4Pg5EnneXD4ZEN7ttbg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 16+ messages in thread From: Andy Lutomirski @ 2016-09-14 19:36 UTC (permalink / raw) To: Andrew Cooper Cc: Boris Ostrovsky, Kyle Huey, Thomas Gleixner, John Stultz, Ingo Molnar, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Dmitry Vyukov, Vladimir Zapolskiy, Jiri Slaby, Andrey Ryabinin On Wed, Sep 14, 2016 at 12:28 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 14/09/2016 20:23, Boris Ostrovsky wrote: >> On 09/14/2016 02:52 PM, Andy Lutomirski wrote: >>> On Tue, Sep 13, 2016 at 11:13 PM, Kyle Huey <me@kylehuey.com> wrote: >>>> On Mon, Sep 12, 2016 at 9:56 AM, Andy Lutomirski <luto@amacapital.net> wrote: >>>>> You should explicitly check that, if the >>>>> feature is set under Xen PV, then the MSR actually works as >>>>> advertised. This may require talking to the Xen folks to make sure >>>>> you're testing the right configuration. >>>> This is interesting. When running under Xen PV the kernel is allowed >>>> to read the real value of MSR_PLATFORM_INFO and see that CPUID >>>> faulting is supported. But as you suggested, writing to >>>> MSR_MISC_FEATURES_ENABLES doesn't actually enable CPUID faulting, at >>>> least not in any way that works. >>>> >>>> It's not obvious to me how to test this, because when this feature >>>> works, CPUID only faults in userspace, not in the kernel. Is there >>>> existing code somewhere that runs tests like this in userspace? >>>> >>> Andrew, Boris: should we expect Xen PV to do anything sensible when we >>> write to MSR_PLATFORM_INFO to turn on CPUID faulting? Should the Xen >>> PV rdmsr hooks or perhaps the hypervisor mask out the feature if it >>> isn't going to be supported? >> The hypervisor uses CPUID faulting so we shouldn't advertise this >> feature to guests. > > In the case that the hardware has faulting, or for any HVM guest, the > extra cost to making the feature available to the guest is a single > conditional test in the cpuid path. This is about as close to zero as a > feature gets. We really should be offering the feature to guests, and > have it actually working. The issue here is that it is leaking when we > weren't intending to offer it. As long as Xen can fix this one way or the other in reasonably short order, I think I'm okay with having Linux incorrectly think it works on old Xen hypervisors. --Andy ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <CALCETrUkmiwLDD0V-b5woDncOzdqZaS4Pg5EnneXD4ZEN7ttbg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction. [not found] ` <CALCETrUkmiwLDD0V-b5woDncOzdqZaS4Pg5EnneXD4ZEN7ttbg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2016-09-14 19:42 ` Andrew Cooper 0 siblings, 0 replies; 16+ messages in thread From: Andrew Cooper @ 2016-09-14 19:42 UTC (permalink / raw) To: Andy Lutomirski Cc: Boris Ostrovsky, Kyle Huey, Thomas Gleixner, John Stultz, Ingo Molnar, Michal Hocko, Andrew Morton, Michael S. Tsirkin, Alexander Shishkin, Aravind Gopalakrishnan, Vlastimil Babka, Luis R. Rodriguez, Mateusz Guzik, Alex Thorlton, Rafael J. Wysocki, Dmitry Vyukov, Vladimir Zapolskiy, Jiri Slaby, Andrey Ryabinin On 14/09/2016 20:36, Andy Lutomirski wrote: > On Wed, Sep 14, 2016 at 12:28 PM, Andrew Cooper > <andrew.cooper3-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org> wrote: >> On 14/09/2016 20:23, Boris Ostrovsky wrote: >>> On 09/14/2016 02:52 PM, Andy Lutomirski wrote: >>>> On Tue, Sep 13, 2016 at 11:13 PM, Kyle Huey <me-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> wrote: >>>>> On Mon, Sep 12, 2016 at 9:56 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote: >>>>>> You should explicitly check that, if the >>>>>> feature is set under Xen PV, then the MSR actually works as >>>>>> advertised. This may require talking to the Xen folks to make sure >>>>>> you're testing the right configuration. >>>>> This is interesting. When running under Xen PV the kernel is allowed >>>>> to read the real value of MSR_PLATFORM_INFO and see that CPUID >>>>> faulting is supported. But as you suggested, writing to >>>>> MSR_MISC_FEATURES_ENABLES doesn't actually enable CPUID faulting, at >>>>> least not in any way that works. >>>>> >>>>> It's not obvious to me how to test this, because when this feature >>>>> works, CPUID only faults in userspace, not in the kernel. Is there >>>>> existing code somewhere that runs tests like this in userspace? >>>>> >>>> Andrew, Boris: should we expect Xen PV to do anything sensible when we >>>> write to MSR_PLATFORM_INFO to turn on CPUID faulting? Should the Xen >>>> PV rdmsr hooks or perhaps the hypervisor mask out the feature if it >>>> isn't going to be supported? >>> The hypervisor uses CPUID faulting so we shouldn't advertise this >>> feature to guests. >> In the case that the hardware has faulting, or for any HVM guest, the >> extra cost to making the feature available to the guest is a single >> conditional test in the cpuid path. This is about as close to zero as a >> feature gets. We really should be offering the feature to guests, and >> have it actually working. The issue here is that it is leaking when we >> weren't intending to offer it. > As long as Xen can fix this one way or the other in reasonably short > order, I think I'm okay with having Linux incorrectly think it works > on old Xen hypervisors. For now, unilaterally hiding CPUID faulting is easy, and simple to backport. Making the feature available for guests to use is slightly more tricky, as the toolstack still depends on not being faulted to construct HVM domains properly. This is the subject of my current CPUID project, which will result in dom0 being no more special than any other domain (in terms of hypervisor-side cpuid handling). ~Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-09-14 19:42 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-12 0:29 [PATCH] prctl,x86 Add PR_[GET|SET]_CPUID for controlling the CPUID instruction Kyle Huey [not found] ` <1473640169-24145-1-git-send-email-khuey-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> 2016-09-12 9:07 ` Borislav Petkov 2016-09-12 14:15 ` Kyle Huey 2016-09-12 14:34 ` Borislav Petkov 2016-09-13 18:42 ` Kyle Huey 2016-09-12 16:56 ` Andy Lutomirski [not found] ` <CALCETrVJAib918qn9CL4Tvyfok-8eAHimYDabXUF=1Nsxo4-Pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-09-12 17:18 ` Borislav Petkov 2016-09-12 17:56 ` Jann Horn 2016-09-12 21:07 ` Andy Lutomirski 2016-09-14 6:13 ` Kyle Huey [not found] ` <CAP045ApLcQXZP44wry8mofLz2ip1vQbvpnhCjQZsT+JxXmVEzg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-09-14 18:52 ` Andy Lutomirski 2016-09-14 19:22 ` Andrew Cooper 2016-09-14 19:23 ` Boris Ostrovsky [not found] ` <e7b0b419-ed32-e35b-2f7b-eee71834fafd-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> 2016-09-14 19:28 ` Andrew Cooper 2016-09-14 19:36 ` Andy Lutomirski [not found] ` <CALCETrUkmiwLDD0V-b5woDncOzdqZaS4Pg5EnneXD4ZEN7ttbg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-09-14 19:42 ` Andrew Cooper
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).