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
| ||
|
Date: Tue, 6 Nov 2018 17:23:26 -0800 From: Andy Lutomirski <luto@...nel.org> To: "Bae, Chang Seok" <chang.seok.bae@...el.com> Cc: Ingo Molnar <mingo@...nel.org>, Andrew Lutomirski <luto@...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 v3] x86/fsgsbase/64: Fix the base write helper functions On Thu, Nov 1, 2018 at 1:32 PM Chang S. Bae <chang.seok.bae@...el.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@...nel.org> > Signed-off-by: Chang S. Bae <chang.seok.bae@...el.com> > Cc: Ingo Molnar <mingo@...nel.org> > Cc: Thomas Gleixner <tglx@...utronix.de> > Cc: H. Peter Anvin <hpa@...or.com> > Cc: Andi Kleen <ak@...ux.intel.com> > Cc: Dave Hansen <dave.hansen@...ux.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 >
Powered by blists - more mailing lists