linux-kernel - Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS

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:   Mon, 12 Mar 2018 15:34:47 +0000
From:   Andy Lutomirski <luto@...nel.org>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm list <kvm@...r.kernel.org>, X86 ML <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>
Subject: Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

On Mon, Mar 12, 2018 at 2:02 PM, Vitaly Kuznetsov <vkuznets@...hat.com> wrote:
> vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so
> the context is pretty well defined. Read MSR_FS_BASE from
> current->thread.fsbase after calling save_fsgs() which takes care of
> X86_BUG_NULL_SEG case now and will do RD[FG,GS]BASE when FSGSBASE
> extensions are exposed to userspace (currently they are not).
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> ---
>  arch/x86/include/asm/processor.h |  3 +++
>  arch/x86/kernel/process_64.c     | 20 ++++++++++++++++++++
>  arch/x86/kvm/vmx.c               |  4 +++-
>  3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index b0ccd4847a58..006352b85ba3 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -410,6 +410,9 @@ DECLARE_INIT_PER_CPU(irq_stack_union);
>  DECLARE_PER_CPU(char *, irq_stack_ptr);
>  DECLARE_PER_CPU(unsigned int, irq_count);
>  extern asmlinkage void ignore_sysret(void);
> +
> +/* Save actual FS/GS selectors and bases to current->thread */
> +void save_current_fsgs(void);
>  #else  /* X86_64 */
>  #ifdef CONFIG_CC_STACKPROTECTOR
>  /*
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 9eb448c7859d..eb907fefe02e 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -205,6 +205,26 @@ static __always_inline void save_fsgs(struct task_struct *task)
>         save_base_legacy(task, task->thread.gsindex, GS);
>  }
>
> +/*
> + * Currently, the only way for processes to change their FS/GS base is to call
> + * ARCH_SET_FS/GS prctls and these reflect changes they make in task->thread.
> + * There are, however, additional considerations:
> + *
> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors zeroes
> + * the base and on some it doesn't, we need to check for that
> + * (see save_base_legacy()).
> + *
> + * When FSGSBASE extensions are enabled userspace processes will be able to
> + * change their FS/GS bases without kernel intervention. save_fsgs() will
> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
> + * instructions.
> + */

This is all a very complicated way to say "while a process is running,
current->thread.fsbase and current->thread.gsbase may not match the
corresponding CPU registers.  KVM wants an efficient way to save and
restore FSBASE and GSBASE."

And how about changing this to:

#if IS_ENABLED(CONFIG_KVM)
void save_fsgs_for_kvm(void)
{
    save_fsgs(current);
}
EXPORT_SYMBOL_GPL(save_fsgs_for_kvm);

--Andy