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: <4A1B24BB-E351-4F98-8A55-F2FB9F45BBF8@nutanix.com>
Date: Thu, 27 Feb 2025 19:40:22 +0000
From: Jon Kohler <jon@...anix.com>
To: Sean Christopherson <seanjc@...gle.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 Feb 27, 2025, at 2:34 PM, Sean Christopherson <seanjc@...gle.com> wrote:
> 
> 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

Silly IT !

> 
>>> |-------------------------------------------------------------------!
>>> 
>>> 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.

Ok no problem at all, happy to rebase on top of this when it lands.

Thanks for the suggestions on the diff, will give it a poke

> 
>> 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
> -- 
>