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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 30 Oct 2018 14:25:25 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     "Bae, Chang Seok" <chang.seok.bae@...el.com>
Cc:     Andrew Lutomirski <luto@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>, Andi Kleen <ak@...ux.intel.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/fsgsbase/64: Fix the base write helper functions

On Tue, Oct 30, 2018 at 10:28 AM Chang S. Bae <chang.seok.bae@...el.com> 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;
}