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

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

[Kyle Huey]

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

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

[Kyle Huey]

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

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

[Borislav Petkov]

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.

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

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

[Borislav Petkov]

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

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

[Kyle Huey]

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

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

[Kyle Huey]

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

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

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

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

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

[Kyle Huey]

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

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

[Kyle Huey]

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

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

[Andy Lutomirski]

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

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

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

[Andy Lutomirski]

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

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

[Borislav Petkov]

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

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

[Borislav Petkov]

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

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

[Jann Horn]

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()"?

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

[Jann Horn]

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()"?

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

[Andy Lutomirski]

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

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

[Andy Lutomirski]

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

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

[Kyle Huey]

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?

- Kyle

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

[Kyle Huey]

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

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

[Andy Lutomirski]

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?

--Andy

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

[Andy Lutomirski]

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

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

[Andrew Cooper]

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

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

[Andrew Cooper]

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

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

[Boris Ostrovsky]

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

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

[Boris Ostrovsky]

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

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

[Andrew Cooper]

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.

~Andrew

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

[Andrew Cooper]

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

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

[Andy Lutomirski]

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

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

[Andy Lutomirski]

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

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

[Andrew Cooper]

On 14/09/2016 20:36, Andy Lutomirski wrote:
> 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.

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

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

[Andrew Cooper]

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

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

[Andrew Cooper]

On 14/09/2016 20:36, Andy Lutomirski wrote:
> 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.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Andi Kleen]

Kyle Huey <me-OhBmq/TcCDJWk0Htik3J/w@public.gmane.org> writes:

> 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 will explode when an interrupt handler executes cpuid, won't it?

The cpuid char driver does this, other code may too.

You probably would need to protect these CPUIDs with an exception
handler that temporarily disables this bit and retries.

-Andi

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

[Henrique de Moraes Holschuh]

On Mon, 12 Sep 2016, Andi Kleen wrote:
> The cpuid char driver does this, other code may too.

Such as anything that calls sync_core().

That includes processor hotplug paths, and who knows what else...

> You probably would need to protect these CPUIDs with an exception
> handler that temporarily disables this bit and retries.

Better time that MSR access first.  Hopefully, it is fast enough for
that.

-- 
  Henrique Holschuh