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.
