| 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
| ||
|
Message-ID: <CALCETrXCOgBk7oKrE_CAspA6eqC19CfXckgeCcWUjREVHRHRPg@mail.gmail.com>
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