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 for Android: free password hash cracker in your pocket

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

Message-ID: <362EF130-C3AF-4C3B-883E-48DCBC5EA5C3@intel.com>
Date:   Wed, 31 Oct 2018 18:50:28 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     Andy Lutomirski <luto@...nel.org>
CC:     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>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/fsgsbase/64: Fix the base write helper functions



> On Oct 30, 2018, at 14:25, Andy Lutomirski <luto@...nel.org> wrote:
> 
> 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.
> 

Okay. Let me unchange those read functions, as it does what is claimed to do.

>>                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.
> 

These task write functions are getting thiner, although I agree
the index update should move out.

> 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;
> }

Thank you for the clarification. I think writing base  should come right after loadseg().

Chang