Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH v2] KVM: x86: do not read FS/GS base MSRs when saving them
From: Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <6d10cdd5-e8d8-f5ed-e039-c71df9d0ea9e@redhat.com>
Date: Wed, 3 Jan 2018 16:16:41 -0800
Cc: Andy Lutomirski <luto@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        kvm list <kvm@vger.kernel.org>
Content-Transfer-Encoding: 8BIT
Message-Id: <598B48F9-4552-4442-82D2-60A2674C6241@amacapital.net>
References: <1514901591-12866-1-git-send-email-pbonzini@redhat.com> <CALCETrXJdwkELu81wj2rXdKcK-TXPmBO1y8hDxi7B1zkSa=WgQ@mail.gmail.com> <6d10cdd5-e8d8-f5ed-e039-c71df9d0ea9e@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org


> On Jan 3, 2018, at 3:42 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> On 03/01/2018 23:20, Andy Lutomirski wrote:
>>> On Jan 2, 2018, at 5:59 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> 
>>> The FS and userspace GS bases are available in current->thread, while the
>>> kernel GS base is a percpu variable.  Skip the expensive rdmsr and just
>>> get the values from memory.
>> 
>> That fsbase change is wrong: thread->fsbase is not guaranteed to be
>> correct for current.
> 
> Note that the value I'm storing in HOST_FS_BASE and HOST_GS_BASE is only
> used if FS/GS selector is zero.  If FS/GS selector is not zero, it is
> not used.  Does that avoid this issue?
> 

I'm not convinced that this is correct.  It's not obviously a security problem in the context of KVM, but a lot of state can leak this way.

My general preference would be to make the code obviously fully reload the host state.

I'll try to look at it for real soon.

> Certainly worth a comment or clearer code though.
> 
>>> +#ifdef CONFIG_X86_64
>>> +/* Provide the current kernel GS base.  */
>>> +static inline void *get_current_kernel_gs_base(void)
>>> +{
>>> +    return this_cpu_ptr(irq_stack_union.gs_base);
>>> +}
>>> +#endif
>> 
>> This is an awful name because MSR_KERNEL_GS_BASE means the user gs
>> base.  How about calling it something like
>> get_this_cpu_kernelmode_gs_base() or similar?
> 
> True, I'll adopt your name.
> 
> Paolo
> 
>>> #ifdef CONFIG_X86_64
>>> -    vmcs_writel(HOST_FS_BASE, read_msr(MSR_FS_BASE));
>>> -    vmcs_writel(HOST_GS_BASE, read_msr(MSR_GS_BASE));
>>> +    vmcs_writel(HOST_FS_BASE, current->thread.fsbase);
>> 
>> That's wrong.  thread->fsbase isn't kept up to date while the thread
>> is running.  You could potentially try to expose an interface to get
>> save_base_legacy() called to update it.
>> 
>