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

[Casey Schaufler]

On 7/8/2019 10:16 AM, Xing, Cedric wrote:
> 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?

Yes. That's what I've been saying all along.

> If so, where?

I tried to make it obvious. Put the call to your EMA code
on the line before you call security_enclave_load().

>
>>
>> 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.

OK.

>
> 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.

OK.

>
>>> +/**
>>> + * 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.

Fine, then you should be using CONFIG_SECURITY_EMA.

>
> 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?

Make
	CONFIG_SECURITY_EMA
	depends on CONFIG_INTEL_SGX

When another TEE (maybe MIPS_SSRPQ) comes along you can have

	CONFIG_SECURITY_EMA
	depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
 

>
>>> 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.

commoncap is not optional. It is a base part of the
security subsystem. ema is optional.


>
>>> +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().

I see it now.

>
>>
>>> +
>>> +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.