linux-kernel - Re: [PATCH v3 8/8] KVM:X86: Add user-space read/write interface for CET MSRs.

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]

Message-ID: <20190308173044.GD2528@linux.intel.com>
Date:   Fri, 8 Mar 2019 09:30:45 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Yang Weijiang <weijiang.yang@...el.com>
Cc:     pbonzini@...hat.com, rkrcmar@...hat.com, jmattson@...gle.com,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org, mst@...hat.com,
        yu-cheng.yu@...el.com
Subject: Re: [PATCH v3 8/8] KVM:X86: Add user-space read/write interface for
 CET MSRs.

On Thu, Feb 28, 2019 at 05:41:52PM +0800, Yang Weijiang wrote:
> On Thu, Feb 28, 2019 at 08:32:49AM -0800, Sean Christopherson wrote:
> > On Mon, Feb 25, 2019 at 09:27:16PM +0800, Yang Weijiang wrote:

...

> > >  static int __msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs *msrs,
> > > -		    struct kvm_msr_entry *entries,
> > > +		    struct kvm_msr_entry *entries, bool read,
> > >  		    int (*do_msr)(struct kvm_vcpu *vcpu,
> > >  				  unsigned index, u64 *data))
> > >  {
> > >  	int i;
> > >  
> > > -	for (i = 0; i < msrs->nmsrs; ++i)
> > > +	for (i = 0; i < msrs->nmsrs; ++i) {
> > > +		/* If it comes to CET related MSRs, read them together. */
> > > +		if (entries[i].index == MSR_IA32_U_CET) {
> > > +			i += do_cet_msrs(vcpu, i, entries, read) - 1;
> > 
> > This wrong, not to mention horribly buggy.  The correct way to handle
> > MSRs that are switched via the VMCS is to read/write the VMCS in
> > vmx_{get,set}_msr() as needed, e.g. vmcs_writel(GUEST_GS_BASE, data).
> >
> The code is barely for migration purpose since they're passed through
> to guest, I have no intent to expose them just like normal MSRs.
> I double checked the spec.:
> MSR_IA32_U_CET                  0x6a0
> MSR_IA32_PL0_SSP                0x6a4
> MSR_IA32_PL3_SSP                0x6a7
> should come from xsave area.
> 
> MSR_IA32_S_CET                  0x6a2
> MSR_IA32_INTR_SSP_TABL          0x6a8
> should come from VMCS guest fields.
> 
> for the former, they're stored in guest fpu area, need
> to restore them with xrstors temporarily before read, while the later is stored in
> VMCS guest fields, I can read them out via vmcs_read() directly.
> 
> I'd like to read them out as a whole(all or nothing) due to cost induced 
> by xsaves/xrstors, in Qemu I put these MSRs as sequential fields.
> 
> what do you think of it?

It doesn't need to be all or nothing, it should be simple enough to set
a flag when we load guest state and use it to skip reloading guest state
and to load host state before returning.  I think the attached patch
does the trick.

View attachment "0001-KVM-x86-load-guest-fpu-state-when-accessing-MSRs-man.patch" of type "text/x-diff" (3740 bytes)