linux-kernel - Re: [PATCH v2 2/2] KVM: nVMX: Decouple EPT RWX bits from EPT Violation protection bits

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: <Z8C-PRStaoikVlGx@google.com>
Date: Thu, 27 Feb 2025 19:34:21 +0000
From: Sean Christopherson <seanjc@...gle.com>
To: Jon Kohler <jon@...anix.com>
Cc: Nikolay Borisov <nik.borisov@...e.com>, Paolo Bonzini <pbonzini@...hat.com>, 
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] KVM: nVMX: Decouple EPT RWX bits from EPT
 Violation protection bits

On Thu, Feb 27, 2025, Jon Kohler wrote:
> > On Feb 27, 2025, at 1:52 AM, Nikolay Borisov <nik.borisov@...e.com> wrote:
> > 
> > !-------------------------------------------------------------------|
> > CAUTION: External Email

Noted.  :-D

> > |-------------------------------------------------------------------!
> > 
> > On 27.02.25 г. 2:07 ч., Sean Christopherson wrote:
> >> Define independent macros for the RWX protection bits that are enumerated
> >> via EXIT_QUALIFICATION for EPT Violations, and tie them to the RWX bits in
> >> EPT entries via compile-time asserts.  Piggybacking the EPTE defines works
> >> for now, but it creates holes in the EPT_VIOLATION_xxx macros and will
> >> cause headaches if/when KVM emulates Mode-Based Execution (MBEC), or any
> >> other features that introduces additional protection information.
> >> Opportunistically rename EPT_VIOLATION_RWX_MASK to EPT_VIOLATION_PROT_MASK
> >> so that it doesn't become stale if/when MBEC support is added.
> >> No functional change intended.
> >> Cc: Jon Kohler <jon@...anix.com>
> >> Cc: Nikolay Borisov <nik.borisov@...e.com>
> >> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> > 
> > Reviewed-by: Nikolay Borisov <nik.borisov@...e.com>
> 
> LGTM, but any chance we could hold this until I get the MBEC RFC out? 

No?  It's definitely landing before the MBEC support, and IOM it works quite nicely
with the MBEC support (my diff at the bottom).  I don't see any reason to delay
or change this cleanup.

> My apologies on the delay, I caught a terrible chest cold after we met about
> it, followed by a secondary case of strep!

Ow.  Don't rush on behalf of upstream, KVM has lived without MBEC for a long time,
it's not going anywhere.o

---
 arch/x86/include/asm/vmx.h     | 4 +++-
 arch/x86/kvm/mmu/paging_tmpl.h | 9 +++++++--
 arch/x86/kvm/vmx/vmx.c         | 7 +++++++
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index d7ab0ad63be6..61e31e915e46 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -587,9 +587,11 @@ enum vm_entry_failure_code {
 #define EPT_VIOLATION_PROT_READ		BIT(3)
 #define EPT_VIOLATION_PROT_WRITE	BIT(4)
 #define EPT_VIOLATION_PROT_EXEC		BIT(5)
+#define EPT_VIOLATION_PROT_USER_EXEC	BIT(6)
 #define EPT_VIOLATION_PROT_MASK		(EPT_VIOLATION_PROT_READ  | \
 					 EPT_VIOLATION_PROT_WRITE | \
-					 EPT_VIOLATION_PROT_EXEC)
+					 EPT_VIOLATION_PROT_EXEC  | \
+					 EPT_VIOLATION_PROT_USER_EXEC)
 #define EPT_VIOLATION_GVA_IS_VALID	BIT(7)
 #define EPT_VIOLATION_GVA_TRANSLATED	BIT(8)
 
diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index 68e323568e95..ede8207bf4d7 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -181,8 +181,9 @@ static inline unsigned FNAME(gpte_access)(u64 gpte)
 	unsigned access;
 #if PTTYPE == PTTYPE_EPT
 	access = ((gpte & VMX_EPT_WRITABLE_MASK) ? ACC_WRITE_MASK : 0) |
-		((gpte & VMX_EPT_EXECUTABLE_MASK) ? ACC_EXEC_MASK : 0) |
-		((gpte & VMX_EPT_READABLE_MASK) ? ACC_USER_MASK : 0);
+		 ((gpte & VMX_EPT_EXECUTABLE_MASK) ? ACC_EXEC_MASK : 0) |
+		 ((gpte & VMX_EPT_USER_EXECUTABLE_MASK) ? ACC_USER_EXEC_MASK : 0) |
+		 ((gpte & VMX_EPT_READABLE_MASK) ? ACC_USER_MASK : 0);
 #else
 	BUILD_BUG_ON(ACC_EXEC_MASK != PT_PRESENT_MASK);
 	BUILD_BUG_ON(ACC_EXEC_MASK != 1);
@@ -511,6 +512,10 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
 		 * ACC_*_MASK flags!
 		 */
 		walker->fault.exit_qualification |= EPT_VIOLATION_RWX_TO_PROT(pte_access);
+		/* This is also wrong.*/
+		if (vcpu->arch.pt_guest_exec_control &&
+		    (pte_access & VMX_EPT_USER_EXECUTABLE_MASK))
+			walker->fault.exit_qualification |= EPT_VIOLATION_PROT_USER_EXEC;
 	}
 #endif
 	walker->fault.address = addr;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 0db64f4adf2a..4684647ef063 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5806,6 +5806,13 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu)
 
 	exit_qualification = vmx_get_exit_qual(vcpu);
 
+	/*
+	 * The USER_EXEC flag is undefined if MBEC is disabled.
+	 * Note, this is wrong, MBEC should be a property of the MMU.
+	 */
+	if (!vcpu->arch.pt_guest_exec_control)
+		exit_qualification &= ~EPT_VIOLATION_PROT_USER_EXEC;
+
 	/*
 	 * EPT violation happened while executing iret from NMI,
 	 * "blocked by NMI" bit has to be set before next VM entry.

base-commit: 67983df09fc3f96d0d6107fe1a99d29460bab481
--