linux-kernel - Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

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:   Fri, 22 Jul 2022 19:00:17 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>, dave@...1.net,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>, Kai Huang <kai.huang@...el.com>,
        Haitao Huang <haitao.huang@...ux.intel.com>, x86@...nel.org,
        linux-sgx@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit
 Notification

On Wed, Jul 20, 2022, Dave Hansen wrote:
> On 7/20/22 12:49, Sean Christopherson wrote:
> > On Wed, Jul 20, 2022, Dave Hansen wrote:
> >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> >> index 0c1ba6aa0765..96a73b5b4369 100644
> >> --- a/arch/x86/kvm/cpuid.c
> >> +++ b/arch/x86/kvm/cpuid.c
> >> @@ -1022,9 +1022,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> >>  		 * userspace.  ATTRIBUTES.XFRM is not adjusted as userspace is
> >>  		 * expected to derive it from supported XCR0.
> >>  		 */
> >> -		entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT |
> >> -			      SGX_ATTR_PROVISIONKEY | SGX_ATTR_EINITTOKENKEY |
> >> -			      SGX_ATTR_KSS;
> >> +		entry->eax &= SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK;
> > 
> > It may seem like a maintenance burdern, and it is to some extent, but I think it's
> > better for KVM to have to explicitly "enable" each flag.  There is no guarantee
> > that a new feature will not require additional KVM enabling, i.e. we want the pain
> > of having to manually update KVM so that we get "feature X isn't virtualized"
> > complaints and not "I upgraded my kernel and my enclaves broke" bug reports.
> > 
> > I don't think it's likely that attribute-based features will require additional
> > enabling since there aren't any virtualization controls for the ENCLU side of
> > things (ENCLU is effectively disabled by blocking ENCLS[ECREATE]), but updating
> > KVM isn't particularly difficult so I'd rather be paranoid.
> 
> How about something where KVM gets to keep a discrete mask, but where
> it's at least defined next to the attributes, something like:
> 
> /*
>  * These attributes will be advertised to KVM guests as being
>  * available.  This includes privileged attributes.  Only add
>  * to this list when host-side KVM does not require additional
>  * enabling for the attribute.
>  */
> #define SGX_ATTR_KVM_MASK       (SGX_ATTR_DEBUG         | \
>                                  SGX_ATTR_MODE64BIT     | \
>                                  SGX_ATTR_PROVISIONKEY  | \
>                                  SGX_ATTR_EINITTOKENKEY | \
>                                  SGX_ATTR_KSS           | \
>                                  SGX_ATTR_ASYNC_EXIT_NOTIFY)
> 
> That at least has a *chance* of someone seeing it who goes to add a new
> attribute.

Hmm, what if we enforce it in code with a compile-time assert?  That will make it
even harder to screw things up, and it also avoids a scenario where someone
extends SGX_ATTR_KVM_MASK without getting approval from KVM folks.  And conversely,
KVM won't need to touch SGX files if there's ever a need to tweak KVM behavior.

		/*
		 * Index 1: SECS.ATTRIBUTES.  ATTRIBUTES are restricted a la
		 * feature flags.  Advertise all supported flags, including
		 * privileged attributes that require explicit opt-in from
		 * userspace.  ATTRIBUTES.XFRM is not adjusted as userspace is
		 * expected to derive it from supported XCR0.
		 */
#define KVM_SGX_ATTR_ALLOWED_MASK (SGX_ATTR_DEBUG |		\
				   SGX_ATTR_MODE64BIT |		\
				   SGX_ATTR_PROVISIONKEY |	\
				   SGX_ATTR_EINITTOKENKEY |	\
				   SGX_ATTR_KSS |		\
				   SGX_ATTR_ASYNC_EXIT_NOTIFY)

#define KVM_SGX_ATTR_DENIED_MASK (0)

		/*
		 * Assert that KVM explicitly allows or denies exposing all
		 * features, i.e. detect attempts to add kernel support without
		 * also updating KVM.
		 */
		BUILD_BUG_ON((KVM_SGX_ATTR_ALLOWED_MASK | KVM_SGX_ATTR_DENIED_MASK) !=
			     (SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK));

		entry->eax &= KVM_SGX_ATTR_ALLOWED_MASK;
		entry->ebx &= 0;
		break;