On 7/9/19 8:55 PM, Xing, Cedric wrote:
On 7/9/2019 5:10 PM, Casey Schaufler wrote:
On 7/9/2019 3:13 PM, Xing, Cedric wrote:
On 7/8/2019 4:53 PM, Casey Schaufler wrote:
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().
Sorry but I'm still confused.
EMA code is used by LSMs only. Making it callable from other parts of
the kernel IMHO is probably not a good idea. And more importantly I
don't understand the motivation behind it. Would you please elaborate?
LSM modules implement additional access control restrictions.
The EMA code does not do that, it provides management of data
that is used by security modules. It is not one itself. VFS
also performs this role, but no one would consider making VFS
a security module.
You are right. EMA is more like a helper library than a real LSM. But
the practical problem is, it has a piece of initialization code, to
basically request some space in the file blob from the LSM
infrastructure. That cannot be done by any LSMs at runtime. So it has to
either be done in LSM infrastructure directly, or make itself an LSM to
make its initialization function invoked by LSM infrastructure
automatically. You have objected to the former, so I switched to the
latter. Are you now objecting to the latter as well? Then what are you
suggesting, really?
VFS is a completely different story. It's the file system abstraction so
it has a natural place to live in the kernel, and its initialization
doesn't depend on the LSM infrastructure. EMA on the other hand, shall
belong to LSM because it is both produced and consumed within LSM.
And, Stephen, do you have an opinion on this?
I don't really understand Casey's position. I also wouldn't necessarily
view his opinion on the matter as necessarily authoritative since he is
not the LSM maintainer as far as I know although he has contributed a
lot of changes in recent years.
I understood the architecture of the original EMA implementation (i.e. a
support library to be used by the security modules to help them in
implementing their own access control policies), although I do have some
concerns about the complexity and lifecycle issues, and wonder if a
simpler model as suggested by Dr. Greg isn't feasible.
I'd also feel better if there was clear consensus among all of the
@intel.com participants that this is the right approach. To date that
has seemed elusive.
If there were consensus that the EMA approach was the right one and that
a simpler model is not feasible, and if the only obstacle to adopting
EMA was Casey's objections, then I'd say just put it directly into
SELinux and be done with it. I originally thought that was a mistake
but if other security modules don't want the support, so be it.
+/**
+ * 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
Your suggestions are reasonable. Given such config change wouldn't
affect any code, can we do it later,
That doesn't make the current options any less confusing,
and it will be easier to make the change now than at some
point in the future.
e.g., when additional TEEs come online and make use of these new
hooks? After all, security_enclave_init() will need amendment anyway
as one of its current parameters is of type 'struct sgx_sigstruct',
which will need to be replaced with something more generic. At the
time being, I'd like to keep things intuitive so as not to confuse
reviewers.
Reviewers (including me) are already confused by the inconsistency.
OK. Let me make this change.
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.
Alright. I'd move it into a sub-folder and rename it to ema.c. Would
you be ok with that?
Sounds fine.
This is another part that confuses me. Per you comment here, I think you
are OK with EMA being part of LSM (I mean, living somewhere under
security/). But your other comment of calling ema_enclave_load()
alongside security_enclave_load() made me think EMA and LSM were
separate. What do you want really?
