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;
