linux-kernel - Re: [PATCH 1/2] KVM: MMU: fix ept=0/pte.u=0/pte.w=0/CR0.WP=0/CR4.SMEP=1/EFER.NX=0 combo

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:	Thu, 10 Mar 2016 11:09:27 +0100
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Xiao Guangrong <guangrong.xiao@...ux.intel.com>,
	linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc:	stable@...r.kernel.org, Andy Lutomirski <luto@...capital.net>
Subject: Re: [PATCH 1/2] KVM: MMU: fix
 ept=0/pte.u=0/pte.w=0/CR0.WP=0/CR4.SMEP=1/EFER.NX=0 combo



On 10/03/2016 09:27, Xiao Guangrong wrote:
>>
> 
>> +    if (!enable_ept) {
>> +        guest_efer |= EFER_NX;
>> +        ignore_bits |= EFER_NX;
> 
> Update ignore_bits is not necessary i think.

More precisely, ignore_bits is only needed if guest EFER.NX=0 and we're
not in this CR0.WP=1/CR4.SMEP=0 situation.  In theory you could have
guest EFER.NX=1 and host EFER.NX=0.

This is what I came up with (plus some comments :)):

	u64 guest_efer = vmx->vcpu.arch.efer;
	u64 ignore_bits = 0;

	if (!enable_ept) {
		if (boot_cpu_has(X86_FEATURE_SMEP))
			guest_efer |= EFER_NX;
		else if (!(guest_efer & EFER_NX))
			ignore_bits |= EFER_NX;
	}

>> -        guest_efer = vmx->vcpu.arch.efer;
>>           if (!(guest_efer & EFER_LMA))
>>               guest_efer &= ~EFER_LME;
>>           if (guest_efer != host_efer)
>>               add_atomic_switch_msr(vmx, MSR_EFER,
>>                             guest_efer, host_efer);
> 
> So, why not set EFER_NX (if !ept) just in this branch to make the fix
> more simpler?

I didn't like having

	guest_efer = vmx->vcpu.arch.efer;
	...
	if (!enable_ept)
		guest_efer |= EFER_NX;
	guest_efer &= ~ignore_bits;
	guest_efer |= host_efer & ignore_bits;
	...
	if (...) {
		guest_efer = vmx->vcpu.arch.efer;
		if (!enable_ept)
			guest_efer |= EFER_NX;
		...
	}

My patch is bigger but the resulting code is smaller and easier to follow:

	guest_efer = vmx->vcpu.arch.efer;
	if (!enable_ept)
		guest_efer |= EFER_NX;
	...
	if (...) {
		...
	} else {
		guest_efer &= ~ignore_bits;
		guest_efer |= host_efer & ignore_bits;
	}

Paolo