linux-kernel - Re: [PATCH 2/2] kvm/x86: don't expose MSR_IA32_UMWAIT

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: <0c1a0c81bbdcfaf4ae9af545f4a38439b1a56d11.camel@redhat.com>
Date:   Wed, 20 May 2020 19:56:43 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] kvm/x86: don't expose MSR_IA32_UMWAIT_CONTROL
 unconditionally

On Wed, 2020-05-20 at 18:33 +0200, Vitaly Kuznetsov wrote:
> Maxim Levitsky <mlevitsk@...hat.com> writes:
> 
> > This msr is only available when the host supports WAITPKG feature.
> > 
> > This breaks a nested guest, if the L1 hypervisor is set to ignore
> > unknown msrs, because the only other safety check that the
> > kernel does is that it attempts to read the msr and
> > rejects it if it gets an exception.
> > 
> > Fixes: 6e3ba4abce KVM: vmx: Emulate MSR IA32_UMWAIT_CONTROL
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> > ---
> >  arch/x86/kvm/x86.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index fe3a24fd6b263..9c507b32b1b77 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5314,6 +5314,10 @@ static void kvm_init_msr_list(void)
> >  			if (msrs_to_save_all[i] - MSR_ARCH_PERFMON_EVENTSEL0 >=
> >  			    min(INTEL_PMC_MAX_GENERIC, x86_pmu.num_counters_gp))
> >  				continue;
> > +			break;
> > +		case MSR_IA32_UMWAIT_CONTROL:
> > +			if (!kvm_cpu_cap_has(X86_FEATURE_WAITPKG))
> > +				continue;
> 
> I'm probably missing something but (if I understand correctly) the only
> effect of dropping MSR_IA32_UMWAIT_CONTROL from msrs_to_save would be
> that KVM userspace won't see it in e.g. KVM_GET_MSR_INDEX_LIST. But why
> is this causing an issue? I see both vmx_get_msr()/vmx_set_msr() have
> 'host_initiated' check:
> 
>        case MSR_IA32_UMWAIT_CONTROL:
>                 if (!msr_info->host_initiated && !vmx_has_waitpkg(vmx))
>                         return 1;

Here it fails like that:

1. KVM_GET_MSR_INDEX_LIST returns this msrs, and qemu notes that
   it is supported in 'has_msr_umwait' global var

2. Qemu does kvm_arch_get/put_registers->kvm_get/put_msrs->ioctl(KVM_GET_MSRS)
   and while doing this it adds MSR_IA32_UMWAIT_CONTROL to that msr list.
   That reaches 'svm_get_msr', and this one knows nothing about MSR_IA32_UMWAIT_CONTROL.

So the difference here is that vmx_get_msr not called at all.
I can add this msr to svm_get_msr instead but that feels wrong since this feature
is not yet supported on AMD.
When AMD adds support for this feature, then the VMX specific code can be moved to
kvm_get_msr_common I guess.

> 
> so KVM userspace should be able to read/write this MSR even when there's
> no hardware support for it. Or who's trying to read/write it?
> 
> Also, kvm_cpu_cap_has() check is not equal to vmx_has_waitpkg() which
> checks secondary execution controls.

I was afraid that something like that will happen, but in this particular
case we can only check CPUID support and if supported, the then it means
we are dealing with intel system and thus vmx_get_msr will be called and
ignore that msr.

Calling vmx_has_waitpkg from the common code doesn't seem right, and besides,
it checks the secondary controls which are set by the host and can change,
at least in theory during runtime (I don't know if KVM does this).

Note that if I now understand correctly, the 'host_initiated' means that MSR read/write
is done by the host itself and not on behalf of the guest.

Best regards,
	Maxim Levitsky

> 
> >  		default:
> >  			break;
> >  		}