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

[Andy Lutomirski]

On Thu, Nov 1, 2018 at 1:32 PM Chang S. Bae <chang.seok.bae@xxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Chang S. Bae <chang.seok.bae@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> ---
>  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
>