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

Commit Message

Xing, Cedric July 7, 2019, 11:41 p.m. UTC

As enclave pages have different lifespan than the existing MAP_PRIVATE and
MAP_SHARED pages, a new data structure is required outside of VMA to track
their protections and/or origins. Enclave Memory Area (or EMA for short) has
been introduced to address the need. EMAs are maintained by a new LSM module
named “ema”, which is similar to the idea of the “capability” LSM module.

This new “ema” module has LSM_ORDER_FIRST so will always be dispatched before
other LSM_ORDER_MUTABLE modules (e.g. selinux, apparmor, etc.). It is
responsible for initializing EMA maps, and inserting and freeing EMA nodes, and
offers APIs for other LSM modules to query/update EMAs. Details could be found
in include/linux/lsm_ema.h and security/commonema.c.

Signed-off-by: Cedric Xing <[e-mail]>
---
 include/linux/lsm_ema.h |  97 ++++++++++++++
 security/Makefile       |   1 +
 security/commonema.c    | 277 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 375 insertions(+)
 create mode 100644 include/linux/lsm_ema.h
 create mode 100644 security/commonema.c

Comments

Casey Schaufler July 8, 2019, 4:26 p.m. UTC | #1

On 7/7/2019 4:41 PM, Cedric Xing wrote:
> As enclave pages have different lifespan than the existing MAP_PRIVATE and
> MAP_SHARED pages, a new data structure is required outside of VMA to track
> their protections and/or origins. Enclave Memory Area (or EMA for short) has
> been introduced to address the need. EMAs are maintained by a new LSM module
> named “ema”, which is similar to the idea of the “capability” LSM module.

First off, I'll say that this is an improvement over
the LSM integrated version that preceded it. I still
have some issues with the naming, but I'll address that
inline. I do have a suggestion that I think will make
this more conventional.

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


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.

>
> This new “ema” module has LSM_ORDER_FIRST so will always be dispatched before
> other LSM_ORDER_MUTABLE modules (e.g. selinux, apparmor, etc.). It is
> responsible for initializing EMA maps, and inserting and freeing EMA nodes, and
> offers APIs for other LSM modules to query/update EMAs. Details could be found
> in include/linux/lsm_ema.h and security/commonema.c.
>
> Signed-off-by: Cedric Xing <[e-mail]>
> ---
>  include/linux/lsm_ema.h |  97 ++++++++++++++

I still think this should be enclave.h (or commonema.h)
as it is not LSM code.

>  security/Makefile       |   1 +
>  security/commonema.c    | 277 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 375 insertions(+)
>  create mode 100644 include/linux/lsm_ema.h
>  create mode 100644 security/commonema.c
>
> diff --git a/include/linux/lsm_ema.h b/include/linux/lsm_ema.h
> new file mode 100644
> index 000000000000..59fc4ea6fa78
> --- /dev/null
> +++ b/include/linux/lsm_ema.h
> @@ -0,0 +1,97 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
> +/**
> + * Enclave Memory Area interface for LSM modules
> + *
> + * Copyright(c) 2016-19 Intel Corporation.
> + */
> +
> +#ifndef _LSM_EMA_H_
> +#define _LSM_EMA_H_
> +
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/fs.h>
> +#include <linux/file.h>
> +
> +/**
> + * ema - Enclave Memory Area structure for LSM modules

LSM modules is redundant. "LSM" or "LSMs" would be better.

> + *
> + * Data structure to track origins of enclave pages
> + *
> + * @link:
> + *	Link to adjacent EMAs. EMAs are sorted by their addresses in ascending
> + *	order
> + * @start:
> + *	Starting address
> + * @end:
> + *	Ending address
> + * @source:
> + *	File from which this range was loaded from, or NULL if not loaded from
> + *	any files
> + */
> +struct ema {
> +	struct list_head	link;
> +	size_t			start;
> +	size_t			end;
> +	struct file		*source;
> +};
> +
> +#define ema_data(ema, offset)	\
> +	((void *)((char *)((struct ema *)(ema) + 1) + offset))
> +
> +/**
> + * ema_map - LSM Enclave Memory Map structure for LSM modules

As above.

> + *
> + * Container for EMAs of an enclave
> + *
> + * @list:
> + *	Head of a list of sorted EMAs
> + * @lock:
> + *	Acquire before querying/updateing the list EMAs
> + */
> +struct ema_map {
> +	struct list_head	list;
> +	struct mutex		lock;
> +};
> +
> +size_t __init ema_request_blob(size_t blob_size);
> +struct ema_map *ema_get_map(struct file *encl);
> +int ema_apply_to_range(struct ema_map *map, size_t start, size_t end,
> +		       int (*cb)(struct ema *ema, void *arg), void *arg);
> +void ema_remove_range(struct ema_map *map, size_t start, size_t end);
> +
> +static inline int __must_check ema_lock_map(struct ema_map *map)
> +{
> +	return mutex_lock_interruptible(&map->lock);
> +}
> +
> +static inline void ema_unlock_map(struct ema_map *map)
> +{
> +	mutex_unlock(&map->lock);
> +}
> +
> +static inline int ema_lock_apply_to_range(struct ema_map *map,
> +					  size_t start, size_t end,
> +					  int (*cb)(struct ema *, void *),
> +					  void *arg)
> +{
> +	int rc = ema_lock_map(map);
> +	if (!rc) {
> +		rc = ema_apply_to_range(map, start, end, cb, arg);
> +		ema_unlock_map(map);
> +	}
> +	return rc;
> +}
> +
> +static inline int ema_lock_remove_range(struct ema_map *map,
> +					size_t start, size_t end)
> +{
> +	int rc = ema_lock_map(map);
> +	if (!rc) {
> +		ema_remove_range(map, start, end);
> +		ema_unlock_map(map);
> +	}
> +	return rc;
> +}
> +
> +#endif /* _LSM_EMA_H_ */
> 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.

>  
>  # Object integrity file lists
>  subdir-$(CONFIG_INTEGRITY)		+= integrity
> diff --git a/security/commonema.c b/security/commonema.c

Put this in a subdirectory. Please.

> new file mode 100644
> index 000000000000..c5b0bdfdc013
> --- /dev/null
> +++ b/security/commonema.c
> @@ -0,0 +1,277 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2016-18 Intel Corporation.
> +
> +#include <linux/lsm_ema.h>
> +#include <linux/lsm_hooks.h>
> +#include <linux/slab.h>
> +
> +static struct kmem_cache *_map_cache;
> +static struct kmem_cache *_node_cache;
> +static size_t _data_size __lsm_ro_after_init;
> +
> +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().

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

> +}
> +
> +static struct ema_map *_alloc_map(void)

Function header comments, please.

> +{
> +	struct ema_map *m;
> +
> +	m = kmem_cache_zalloc(_map_cache, GFP_KERNEL);
> +	if (likely(m)) {
> +		INIT_LIST_HEAD(&m->list);
> +		mutex_init(&m->lock);
> +	}
> +	return m;
> +}
> +
> +static struct ema *_new_ema(size_t start, size_t end, struct file *src)
> +{
> +	struct ema *ema;
> +
> +	if (unlikely(!_node_cache)) {
> +		struct kmem_cache *c;
> +
> +		c = kmem_cache_create("lsm-ema", sizeof(*ema) + _data_size,
> +				      __alignof__(typeof(*ema)), SLAB_PANIC,
> +				      NULL);
> +		if (atomic_long_cmpxchg((atomic_long_t *)&_node_cache, 0,
> +					(long)c))
> +			kmem_cache_destroy(c);
> +	}
> +
> +	ema = kmem_cache_zalloc(_node_cache, GFP_KERNEL);
> +	if (likely(ema)) {
> +		INIT_LIST_HEAD(&ema->link);
> +		ema->start = start;
> +		ema->end = end;
> +		if (src)
> +			ema->source = get_file(src);
> +	}
> +	return ema;
> +}
> +
> +static void _free_ema(struct ema *ema)
> +{
> +	if (ema->source)
> +		fput(ema->source);
> +	kmem_cache_free(_node_cache, ema);
> +}
> +
> +static void _free_map(struct ema_map *map)
> +{
> +	struct ema *p, *n;
> +
> +	WARN_ON(mutex_is_locked(&map->lock));
> +	list_for_each_entry_safe(p, n, &map->list, link)
> +		_free_ema(p);
> +	kmem_cache_free(_map_cache, map);
> +}
> +
> +static struct ema_map *_init_map(struct file *encl)
> +{
> +	struct ema_map *m = ema_get_map(encl);
> +	if (!m) {
> +		m = _alloc_map();
> +		if (atomic_long_cmpxchg(_map_file(encl), 0, (long)m)) {
> +			_free_map(m);
> +			m = ema_get_map(encl);
> +		}
> +	}
> +	return m;
> +}
> +
> +static inline struct ema *_next_ema(struct ema *p, struct ema_map *map)
> +{
> +	p = list_next_entry(p, link);
> +	return &p->link == &map->list ? NULL : p;
> +}
> +
> +static inline struct ema *_find_ema(struct ema_map *map, size_t a)
> +{
> +	struct ema *p;
> +
> +	WARN_ON(!mutex_is_locked(&map->lock));
> +
> +	list_for_each_entry(p, &map->list, link)
> +		if (a < p->end)
> +			break;
> +	return &p->link == &map->list ? NULL : p;
> +}
> +
> +static struct ema *_split_ema(struct ema *p, size_t at)
> +{
> +	typeof(p) n;
> +
> +	if (at <= p->start || at >= p->end)
> +		return p;
> +
> +	n = _new_ema(p->start, at, p->source);
> +	if (likely(n)) {
> +		memcpy(n + 1, p + 1, _data_size);
> +		p->start = at;
> +		list_add_tail(&n->link, &p->link);
> +	}
> +	return n;
> +}
> +
> +static int _merge_ema(struct ema *p, struct ema_map *map)
> +{
> +	typeof(p) prev = list_prev_entry(p, link);
> +
> +	WARN_ON(!mutex_is_locked(&map->lock));
> +
> +	if (&prev->link == &map->list || prev->end != p->start ||
> +	    prev->source != p->source || memcmp(prev + 1, p + 1, _data_size))
> +		return 0;
> +
> +	p->start = prev->start;
> +	fput(prev->source);
> +	_free_ema(prev);
> +	return 1;
> +}
> +
> +static inline int _insert_ema(struct ema_map *map, struct ema *n)
> +{
> +	typeof(n) p = _find_ema(map, n->start);
> +
> +	if (!p)
> +		list_add_tail(&n->link, &map->list);
> +	else if (n->end <= p->start)
> +		list_add_tail(&n->link, &p->link);
> +	else
> +		return -EEXIST;
> +
> +	_merge_ema(n, map);
> +	if (p)
> +		_merge_ema(p, map);
> +	return 0;
> +}
> +
> +static void ema_file_free_security(struct file *encl)
> +{
> +	struct ema_map *m = ema_get_map(encl);
> +	if (m)
> +		_free_map(m);
> +}
> +
> +static int ema_enclave_load(struct file *encl, size_t start, size_t end,
> +			    size_t flags, struct vm_area_struct *vma)
> +{
> +	struct ema_map *m;
> +	struct ema *ema;
> +	int rc;
> +
> +	m = _init_map(encl);
> +	if (unlikely(!m))
> +		return -ENOMEM;
> +
> +	ema = _new_ema(start, end, vma ? vma->vm_file : NULL);
> +	if (unlikely(!ema))
> +		return -ENOMEM;
> +
> +	rc = ema_lock_map(m);
> +	if (!rc) {
> +		rc = _insert_ema(m, ema);
> +		ema_unlock_map(m);
> +	}
> +	if (rc)
> +		_free_ema(ema);
> +	return rc;
> +}
> +
> +static int ema_enclave_init(struct file *encl, struct sgx_sigstruct *sigstruct,
> +			    struct vm_area_struct *vma)
> +{
> +	if (unlikely(!_init_map(encl)))
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +static struct security_hook_list ema_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(file_free_security, ema_file_free_security),
> +	LSM_HOOK_INIT(enclave_load, ema_enclave_load),
> +	LSM_HOOK_INIT(enclave_init, ema_enclave_init),
> +};
> +
> +static int __init ema_init(void)
> +{
> +	_map_cache = kmem_cache_create("lsm-ema_map", sizeof(struct ema_map),
> +				       __alignof__(struct ema_map), SLAB_PANIC,
> +				       NULL);
> +	security_add_hooks(ema_hooks, ARRAY_SIZE(ema_hooks), "ema");
> +	return 0;
> +}
> +
> +DEFINE_LSM(ema) = {
> +	.name = "ema",
> +	.order = LSM_ORDER_FIRST,
> +	.init = ema_init,
> +	.blobs = &ema_blob_sizes,
> +};
> +
> +/* ema_request_blob shall only be called from LSM module init function */
> +size_t __init ema_request_blob(size_t size)
> +{
> +	typeof(_data_size) offset = _data_size;
> +	_data_size += size;
> +	return offset;
> +}
> +
> +struct ema_map *ema_get_map(struct file *encl)
> +{
> +	return (struct ema_map *)atomic_long_read(_map_file(encl));
> +}
> +
> +/**
> + * Invoke a callback function on every EMA falls within range, split EMAs as
> + * needed
> + */
> +int ema_apply_to_range(struct ema_map *map, size_t start, size_t end,
> +		       int (*cb)(struct ema *, void *), void *arg)
> +{
> +	struct ema *ema;
> +	int rc;
> +
> +	ema = _find_ema(map, start);
> +	while (ema && end > ema->start) {
> +		if (start > ema->start)
> +			_split_ema(ema, start);
> +		if (end < ema->end)
> +			ema = _split_ema(ema, end);
> +
> +		rc = (*cb)(ema, arg);
> +		_merge_ema(ema, map);
> +		if (rc)
> +			return rc;
> +
> +		ema = _next_ema(ema, map);
> +	}
> +
> +	if (ema)
> +		_merge_ema(ema, map);
> +	return 0;
> +}
> +
> +/* Remove all EMAs falling within range, split EMAs as needed */
> +void ema_remove_range(struct ema_map *map, size_t start, size_t end)
> +{
> +	struct ema *ema, *n;
> +
> +	ema = _find_ema(map, start);
> +	while (ema && end > ema->start) {
> +		if (start > ema->start)
> +			_split_ema(ema, start);
> +		if (end < ema->end)
> +			ema = _split_ema(ema, end);
> +
> +		n = _next_ema(ema, map);
> +		_free_ema(ema);
> +		ema = n;
> +	}
> +}

Xing, Cedric July 8, 2019, 5:16 p.m. UTC | #2

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.

Casey Schaufler July 8, 2019, 11:53 p.m. UTC | #3

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.

Xing, Cedric July 9, 2019, 10:13 p.m. UTC | #4

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?

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

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

Casey Schaufler July 10, 2019, 12:10 a.m. UTC | #5

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.

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

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

Xing, Cedric July 10, 2019, 12:55 a.m. UTC | #6

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?

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

Casey Schaufler July 10, 2019, 9:14 p.m. UTC | #7

On 7/9/2019 5: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.

So far, so good ...

> EMA is more like a helper library than a real LSM.

Then you should use it as such.

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

The security modules that want to use EMA should
request that space.

> That cannot be done by any LSMs at runtime.

Sure it can. And it has to. What if you don't
have any security modules that use the EMA data?
Surely you don't want to be allocating blob space
for EMA data if no one is going to use it.

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

That is not true. The security module that wants to use
the EMA data can call whatever allocation function you use.
Or, the call can be made from the code just before you call
the security hook, which would be identical to calling it
as a "first" hook.

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

Call your allocation function just before you call
security_enclave_load(). There is no way that selinux_enclave_load()
could tell the difference.

> 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 this is the enclave abstraction, or rather, should be
according to at least half the people joining in on the
thread. It does not belong in the LSM infrastructure because
it is it's own thing, with its own state and data, which it
needs to maintain in its own way and place. It needs interfaces
so that security modules can use that information appropriately.
It needs a hook or two so that the enclave abstraction can ask
the security modules to make decisions.

>
> And, Stephen, do you have an opinion on this?
>
>>>>>>> +/**
>>>>>>> + * 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.

Thank you.

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

Ah. Being in the security directory does not mean it's
a part of the LSM system. Keys and integrity are security
subsystems that are related to, but not part of, the LSM
sub-system.

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

Please stop asking the same question over and over.
You're not going to get a different answer from what
you've gotten already. Look back at it what's already
been said.

Stephen Smalley July 11, 2019, 1:51 p.m. UTC | #8

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?

Sean Christopherson July 11, 2019, 3:12 p.m. UTC | #9

On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
> 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.

That's a very kind way to phrase things :-)

For initial upstreaming, we've agreed that there is no need to extend the
uapi, i.e. we can punt on deciding between on-the-fly tracking and having
userspace specify maximal permissions until we add SGX2 support.

The last open (knock on wood) for initial upstreaming is whether SELinux
would prefer to have new enclave specific permissions or reuse the
existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
My understanding is that enclave specific permissions are preferred.

Stephen Smalley July 11, 2019, 4:11 p.m. UTC | #10

On 7/11/19 11:12 AM, Sean Christopherson wrote:
> On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
>> 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.
> 
> That's a very kind way to phrase things :-)
> 
> For initial upstreaming, we've agreed that there is no need to extend the
> uapi, i.e. we can punt on deciding between on-the-fly tracking and having
> userspace specify maximal permissions until we add SGX2 support.
> 
> The last open (knock on wood) for initial upstreaming is whether SELinux
> would prefer to have new enclave specific permissions or reuse the
> existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
> My understanding is that enclave specific permissions are preferred.

I was left unclear on this topic after the email exchanges with Cedric.
There are at least three options:

1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions.  Pros: 
Existing distro policies will be applied in the expected manner with 
respect to the introduction of executable code into the system, 
consistent control will be provided over the enclave and the host 
process, no change for users/documentation wrt policy.  Cons: Existing 
permissions don't map exactly to SGX semantics, no ability to 
distinguish executable content within the enclave versus the host 
process at the LSM level (argued earlier by Cedric to be unnecessary and 
perhaps meaningless), need to allow FILE__EXECUTE or other checks on 
sigstruct files that may not actually contain code.

2) Define new permissions within existing security classes (e.g. 
process2, file). Pros: Can tailor permission names and definitions to 
SGX semantics, ability to distinguish enclave versus host process 
execute access, no need to grant FILE__EXECUTE to sigstruct files, class 
matches the target object, permissions computed and cached upon existing 
checks (i.e. when a process accesses a file, all of the permissions to 
that file are computed and then cached at once, including the 
enclave-related ones).  Cons: Typical distro policies (unlike Android) 
allow unknown permissions by default for forward kernel compatibility 
reasons, so existing policies will permit these new permissions by 
default and enforcement will only truly take effect once policies are 
updated, adding new permissions to existing classes requires an update 
to the base policy (so they can't be shipped as a third party policy 
module alongside the SGX driver or installed as a local module by an 
admin, for example), documentation/user education required for new 
permissions.

3) Define new permissions in new security classes (e.g. enclave). Pros 
relative to #2: New classes and permissions can be defined and installed 
in third party or local policy module without requiring a change to the 
base policy.  Cons relative to #2: Class won't correspond to the target 
object, permissions won't be computed and cached upon existing checks 
(only when performing the checks against the new classes).

Combinations are also possible, of course.

Sean Christopherson July 11, 2019, 4:25 p.m. UTC | #11

On Thu, Jul 11, 2019 at 12:11:06PM -0400, Stephen Smalley wrote:
> On 7/11/19 11:12 AM, Sean Christopherson wrote:
> >On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
> >>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.
> >
> >That's a very kind way to phrase things :-)
> >
> >For initial upstreaming, we've agreed that there is no need to extend the
> >uapi, i.e. we can punt on deciding between on-the-fly tracking and having
> >userspace specify maximal permissions until we add SGX2 support.
> >
> >The last open (knock on wood) for initial upstreaming is whether SELinux
> >would prefer to have new enclave specific permissions or reuse the
> >existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
> >My understanding is that enclave specific permissions are preferred.
> 
> I was left unclear on this topic after the email exchanges with Cedric.
> There are at least three options:
> 
> 1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions.  Pros:
> Existing distro policies will be applied in the expected manner with respect
> to the introduction of executable code into the system, consistent control
> will be provided over the enclave and the host process, no change for
> users/documentation wrt policy.  Cons: Existing permissions don't map
> exactly to SGX semantics, no ability to distinguish executable content
> within the enclave versus the host process at the LSM level (argued earlier
> by Cedric to be unnecessary and perhaps meaningless), need to allow
> FILE__EXECUTE or other checks on sigstruct files that may not actually
> contain code.
> 
> 2) Define new permissions within existing security classes (e.g. process2,
> file). Pros: Can tailor permission names and definitions to SGX semantics,
> ability to distinguish enclave versus host process execute access, no need
> to grant FILE__EXECUTE to sigstruct files, class matches the target object,
> permissions computed and cached upon existing checks (i.e. when a process
> accesses a file, all of the permissions to that file are computed and then
> cached at once, including the enclave-related ones).  Cons: Typical distro
> policies (unlike Android) allow unknown permissions by default for forward
> kernel compatibility reasons, so existing policies will permit these new
> permissions by default and enforcement will only truly take effect once
> policies are updated, adding new permissions to existing classes requires an
> update to the base policy (so they can't be shipped as a third party policy
> module alongside the SGX driver or installed as a local module by an admin,
> for example), documentation/user education required for new permissions.
> 
> 3) Define new permissions in new security classes (e.g. enclave). Pros
> relative to #2: New classes and permissions can be defined and installed in
> third party or local policy module without requiring a change to the base
> policy.  Cons relative to #2: Class won't correspond to the target object,
> permissions won't be computed and cached upon existing checks (only when
> performing the checks against the new classes).
> 
> Combinations are also possible, of course.

What's the impact on distros/ecosystems if we go with #1 for now and later
decide to switch to #2 after upstreaming?  I.e. can we take a minimal-ish
approach now without painting ourselves into a corner?

We can map quite closely to the existing intent of EXECUTE, EXECMOD and
EXECMEM via a combination of checking protections at enclave load time and
again at mmap()/mprotect(), e.g.:

#ifdef CONFIG_INTEL_SGX
static inline int enclave_has_perm(u32 sid, u32 requested)
{
	return avc_has_perm(&selinux_state, sid, sid,
			    SECCLASS_PROCESS, requested, NULL);
}

static int selinux_enclave_map(unsigned long prot)
{
	const struct cred *cred = current_cred();
	u32 sid = cred_sid(cred);

	if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
		return enclave_has_perm(sid, PROCESS__EXECMEM);

	return 0;
}

static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot)
{
	const struct cred *cred = current_cred();
	u32 sid = cred_sid(cred);
	int ret;

	/* Only executable enclave pages are restricted in any way. */
	if (!(prot & PROT_EXEC))
		return 0;

	if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file))) {
		ret = enclave_has_perm(sid, PROCESS__EXECMEM);
	} else {
		ret = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
		if (ret)
			goto out;

		/*
		 * Load code from a modified private mapping or from a file
		 * with the ability to do W->X within the enclave.
		 */
		if (vma->anon_vma || (prot & PROT_WRITE))
			ret = file_has_perm(cred, vma->vm_file,
					    FILE__EXECMOD);
	}

out:
	return ret;
}
#endif

Stephen Smalley July 11, 2019, 4:32 p.m. UTC | #12

On 7/11/19 12:25 PM, Sean Christopherson wrote:
> On Thu, Jul 11, 2019 at 12:11:06PM -0400, Stephen Smalley wrote:
>> On 7/11/19 11:12 AM, Sean Christopherson wrote:
>>> On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
>>>> 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.
>>>
>>> That's a very kind way to phrase things :-)
>>>
>>> For initial upstreaming, we've agreed that there is no need to extend the
>>> uapi, i.e. we can punt on deciding between on-the-fly tracking and having
>>> userspace specify maximal permissions until we add SGX2 support.
>>>
>>> The last open (knock on wood) for initial upstreaming is whether SELinux
>>> would prefer to have new enclave specific permissions or reuse the
>>> existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
>>> My understanding is that enclave specific permissions are preferred.
>>
>> I was left unclear on this topic after the email exchanges with Cedric.
>> There are at least three options:
>>
>> 1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions.  Pros:
>> Existing distro policies will be applied in the expected manner with respect
>> to the introduction of executable code into the system, consistent control
>> will be provided over the enclave and the host process, no change for
>> users/documentation wrt policy.  Cons: Existing permissions don't map
>> exactly to SGX semantics, no ability to distinguish executable content
>> within the enclave versus the host process at the LSM level (argued earlier
>> by Cedric to be unnecessary and perhaps meaningless), need to allow
>> FILE__EXECUTE or other checks on sigstruct files that may not actually
>> contain code.
>>
>> 2) Define new permissions within existing security classes (e.g. process2,
>> file). Pros: Can tailor permission names and definitions to SGX semantics,
>> ability to distinguish enclave versus host process execute access, no need
>> to grant FILE__EXECUTE to sigstruct files, class matches the target object,
>> permissions computed and cached upon existing checks (i.e. when a process
>> accesses a file, all of the permissions to that file are computed and then
>> cached at once, including the enclave-related ones).  Cons: Typical distro
>> policies (unlike Android) allow unknown permissions by default for forward
>> kernel compatibility reasons, so existing policies will permit these new
>> permissions by default and enforcement will only truly take effect once
>> policies are updated, adding new permissions to existing classes requires an
>> update to the base policy (so they can't be shipped as a third party policy
>> module alongside the SGX driver or installed as a local module by an admin,
>> for example), documentation/user education required for new permissions.
>>
>> 3) Define new permissions in new security classes (e.g. enclave). Pros
>> relative to #2: New classes and permissions can be defined and installed in
>> third party or local policy module without requiring a change to the base
>> policy.  Cons relative to #2: Class won't correspond to the target object,
>> permissions won't be computed and cached upon existing checks (only when
>> performing the checks against the new classes).
>>
>> Combinations are also possible, of course.
> 
> What's the impact on distros/ecosystems if we go with #1 for now and later
> decide to switch to #2 after upstreaming?  I.e. can we take a minimal-ish
> approach now without painting ourselves into a corner?

Yes, I think that's fine.

> We can map quite closely to the existing intent of EXECUTE, EXECMOD and
> EXECMEM via a combination of checking protections at enclave load time and
> again at mmap()/mprotect(), e.g.:
> 
> #ifdef CONFIG_INTEL_SGX
> static inline int enclave_has_perm(u32 sid, u32 requested)
> {
> 	return avc_has_perm(&selinux_state, sid, sid,
> 			    SECCLASS_PROCESS, requested, NULL);
> }
> 
> static int selinux_enclave_map(unsigned long prot)
> {
> 	const struct cred *cred = current_cred();
> 	u32 sid = cred_sid(cred);
> 
> 	if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> 		return enclave_has_perm(sid, PROCESS__EXECMEM);
> 
> 	return 0;
> }
> 
> static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot)
> {
> 	const struct cred *cred = current_cred();
> 	u32 sid = cred_sid(cred);
> 	int ret;
> 
> 	/* Only executable enclave pages are restricted in any way. */
> 	if (!(prot & PROT_EXEC))
> 		return 0;
> 
> 	if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file))) {
> 		ret = enclave_has_perm(sid, PROCESS__EXECMEM);
> 	} else {
> 		ret = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
> 		if (ret)
> 			goto out;
> 
> 		/*
> 		 * Load code from a modified private mapping or from a file
> 		 * with the ability to do W->X within the enclave.
> 		 */
> 		if (vma->anon_vma || (prot & PROT_WRITE))
> 			ret = file_has_perm(cred, vma->vm_file,
> 					    FILE__EXECMOD);
> 	}
> 
> out:
> 	return ret;
> }
> #endif
>

Xing, Cedric July 11, 2019, 11:41 p.m. UTC | #13

On 7/11/2019 9:32 AM, Stephen Smalley wrote:
> On 7/11/19 12:25 PM, Sean Christopherson wrote:
>> On Thu, Jul 11, 2019 at 12:11:06PM -0400, Stephen Smalley wrote:
>>> On 7/11/19 11:12 AM, Sean Christopherson wrote:
>>>> On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
>>>>> 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.
>>>>
>>>> That's a very kind way to phrase things :-)
>>>>
>>>> For initial upstreaming, we've agreed that there is no need to 
>>>> extend the
>>>> uapi, i.e. we can punt on deciding between on-the-fly tracking and 
>>>> having
>>>> userspace specify maximal permissions until we add SGX2 support.
>>>>
>>>> The last open (knock on wood) for initial upstreaming is whether 
>>>> SELinux
>>>> would prefer to have new enclave specific permissions or reuse the
>>>> existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
>>>> My understanding is that enclave specific permissions are preferred.
>>>
>>> I was left unclear on this topic after the email exchanges with Cedric.
>>> There are at least three options:
>>>
>>> 1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions.  Pros:
>>> Existing distro policies will be applied in the expected manner with 
>>> respect
>>> to the introduction of executable code into the system, consistent 
>>> control
>>> will be provided over the enclave and the host process, no change for
>>> users/documentation wrt policy.  Cons: Existing permissions don't map
>>> exactly to SGX semantics, no ability to distinguish executable content
>>> within the enclave versus the host process at the LSM level (argued 
>>> earlier
>>> by Cedric to be unnecessary and perhaps meaningless), need to allow
>>> FILE__EXECUTE or other checks on sigstruct files that may not actually
>>> contain code.
>>>
>>> 2) Define new permissions within existing security classes (e.g. 
>>> process2,
>>> file). Pros: Can tailor permission names and definitions to SGX 
>>> semantics,
>>> ability to distinguish enclave versus host process execute access, no 
>>> need
>>> to grant FILE__EXECUTE to sigstruct files, class matches the target 
>>> object,
>>> permissions computed and cached upon existing checks (i.e. when a 
>>> process
>>> accesses a file, all of the permissions to that file are computed and 
>>> then
>>> cached at once, including the enclave-related ones).  Cons: Typical 
>>> distro
>>> policies (unlike Android) allow unknown permissions by default for 
>>> forward
>>> kernel compatibility reasons, so existing policies will permit these new
>>> permissions by default and enforcement will only truly take effect once
>>> policies are updated, adding new permissions to existing classes 
>>> requires an
>>> update to the base policy (so they can't be shipped as a third party 
>>> policy
>>> module alongside the SGX driver or installed as a local module by an 
>>> admin,
>>> for example), documentation/user education required for new permissions.
>>>
>>> 3) Define new permissions in new security classes (e.g. enclave). Pros
>>> relative to #2: New classes and permissions can be defined and 
>>> installed in
>>> third party or local policy module without requiring a change to the 
>>> base
>>> policy.  Cons relative to #2: Class won't correspond to the target 
>>> object,
>>> permissions won't be computed and cached upon existing checks (only when
>>> performing the checks against the new classes).
>>>
>>> Combinations are also possible, of course.
>>
>> What's the impact on distros/ecosystems if we go with #1 for now and 
>> later
>> decide to switch to #2 after upstreaming?  I.e. can we take a minimal-ish
>> approach now without painting ourselves into a corner?
> 
> Yes, I think that's fine.

I can't agree more on this. It's easier to add new things than to take 
existing things out. We can just wait until usages come up that really 
require new permissions.

Patch

diff --git a/include/linux/lsm_ema.h b/include/linux/lsm_ema.h
new file mode 100644
index 000000000000..59fc4ea6fa78
--- /dev/null
+++ b/include/linux/lsm_ema.h
@@ -0,0 +1,97 @@ 
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
+/**
+ * Enclave Memory Area interface for LSM modules
+ *
+ * Copyright(c) 2016-19 Intel Corporation.
+ */
+
+#ifndef _LSM_EMA_H_
+#define _LSM_EMA_H_
+
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/fs.h>
+#include <linux/file.h>
+
+/**
+ * ema - Enclave Memory Area structure for LSM modules
+ *
+ * Data structure to track origins of enclave pages
+ *
+ * @link:
+ *	Link to adjacent EMAs. EMAs are sorted by their addresses in ascending
+ *	order
+ * @start:
+ *	Starting address
+ * @end:
+ *	Ending address
+ * @source:
+ *	File from which this range was loaded from, or NULL if not loaded from
+ *	any files
+ */
+struct ema {
+	struct list_head	link;
+	size_t			start;
+	size_t			end;
+	struct file		*source;
+};
+
+#define ema_data(ema, offset)	\
+	((void *)((char *)((struct ema *)(ema) + 1) + offset))
+
+/**
+ * ema_map - LSM Enclave Memory Map structure for LSM modules
+ *
+ * Container for EMAs of an enclave
+ *
+ * @list:
+ *	Head of a list of sorted EMAs
+ * @lock:
+ *	Acquire before querying/updateing the list EMAs
+ */
+struct ema_map {
+	struct list_head	list;
+	struct mutex		lock;
+};
+
+size_t __init ema_request_blob(size_t blob_size);
+struct ema_map *ema_get_map(struct file *encl);
+int ema_apply_to_range(struct ema_map *map, size_t start, size_t end,
+		       int (*cb)(struct ema *ema, void *arg), void *arg);
+void ema_remove_range(struct ema_map *map, size_t start, size_t end);
+
+static inline int __must_check ema_lock_map(struct ema_map *map)
+{
+	return mutex_lock_interruptible(&map->lock);
+}
+
+static inline void ema_unlock_map(struct ema_map *map)
+{
+	mutex_unlock(&map->lock);
+}
+
+static inline int ema_lock_apply_to_range(struct ema_map *map,
+					  size_t start, size_t end,
+					  int (*cb)(struct ema *, void *),
+					  void *arg)
+{
+	int rc = ema_lock_map(map);
+	if (!rc) {
+		rc = ema_apply_to_range(map, start, end, cb, arg);
+		ema_unlock_map(map);
+	}
+	return rc;
+}
+
+static inline int ema_lock_remove_range(struct ema_map *map,
+					size_t start, size_t end)
+{
+	int rc = ema_lock_map(map);
+	if (!rc) {
+		ema_remove_range(map, start, end);
+		ema_unlock_map(map);
+	}
+	return rc;
+}
+
+#endif /* _LSM_EMA_H_ */
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
 
 # Object integrity file lists
 subdir-$(CONFIG_INTEGRITY)		+= integrity
diff --git a/security/commonema.c b/security/commonema.c
new file mode 100644
index 000000000000..c5b0bdfdc013
--- /dev/null
+++ b/security/commonema.c
@@ -0,0 +1,277 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-18 Intel Corporation.
+
+#include <linux/lsm_ema.h>
+#include <linux/lsm_hooks.h>
+#include <linux/slab.h>
+
+static struct kmem_cache *_map_cache;
+static struct kmem_cache *_node_cache;
+static size_t _data_size __lsm_ro_after_init;
+
+static struct lsm_blob_sizes ema_blob_sizes __lsm_ro_after_init = {
+	.lbs_file = sizeof(atomic_long_t),
+};
+
+static atomic_long_t *_map_file(struct file *encl)
+{
+	return (void *)((char *)(encl->f_security) + ema_blob_sizes.lbs_file);
+}
+
+static struct ema_map *_alloc_map(void)
+{
+	struct ema_map *m;
+
+	m = kmem_cache_zalloc(_map_cache, GFP_KERNEL);
+	if (likely(m)) {
+		INIT_LIST_HEAD(&m->list);
+		mutex_init(&m->lock);
+	}
+	return m;
+}
+
+static struct ema *_new_ema(size_t start, size_t end, struct file *src)
+{
+	struct ema *ema;
+
+	if (unlikely(!_node_cache)) {
+		struct kmem_cache *c;
+
+		c = kmem_cache_create("lsm-ema", sizeof(*ema) + _data_size,
+				      __alignof__(typeof(*ema)), SLAB_PANIC,
+				      NULL);
+		if (atomic_long_cmpxchg((atomic_long_t *)&_node_cache, 0,
+					(long)c))
+			kmem_cache_destroy(c);
+	}
+
+	ema = kmem_cache_zalloc(_node_cache, GFP_KERNEL);
+	if (likely(ema)) {
+		INIT_LIST_HEAD(&ema->link);
+		ema->start = start;
+		ema->end = end;
+		if (src)
+			ema->source = get_file(src);
+	}
+	return ema;
+}
+
+static void _free_ema(struct ema *ema)
+{
+	if (ema->source)
+		fput(ema->source);
+	kmem_cache_free(_node_cache, ema);
+}
+
+static void _free_map(struct ema_map *map)
+{
+	struct ema *p, *n;
+
+	WARN_ON(mutex_is_locked(&map->lock));
+	list_for_each_entry_safe(p, n, &map->list, link)
+		_free_ema(p);
+	kmem_cache_free(_map_cache, map);
+}
+
+static struct ema_map *_init_map(struct file *encl)
+{
+	struct ema_map *m = ema_get_map(encl);
+	if (!m) {
+		m = _alloc_map();
+		if (atomic_long_cmpxchg(_map_file(encl), 0, (long)m)) {
+			_free_map(m);
+			m = ema_get_map(encl);
+		}
+	}
+	return m;
+}
+
+static inline struct ema *_next_ema(struct ema *p, struct ema_map *map)
+{
+	p = list_next_entry(p, link);
+	return &p->link == &map->list ? NULL : p;
+}
+
+static inline struct ema *_find_ema(struct ema_map *map, size_t a)
+{
+	struct ema *p;
+
+	WARN_ON(!mutex_is_locked(&map->lock));
+
+	list_for_each_entry(p, &map->list, link)
+		if (a < p->end)
+			break;
+	return &p->link == &map->list ? NULL : p;
+}
+
+static struct ema *_split_ema(struct ema *p, size_t at)
+{
+	typeof(p) n;
+
+	if (at <= p->start || at >= p->end)
+		return p;
+
+	n = _new_ema(p->start, at, p->source);
+	if (likely(n)) {
+		memcpy(n + 1, p + 1, _data_size);
+		p->start = at;
+		list_add_tail(&n->link, &p->link);
+	}
+	return n;
+}
+
+static int _merge_ema(struct ema *p, struct ema_map *map)
+{
+	typeof(p) prev = list_prev_entry(p, link);
+
+	WARN_ON(!mutex_is_locked(&map->lock));
+
+	if (&prev->link == &map->list || prev->end != p->start ||
+	    prev->source != p->source || memcmp(prev + 1, p + 1, _data_size))
+		return 0;
+
+	p->start = prev->start;
+	fput(prev->source);
+	_free_ema(prev);
+	return 1;
+}
+
+static inline int _insert_ema(struct ema_map *map, struct ema *n)
+{
+	typeof(n) p = _find_ema(map, n->start);
+
+	if (!p)
+		list_add_tail(&n->link, &map->list);
+	else if (n->end <= p->start)
+		list_add_tail(&n->link, &p->link);
+	else
+		return -EEXIST;
+
+	_merge_ema(n, map);
+	if (p)
+		_merge_ema(p, map);
+	return 0;
+}
+
+static void ema_file_free_security(struct file *encl)
+{
+	struct ema_map *m = ema_get_map(encl);
+	if (m)
+		_free_map(m);
+}
+
+static int ema_enclave_load(struct file *encl, size_t start, size_t end,
+			    size_t flags, struct vm_area_struct *vma)
+{
+	struct ema_map *m;
+	struct ema *ema;
+	int rc;
+
+	m = _init_map(encl);
+	if (unlikely(!m))
+		return -ENOMEM;
+
+	ema = _new_ema(start, end, vma ? vma->vm_file : NULL);
+	if (unlikely(!ema))
+		return -ENOMEM;
+
+	rc = ema_lock_map(m);
+	if (!rc) {
+		rc = _insert_ema(m, ema);
+		ema_unlock_map(m);
+	}
+	if (rc)
+		_free_ema(ema);
+	return rc;
+}
+
+static int ema_enclave_init(struct file *encl, struct sgx_sigstruct *sigstruct,
+			    struct vm_area_struct *vma)
+{
+	if (unlikely(!_init_map(encl)))
+		return -ENOMEM;
+	return 0;
+}
+
+static struct security_hook_list ema_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(file_free_security, ema_file_free_security),
+	LSM_HOOK_INIT(enclave_load, ema_enclave_load),
+	LSM_HOOK_INIT(enclave_init, ema_enclave_init),
+};
+
+static int __init ema_init(void)
+{
+	_map_cache = kmem_cache_create("lsm-ema_map", sizeof(struct ema_map),
+				       __alignof__(struct ema_map), SLAB_PANIC,
+				       NULL);
+	security_add_hooks(ema_hooks, ARRAY_SIZE(ema_hooks), "ema");
+	return 0;
+}
+
+DEFINE_LSM(ema) = {
+	.name = "ema",
+	.order = LSM_ORDER_FIRST,
+	.init = ema_init,
+	.blobs = &ema_blob_sizes,
+};
+
+/* ema_request_blob shall only be called from LSM module init function */
+size_t __init ema_request_blob(size_t size)
+{
+	typeof(_data_size) offset = _data_size;
+	_data_size += size;
+	return offset;
+}
+
+struct ema_map *ema_get_map(struct file *encl)
+{
+	return (struct ema_map *)atomic_long_read(_map_file(encl));
+}
+
+/**
+ * Invoke a callback function on every EMA falls within range, split EMAs as
+ * needed
+ */
+int ema_apply_to_range(struct ema_map *map, size_t start, size_t end,
+		       int (*cb)(struct ema *, void *), void *arg)
+{
+	struct ema *ema;
+	int rc;
+
+	ema = _find_ema(map, start);
+	while (ema && end > ema->start) {
+		if (start > ema->start)
+			_split_ema(ema, start);
+		if (end < ema->end)
+			ema = _split_ema(ema, end);
+
+		rc = (*cb)(ema, arg);
+		_merge_ema(ema, map);
+		if (rc)
+			return rc;
+
+		ema = _next_ema(ema, map);
+	}
+
+	if (ema)
+		_merge_ema(ema, map);
+	return 0;
+}
+
+/* Remove all EMAs falling within range, split EMAs as needed */
+void ema_remove_range(struct ema_map *map, size_t start, size_t end)
+{
+	struct ema *ema, *n;
+
+	ema = _find_ema(map, start);
+	while (ema && end > ema->start) {
+		if (start > ema->start)
+			_split_ema(ema, start);
+		if (end < ema->end)
+			ema = _split_ema(ema, end);
+
+		n = _next_ema(ema, map);
+		_free_ema(ema);
+		ema = n;
+	}
+}