[PATCH v2] x86/fsgsbase/64: Fix the base write helper functions

[PATCH v2] x86/fsgsbase/64: Fix the base write helper functions

[Chang S. Bae]

Factor out the code to change index from the relavant helpers. Now,
the code is located in do_arch_prctl_64().

The helper functions that purport to write the base should just write it
only. It shouldn't have magic optimizations to change the index.

putreg() in ptrace does not write the current task, but a stopped task.

v2: Fix further on the task write functions. Revert the changes on the
task read helpers.

Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/kernel/process_64.c | 48 ++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 31b4755369f0..ad849ce9cb73 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -339,19 +339,11 @@ static unsigned long x86_fsgsbase_read_task(struct task_struct *task,
 
 void x86_fsbase_write_cpu(unsigned long fsbase)
 {
-	/*
-	 * Set the selector to 0 as a notion, that the segment base is
-	 * overwritten, which will be checked for skipping the segment load
-	 * during context switch.
-	 */
-	loadseg(FS, 0);
 	wrmsrl(MSR_FS_BASE, fsbase);
 }
 
 void x86_gsbase_write_cpu_inactive(unsigned long gsbase)
 {
-	/* Set the selector to 0 for the same reason as %fs above. */
-	loadseg(GS, 0);
 	wrmsrl(MSR_KERNEL_GS_BASE, gsbase);
 }
 
@@ -392,12 +384,7 @@ int x86_fsbase_write_task(struct task_struct *task, unsigned long fsbase)
 	if (unlikely(fsbase >= TASK_SIZE_MAX))
 		return -EPERM;
 
-	preempt_disable();
 	task->thread.fsbase = fsbase;
-	if (task == current)
-		x86_fsbase_write_cpu(fsbase);
-	task->thread.fsindex = 0;
-	preempt_enable();
 
 	return 0;
 }
@@ -407,12 +394,7 @@ int x86_gsbase_write_task(struct task_struct *task, unsigned long gsbase)
 	if (unlikely(gsbase >= TASK_SIZE_MAX))
 		return -EPERM;
 
-	preempt_disable();
 	task->thread.gsbase = gsbase;
-	if (task == current)
-		x86_gsbase_write_cpu_inactive(gsbase);
-	task->thread.gsindex = 0;
-	preempt_enable();
 
 	return 0;
 }
@@ -758,11 +740,41 @@ long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
 
 	switch (option) {
 	case ARCH_SET_GS: {
+		preempt_disable();
 		ret = x86_gsbase_write_task(task, arg2);
+		if (ret == 0) {
+			/*
+			 * ARCH_SET_GS has always overwritten the index
+			 * and the base. Zero is the most sensible value
+			 * to put in the index, and is the only value that
+			 * makes any sense if FSGSBASE is unavailable.
+			 */
+			if (task == current) {
+				loadseg(GS, 0);
+				x86_gsbase_write_cpu_inactive(arg2);
+			} else {
+				task->thread.gsindex = 0;
+			}
+		}
+		preempt_enable();
 		break;
 	}
 	case ARCH_SET_FS: {
+		preempt_disable();
 		ret = x86_fsbase_write_task(task, arg2);
+		if (ret == 0) {
+			/*
+			 * Set the selector to 0 for the same reason
+			 * as %gs above.
+			 */
+			if (task == current) {
+				loadseg(FS, 0);
+				x86_fsbase_write_cpu(arg2);
+			} else {
+				task->thread.fsindex = 0;
+			}
+		}
+		preempt_enable();
 		break;
 	}
 	case ARCH_GET_FS: {
-- 
2.19.1

Re: [PATCH v2] x86/fsgsbase/64: Fix the base write helper functions

[Ingo Molnar]

* Chang S. Bae <chang.seok.bae@intel.com> wrote:

> Factor out the code to change index from the relavant helpers. Now,
> the code is located in do_arch_prctl_64().

typo.

> 
> The helper functions that purport to write the base should just write it
> only. It shouldn't have magic optimizations to change the index.
> 
> putreg() in ptrace does not write the current task, but a stopped task.
> 
> v2: Fix further on the task write functions. Revert the changes on the
> task read helpers.
> 
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>

But most importantly, the changelog is confusing: please start the 
changelog with outlining the problem in an easy to digest fashion, 
starting with the highest level problem description and then successively 
iterating down towards the details.

The title says 'fix' so I suppose this fixes something, it doesn't just 
'factor out'?

Thanks,

	Ingo