linux-kernel - Re: [PATCH 9/8] KVM: x86: limit MSR_IA32_SPEC

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: <ae7e1404-3225-2bc5-c26d-1baa0e00c05c@redhat.com>
Date:   Tue, 16 Jan 2018 13:59:57 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Eric Wheeler <kvm@...ts.ewheeler.net>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        rkrcmar@...hat.com, liran.alon@...cle.com, jmattson@...gle.com,
        aliguori@...zon.com, thomas.lendacky@....com, dwmw@...zon.co.uk,
        bp@...en8.de, x86@...nel.org
Subject: Re: [PATCH 9/8] KVM: x86: limit MSR_IA32_SPEC_CTRL access based on
 CPUID availability

On 16/01/2018 01:55, Eric Wheeler wrote:
> On Tue, 9 Jan 2018, Paolo Bonzini wrote:
>> MSR_IA32_SPEC_CTRL is not available unless CPU[7,0].EDX[26] is 1.
>> Check that against host CPUID or guest CPUID, respectively for
>> host-initiated and guest-initiated accesses.
> 
> Hi Radim, Paolo:
> 
> In porting this patch series to v4.14

Just don't apply this patch.

Paolo

> In file included from arch/x86/kvm/vmx.c:21:0:
> In function 'x86_feature_cpuid',
>     inlined from 'guest_cpuid_get_register' at arch/x86/kvm/cpuid.h:72:25,
>     inlined from 'vmx_get_msr' at arch/x86/kvm/cpuid.h:101:6:
> arch/x86/kvm/cpuid.h:64:232: error: call to '__compiletime_assert_64' 
> declared with attribute error: BUILD_BUG_ON failed: 
> reverse_cpuid[x86_leaf].function == 0
>   BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
>                                                                                                                                                                                                                                         
> ^
> In function 'x86_feature_cpuid',
>     inlined from 'guest_cpuid_get_register' at arch/x86/kvm/cpuid.h:72:25,
>     inlined from 'vmx_set_msr' at arch/x86/kvm/cpuid.h:101:6:
> arch/x86/kvm/cpuid.h:64:232: error: call to '__compiletime_assert_64' 
> declared with attribute error: BUILD_BUG_ON failed: 
> reverse_cpuid[x86_leaf].function == 0
>   BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
>                                                                                                                                                                                                                                         
> 
> I think this is caused by the following call stack for 
> X86_FEATURE_SPEC_CTRL, but if not please correct me here:
> 
> arch/x86/kvm/vmx.c:
> vmx_get_msr/vmx_set_msr()
> 	guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)
> 		guest_cpuid_get_register(vcpu, x86_feature); 
> 			x86_feature_cpuid(x86_feature);
> 				x86_feature_cpuid(unsigned x86_feature)
> 					BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
> 
> 
> It looks like I need to add something to reverse_cpuid[] but I'm not sure 
> what.  
> 
> Do you know what needs to be added here?
> 
> -Eric
> 
> --
> Eric Wheeler
> 
> 
> 
>>
>> Suggested-by: Jim Mattson <jmattson@...gle.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
>> ---
>> 	This is for after X86_FEATURE_SPEC_CTRL is added to Linux, but
>> 	I still wanted to ack Jim's improvement.
>>
>>  arch/x86/kvm/svm.c | 8 ++++++++
>>  arch/x86/kvm/vmx.c | 8 ++++++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 97126c2bd663..3a646580d7c5 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -3648,6 +3648,10 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>  		msr_info->data = svm->nested.vm_cr_msr;
>>  		break;
>>  	case MSR_IA32_SPEC_CTRL:
>> +		if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) ||
>> +	    	    (!msr_info->host_initiated &&
>> +       		     !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
>> +			return 1;
>>  		msr_info->data = svm->spec_ctrl;
>>  		break;
>>  	case MSR_IA32_UCODE_REV:
>> @@ -3806,6 +3810,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>  		vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
>>  		break;
>>  	case MSR_IA32_SPEC_CTRL:
>> +		if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) ||
>> +	    	    (!msr_info->host_initiated &&
>> +       		     !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
>> +			return 1;
>>  		svm->spec_ctrl = data;
>>  		break;
>>  	case MSR_IA32_APICBASE:
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 49b4a2d61603..42bc7ee293e4 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -3368,6 +3368,10 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>  		msr_info->data = guest_read_tsc(vcpu);
>>  		break;
>>  	case MSR_IA32_SPEC_CTRL:
>> +		if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) ||
>> +		    (!msr_info->host_initiated &&
>> +		     !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
>> +			return 1;
>>  		msr_info->data = to_vmx(vcpu)->spec_ctrl;
>>  		break;
>>  	case MSR_IA32_SYSENTER_CS:
>> @@ -3510,6 +3514,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>  		kvm_write_tsc(vcpu, msr_info);
>>  		break;
>>  	case MSR_IA32_SPEC_CTRL:
>> +		if (!static_cpu_has(X86_FEATURE_SPEC_CTRL) ||
>> +		    (!msr_info->host_initiated &&
>> +		     !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
>> +			return 1;
>>  		to_vmx(vcpu)->spec_ctrl = data;
>>  		break;
>>  	case MSR_IA32_CR_PAT:
>> -- 
>> 1.8.3.1
>>
>>