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

[Andy Lutomirski]

On Tue, Oct 30, 2018 at 10:28 AM Chang S. Bae <chang.seok.bae@xxxxxxxxx> wrote:
>
> Factor out the code to change index from x86_fsbase_write_cpu() and
> x86_gsbase_write_cpu_inactive(). Now the code is located in
> do_arch_prctl_64().
>

> @@ -359,9 +351,7 @@ unsigned long x86_fsbase_read_task(struct task_struct *task)
>  {
>         unsigned long fsbase;
>
> -       if (task == current)
> -               fsbase = x86_fsbase_read_cpu();
> -       else if (task->thread.fsindex == 0)
> +       if (task->thread.fsindex == 0)

I'm okay with this change but, if you do it, please add:

WARN_ON_ONCE(task == current);

and make it be in a separate patch.

>                 fsbase = task->thread.fsbase;
>         else
>                 fsbase = x86_fsgsbase_read_task(task, task->thread.fsindex);
> @@ -373,9 +363,7 @@ unsigned long x86_gsbase_read_task(struct task_struct *task)
>  {
>         unsigned long gsbase;
>
> -       if (task == current)
> -               gsbase = x86_gsbase_read_cpu_inactive();
> -       else if (task->thread.gsindex == 0)
> +       if (task->thread.gsindex == 0)

Same here.

>                 gsbase = task->thread.gsbase;
>         else
>                 gsbase = x86_fsgsbase_read_task(task, task->thread.gsindex);
> @@ -392,12 +380,8 @@ 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;

I'm confused.  You're still setting fsindex to zero here.

> -       preempt_enable();
>
>         return 0;
>  }
> @@ -407,12 +391,8 @@ 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;

Same here.

> @@ -757,22 +737,53 @@ long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
>         int ret = 0;
>
>         switch (option) {
> -       case ARCH_SET_GS: {
> -               ret = x86_gsbase_write_task(task, arg2);
> -               break;
> -       }
>         case ARCH_SET_FS: {
> +               preempt_disable();
>                 ret = x86_fsbase_write_task(task, arg2);
> +               if (task == current && ret == 0) {
> +                       /*
> +                        * Set the selector to 0, implying that the base is
> +                        * overwritten. The index value will be checked
> +                        * during context switch for skipping segment reload.
> +                        */
> +                       loadseg(FS, 0);
> +                       x86_fsbase_write_cpu(arg2);
> +               }
> +               preempt_enable();
> +               break;
> +       }
> +       case ARCH_SET_GS: {
> +               preempt_disable();
> +               ret = x86_gsbase_write_task(task, arg2);

This would trip the warning that I'm asking you to add.  This makes me
think you should not make that change.

> +               if (task == current && ret == 0) {
> +                       /*
> +                        * Set the selector to 0 for the same reason
> +                        * as %fs above.
> +                        */
> +                       loadseg(GS, 0);
> +                       x86_gsbase_write_cpu_inactive(arg2);
> +               }

Why only if task == current?

I think this code should be:

ret = x86_gsbase_write_task(task, arg);
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);
  else
    task->thread.gsindex = 0;
}