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.
