linux-kernel - Re: [PATCH] x86: add additional EPT bit definitions

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: <Ye7gykcvjig7aPNM@google.com>
Date:   Mon, 24 Jan 2022 17:24:26 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Ayush Ranjan <ayushranjan@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ben Gardon <bgardon@...gle.com>,
        Jim Mattson <jmattson@...gle.com>,
        Andrei Vagin <avagin@...il.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Michael Pratt <mpratt@...gle.com>
Subject: Re: [PATCH] x86: add additional EPT bit definitions

On Sun, Jan 23, 2022, Ayush Ranjan wrote:
> From: Michael Pratt <mpratt@...gle.com>
> 
> Used in gvisor for EPT support.

As you may have surmised from the other patch, the changelogs from patches carried
in our internal kernels rarely meet the criteria for acceptance upstream.  E.g. this
doesn't provide sufficient justification since there's obviously no in-kernel gvisor
that's consuming this.

Submitting patches that we carry internally is perfectly ok, but there needs to be
sufficient justfication, and the patch needs to follow the rules laid out by
Documentation/process/submitting-patches.rst.

> Tested: Builds cleanly
> Signed-off-by: Ayush Ranjan <ayushranjan@...gle.com>
> Signed-off-by: Michael Pratt <mpratt@...gle.com>
> ---
>  arch/x86/include/asm/vmx.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 0ffaa3156a4e..c77ad687cdf7 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -496,7 +496,9 @@ enum vmcs_field {
>  #define VMX_EPT_WRITABLE_MASK			0x2ull
>  #define VMX_EPT_EXECUTABLE_MASK			0x4ull
>  #define VMX_EPT_IPAT_BIT    			(1ull << 6)
> -#define VMX_EPT_ACCESS_BIT			(1ull << 8)
> +#define VMX_EPT_PSE_BIT				(1ull << 7)

I'm not a fan of "PSE", it's unnecessarily terse and "PSE" has different meaning
in IA32 paging.  VMX_EPT_PAGE_SIZE_BIT would be choice.

As for justification, something that has been mentioned once or thrice is the lack
of build-time assertions that the PT_* bits in mmu.h that are reused for EPT entries
do indeed match the EPT definitions.  I can throw together a patch/series to add
that and do the below cleanup.

> +#define VMX_EPT_ACCESS_SHIFT			8

I'd prefer we don't define the "shifts" for EPT (or PTE) bits, they really shouldn't
be used as doing things like test_and_clear_bit() via a shift value can generate
unnecessary lock instructions.  arch/x86/kvm/mmu.h could use a bit of spring cleaning
in this regard.

> +#define VMX_EPT_ACCESS_BIT			(1ull << VMX_EPT_ACCESS_SHIFT)
>  #define VMX_EPT_DIRTY_BIT			(1ull << 9)
>  #define VMX_EPT_RWX_MASK                        (VMX_EPT_READABLE_MASK |       \
>  						 VMX_EPT_WRITABLE_MASK |       \
> -- 
> 2.35.0.rc0.227.g00780c9af4-goog
>