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

[Sean Christopherson <seanjc@google.com>]

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;