Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module

["Xing, Cedric" <cedric.xing@intel.com>]

On 7/8/2019 9:26 AM, Casey Schaufler wrote:
> In this scheme you use an ema LSM to manage your ema data.
> A quick sketch looks like:
> 
> 	sgx_something_in() calls
> 		security_enclave_load() calls
> 			ema_enclave_load()
> 			selinux_enclave_load()
> 			otherlsm_enclave_load()
> 
> Why is this better than:
> 
> 	sgx_something_in() calls
> 		ema_enclave_load()
> 		security_enclave_load() calls
> 			selinux_enclave_load()
> 			otherlsm_enclave_load()

Are you talking about moving EMA somewhere outside LSM? If so, where?

> 
> 
> If you did really want ema to behave like an LSM
> you would put the file data that SELinux is managing
> into the ema portion of the blob and provide interfaces
> for the SELinux (or whoever) to use that. Also, it's
> an abomination (as I've stated before) for ema to
> rely on SELinux to provide a file_free() hook for
> ema's data. If you continue down the LSM route, you
> need to provide an ema_file_free() hook. You can't
> count on SELinux to do it for you. If there are multiple
> LSMs (coming soon!) that use the ema data, they'll all
> try to free it, and then Bad Things can happen.

I'm afraid you have misunderstood the code. What is kept open and gets 
closed in selinux_file_free() is the sigstruct file. SELinux uses it to 
determine the page permissions for enclave pages from anonymous sources. 
It is a policy choice made inside SELinux and has nothing to do with EMA.

There's indeed an ema_file_free_security() to free the EMA map for 
enclaves being closed. EMA does *NOT* rely on any other LSMs to free 
data for it. The only exception is when an LSM fails enclave_load(), it 
has to call ema_remove_range() to remove the range being added, which 
was *not* required originally in v2.

>> +/**
>> + * ema - Enclave Memory Area structure for LSM modules
> 
> LSM modules is redundant. "LSM" or "LSMs" would be better.

Noted

>> diff --git a/security/Makefile b/security/Makefile
>> index c598b904938f..b66d03a94853 100644
>> --- a/security/Makefile
>> +++ b/security/Makefile
>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA)		+= yama/
>>   obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
>>   obj-$(CONFIG_SECURITY_SAFESETID)       += safesetid/
>>   obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
>> +obj-$(CONFIG_INTEL_SGX)			+= commonema.o
> 
> The config option and the file name ought to match,
> or at least be closer.

Just trying to match file names as "capability" uses commoncap.c.

Like I said, this feature could potentially be used by TEEs other than 
SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I 
can rename it to ema.c or enclave.c. Do you have a preference?

>> diff --git a/security/commonema.c b/security/commonema.c
> 
> Put this in a subdirectory. Please.

Then why is commoncap.c located in this directory? I'm just trying to 
match the existing convention.

>> +static struct lsm_blob_sizes ema_blob_sizes __lsm_ro_after_init = {
>> +	.lbs_file = sizeof(atomic_long_t),
>> +};
> 
> If this is ema's data ema must manage it. You *must* have
> a file_free().

There is one indeed - ema_file_free_security().

> 
>> +
>> +static atomic_long_t *_map_file(struct file *encl)
>> +{
>> +	return (void *)((char *)(encl->f_security) + ema_blob_sizes.lbs_file);
> 
> I don't trust all the casting going on here, especially since
> you don't end up with the type you should be returning.

Will change.

>> +}
>> +
>> +static struct ema_map *_alloc_map(void)
> 
> Function header comments, please.

Will add.