* [PATCH v3] x86/fsgsbase/64: Fix the base write helper functions
@ 2018-11-01 20:31 Chang S. Bae
2018-11-07 1:23 ` Andy Lutomirski
0 siblings, 1 reply; 3+ messages in thread
From: Chang S. Bae @ 2018-11-01 20:31 UTC (permalink / raw)
To: Ingo Molnar
Cc: Andy Lutomirski, Thomas Gleixner, H . Peter Anvin, Andi Kleen,
Dave Hansen, Ravi Shankar, Chang S . Bae, LKML
The helper functions that purport to write the base should just write it
only. It shouldn't have magic optimizations to change the index.
Make the index explicitly changed from the caller, instead of including
the code in the helpers.
v2: Fix further on the task write functions. Revert the changes on the
task read helpers.
v3: Fix putreg(). Edit the changelog.
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 ++++++++++++++++++++++--------------
arch/x86/kernel/ptrace.c | 8 +++---
2 files changed, 34 insertions(+), 22 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: {
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index ffae9b9740fd..e4ab1abca5b5 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -397,11 +397,11 @@ static int putreg(struct task_struct *child,
if (value >= TASK_SIZE_MAX)
return -EIO;
/*
- * When changing the FS base, use the same
- * mechanism as for do_arch_prctl_64().
+ * When changing the FS base, use do_arch_prctl_64()
+ * to set the index and the base.
*/
if (child->thread.fsbase != value)
- return x86_fsbase_write_task(child, value);
+ return do_arch_prctl_64(child, ARCH_SET_FS, value);
return 0;
case offsetof(struct user_regs_struct,gs_base):
/*
@@ -410,7 +410,7 @@ static int putreg(struct task_struct *child,
if (value >= TASK_SIZE_MAX)
return -EIO;
if (child->thread.gsbase != value)
- return x86_gsbase_write_task(child, value);
+ return do_arch_prctl_64(child, ARCH_SET_GS, value);
return 0;
#endif
}
--
2.19.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] x86/fsgsbase/64: Fix the base write helper functions
2018-11-01 20:31 [PATCH v3] x86/fsgsbase/64: Fix the base write helper functions Chang S. Bae
@ 2018-11-07 1:23 ` Andy Lutomirski
[not found] ` <94770318-72C4-442C-B2A3-7C4555884AA9@intel.com>
0 siblings, 1 reply; 3+ messages in thread
From: Andy Lutomirski @ 2018-11-07 1:23 UTC (permalink / raw)
To: Bae, Chang Seok
Cc: Ingo Molnar, Andrew Lutomirski, Thomas Gleixner, H. Peter Anvin,
Andi Kleen, Dave Hansen, Ravi V. Shankar, LKML
On Thu, Nov 1, 2018 at 1:32 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>
> The helper functions that purport to write the base should just write it
> only. It shouldn't have magic optimizations to change the index.
>
> Make the index explicitly changed from the caller, instead of including
> the code in the helpers.
>
> v2: Fix further on the task write functions. Revert the changes on the
> task read helpers.
>
> v3: Fix putreg(). Edit the changelog.
>
> 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 ++++++++++++++++++++++--------------
> arch/x86/kernel/ptrace.c | 8 +++---
> 2 files changed, 34 insertions(+), 22 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;
> }
Similar objection to last time. If you get rid of the task == current
check, you should WARN_ON_ONCE(task != current).
> @@ -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();
And you should drop the redundant ...write_task if task == current.
> 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: {
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index ffae9b9740fd..e4ab1abca5b5 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -397,11 +397,11 @@ static int putreg(struct task_struct *child,
> if (value >= TASK_SIZE_MAX)
> return -EIO;
> /*
> - * When changing the FS base, use the same
> - * mechanism as for do_arch_prctl_64().
> + * When changing the FS base, use do_arch_prctl_64()
> + * to set the index and the base.
to set the index to zero and to set the base as requested.
> */
> if (child->thread.fsbase != value)
> - return x86_fsbase_write_task(child, value);
> + return do_arch_prctl_64(child, ARCH_SET_FS, value);
> return 0;
> case offsetof(struct user_regs_struct,gs_base):
> /*
> @@ -410,7 +410,7 @@ static int putreg(struct task_struct *child,
> if (value >= TASK_SIZE_MAX)
> return -EIO;
> if (child->thread.gsbase != value)
> - return x86_gsbase_write_task(child, value);
> + return do_arch_prctl_64(child, ARCH_SET_GS, value);
> return 0;
> #endif
> }
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] x86/fsgsbase/64: Fix the base write helper functions
[not found] ` <94770318-72C4-442C-B2A3-7C4555884AA9@intel.com>
@ 2018-11-14 21:50 ` Bae, Chang Seok
0 siblings, 0 replies; 3+ messages in thread
From: Bae, Chang Seok @ 2018-11-14 21:50 UTC (permalink / raw)
To: LKML
On Nov 6, 2018, at 17:23, Andy Lutomirski <luto@kernel.org> wrote:
>
> On Thu, Nov 1, 2018 at 1:32 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>>
>> @@ -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;
>> }
>
> Similar objection to last time. If you get rid of the task == current
> check, you should WARN_ON_ONCE(task != current).
>
From your previous comments, I was a bit confused by a mixture of options.
It looks like you’d intended to suggest two different ways (by our own words):
1. The _write_task() functions must not be called on current. Then
there should be a warning, and you shouldn't call them on current.
2. The _write_task() functions should work correctly on current. In
this case, you shouldn't need to *also* called _write_cpu /
_write_cpu_inactive. You'll still need the special loadseg handling
in do_arch_prctl_64().
The option 1 looks to be simple and straight forward. So, let me go with that.
Nit. I think it should be WARN_ON_ONCE(task == current).
>> @@ -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();
>
> And you should drop the redundant ...write_task if task == current.
To avoid the redundancy, the sanity check for a base value can be moved out here.
>>
>> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
>> index ffae9b9740fd..e4ab1abca5b5 100644
>> --- a/arch/x86/kernel/ptrace.c
>> +++ b/arch/x86/kernel/ptrace.c
>> @@ -397,11 +397,11 @@ static int putreg(struct task_struct *child,
>> if (value >= TASK_SIZE_MAX)
>> return -EIO;
>> /*
>> - * When changing the FS base, use the same
>> - * mechanism as for do_arch_prctl_64().
>> + * When changing the FS base, use do_arch_prctl_64()
>> + * to set the index and the base.
>
> to set the index to zero and to set the base as requested.
Thanks for the edit.
Chang
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-11-14 21:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-01 20:31 [PATCH v3] x86/fsgsbase/64: Fix the base write helper functions Chang S. Bae
2018-11-07 1:23 ` Andy Lutomirski
[not found] ` <94770318-72C4-442C-B2A3-7C4555884AA9@intel.com>
2018-11-14 21:50 ` Bae, Chang Seok
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).