* [PATCH 6/9] keys: Include target namespace in match criteria [ver #4]
From: David Howells @ 2019-06-19 16:47 UTC (permalink / raw)
To: ebiederm, keyrings
Cc: linux-cifs, linux-nfs, netdev, linux-afs, dhowells, dwalsh,
vgoyal, linux-security-module, linux-fsdevel, linux-kernel
In-Reply-To: <156096279115.28733.8761881995303698232.stgit@warthog.procyon.org.uk>
Currently a key has a standard matching criteria of { type, description }
and this is used to only allow keys with unique criteria in a keyring.
This means, however, that you cannot have keys with the same type and
description but a different target namespace in the same keyring.
This is a potential problem for a containerised environment where, say, a
container is made up of some parts of its mount space involving netfs
superblocks from two different network namespaces.
This is also a problem for shared system management keyrings such as the
DNS records keyring or the NFS idmapper keyring that might contain keys
from different network namespaces.
Fix this by including a namespace component in a key's matching criteria.
Keyring types are marked to indicate which, if any, namespace is relevant
to keys of that type, and that namespace is set when the key is created
from the current task's namespace set.
The capability bit KEYCTL_CAPS1_NS_KEY_TAG is set if the kernel is
employing this feature.
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/linux/key.h | 10 ++++++++++
include/uapi/linux/keyctl.h | 1 +
security/keys/gc.c | 2 +-
security/keys/key.c | 1 +
security/keys/keyctl.c | 3 ++-
security/keys/keyring.c | 36 ++++++++++++++++++++++++++++++++++--
security/keys/persistent.c | 1 +
7 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index ae1177302d70..abc68555bac3 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -82,9 +82,16 @@ struct cred;
struct key_type;
struct key_owner;
+struct key_tag;
struct keyring_list;
struct keyring_name;
+struct key_tag {
+ struct rcu_head rcu;
+ refcount_t usage;
+ bool removed; /* T when subject removed */
+};
+
struct keyring_index_key {
/* [!] If this structure is altered, the union in struct key must change too! */
unsigned long hash; /* Hash value */
@@ -101,6 +108,7 @@ struct keyring_index_key {
unsigned long x;
};
struct key_type *type;
+ struct key_tag *domain_tag; /* Domain of operation */
const char *description;
};
@@ -218,6 +226,7 @@ struct key {
unsigned long hash;
unsigned long len_desc;
struct key_type *type; /* type of key */
+ struct key_tag *domain_tag; /* Domain of operation */
char *description;
};
};
@@ -268,6 +277,7 @@ extern struct key *key_alloc(struct key_type *type,
extern void key_revoke(struct key *key);
extern void key_invalidate(struct key *key);
extern void key_put(struct key *key);
+extern bool key_put_tag(struct key_tag *tag);
static inline struct key *__key_get(struct key *key)
{
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 35b405034674..3bb5324d514f 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -129,5 +129,6 @@ struct keyctl_pkey_params {
#define KEYCTL_CAPS0_RESTRICT_KEYRING 0x40 /* KEYCTL_RESTRICT_KEYRING supported */
#define KEYCTL_CAPS0_MOVE 0x80 /* KEYCTL_MOVE supported */
#define KEYCTL_CAPS1_NS_KEYRING_NAME 0x01 /* Keyring names are per-user_namespace */
+#define KEYCTL_CAPS2_NS_KEY_TAG 0x02 /* Key indexing can include a namespace tag */
#endif /* _LINUX_KEYCTL_H */
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 634e96b380e8..83d279fb7793 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -154,7 +154,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
atomic_dec(&key->user->nikeys);
key_user_put(key->user);
-
+ key_put_tag(key->domain_tag);
kfree(key->description);
memzero_explicit(key, sizeof(*key));
diff --git a/security/keys/key.c b/security/keys/key.c
index 9d52f2472a09..85fdc2ea6c14 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -317,6 +317,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
goto security_error;
/* publish the key by giving it a serial number */
+ refcount_inc(&key->domain_tag->usage);
atomic_inc(&user->nkeys);
key_alloc_serial(key);
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 8a813220f269..aa9be531e5f5 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -40,7 +40,8 @@ static const unsigned char keyrings_capabilities[2] = {
KEYCTL_CAPS0_RESTRICT_KEYRING |
KEYCTL_CAPS0_MOVE
),
- [1] = (KEYCTL_CAPS1_NS_KEYRING_NAME),
+ [1] = (KEYCTL_CAPS1_NS_KEYRING_NAME |
+ KEYCTL_CAPS2_NS_KEY_TAG),
};
static int key_get_type_from_user(char *type,
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 3663e5168583..0da8fa282d56 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -175,6 +175,9 @@ static void hash_key_type_and_desc(struct keyring_index_key *index_key)
type = (unsigned long)index_key->type;
acc = mult_64x32_and_fold(type, desc_len + 13);
acc = mult_64x32_and_fold(acc, 9207);
+ piece = (unsigned long)index_key->domain_tag;
+ acc = mult_64x32_and_fold(acc, piece);
+ acc = mult_64x32_and_fold(acc, 9207);
for (;;) {
n = desc_len;
@@ -208,16 +211,36 @@ static void hash_key_type_and_desc(struct keyring_index_key *index_key)
/*
* Finalise an index key to include a part of the description actually in the
- * index key and to add in the hash too.
+ * index key, to set the domain tag and to calculate the hash.
*/
void key_set_index_key(struct keyring_index_key *index_key)
{
+ static struct key_tag default_domain_tag = { .usage = REFCOUNT_INIT(1), };
size_t n = min_t(size_t, index_key->desc_len, sizeof(index_key->desc));
+
memcpy(index_key->desc, index_key->description, n);
+ index_key->domain_tag = &default_domain_tag;
hash_key_type_and_desc(index_key);
}
+/**
+ * key_put_tag - Release a ref on a tag.
+ * @tag: The tag to release.
+ *
+ * This releases a reference the given tag and returns true if that ref was the
+ * last one.
+ */
+bool key_put_tag(struct key_tag *tag)
+{
+ if (refcount_dec_and_test(&tag->usage)) {
+ kfree_rcu(tag, rcu);
+ return true;
+ }
+
+ return false;
+}
+
/*
* Build the next index key chunk.
*
@@ -238,8 +261,10 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
return index_key->x;
case 2:
return (unsigned long)index_key->type;
+ case 3:
+ return (unsigned long)index_key->domain_tag;
default:
- level -= 3;
+ level -= 4;
if (desc_len <= sizeof(index_key->desc))
return 0;
@@ -268,6 +293,7 @@ static bool keyring_compare_object(const void *object, const void *data)
const struct key *key = keyring_ptr_to_key(object);
return key->index_key.type == index_key->type &&
+ key->index_key.domain_tag == index_key->domain_tag &&
key->index_key.desc_len == index_key->desc_len &&
memcmp(key->index_key.description, index_key->description,
index_key->desc_len) == 0;
@@ -309,6 +335,12 @@ static int keyring_diff_objects(const void *object, const void *data)
goto differ;
level += sizeof(unsigned long);
+ seg_a = (unsigned long)a->domain_tag;
+ seg_b = (unsigned long)b->domain_tag;
+ if ((seg_a ^ seg_b) != 0)
+ goto differ;
+ level += sizeof(unsigned long);
+
i = sizeof(a->desc);
if (a->desc_len <= i)
goto same;
diff --git a/security/keys/persistent.c b/security/keys/persistent.c
index 90303fe4a394..9944d855a28d 100644
--- a/security/keys/persistent.c
+++ b/security/keys/persistent.c
@@ -84,6 +84,7 @@ static long key_get_persistent(struct user_namespace *ns, kuid_t uid,
long ret;
/* Look in the register if it exists */
+ memset(&index_key, 0, sizeof(index_key));
index_key.type = &key_type_keyring;
index_key.description = buf;
index_key.desc_len = sprintf(buf, "_persistent.%u", from_kuid(ns, uid));
^ permalink raw reply related
* [PATCH 7/9] keys: Garbage collect keys for which the domain has been removed [ver #4]
From: David Howells @ 2019-06-19 16:47 UTC (permalink / raw)
To: ebiederm, keyrings
Cc: linux-cifs, linux-nfs, netdev, linux-afs, dhowells, dwalsh,
vgoyal, linux-security-module, linux-fsdevel, linux-kernel
In-Reply-To: <156096279115.28733.8761881995303698232.stgit@warthog.procyon.org.uk>
If a key operation domain (such as a network namespace) has been removed
then attempt to garbage collect all the keys that use it.
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/linux/key.h | 2 ++
security/keys/internal.h | 3 ++-
security/keys/keyring.c | 15 +++++++++++++++
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index abc68555bac3..60c076c6e47f 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -278,6 +278,7 @@ extern void key_revoke(struct key *key);
extern void key_invalidate(struct key *key);
extern void key_put(struct key *key);
extern bool key_put_tag(struct key_tag *tag);
+extern void key_remove_domain(struct key_tag *domain_tag);
static inline struct key *__key_get(struct key *key)
{
@@ -446,6 +447,7 @@ extern void key_init(void);
#define key_fsgid_changed(c) do { } while(0)
#define key_init() do { } while(0)
#define key_free_user_ns(ns) do { } while(0)
+#define key_remove_domain(d) do { } while(0)
#endif /* CONFIG_KEYS */
#endif /* __KERNEL__ */
diff --git a/security/keys/internal.h b/security/keys/internal.h
index d3a9439e2386..5a561f5f199e 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -209,7 +209,8 @@ static inline bool key_is_dead(const struct key *key, time64_t limit)
return
key->flags & ((1 << KEY_FLAG_DEAD) |
(1 << KEY_FLAG_INVALIDATED)) ||
- (key->expiry > 0 && key->expiry <= limit);
+ (key->expiry > 0 && key->expiry <= limit) ||
+ key->domain_tag->removed;
}
/*
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 0da8fa282d56..d3c86fda1510 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -241,6 +241,21 @@ bool key_put_tag(struct key_tag *tag)
return false;
}
+/**
+ * key_remove_domain - Kill off a key domain and gc its keys
+ * @domain_tag: The domain tag to release.
+ *
+ * This marks a domain tag as being dead and releases a ref on it. If that
+ * wasn't the last reference, the garbage collector is poked to try and delete
+ * all keys that were in the domain.
+ */
+void key_remove_domain(struct key_tag *domain_tag)
+{
+ domain_tag->removed = true;
+ if (!key_put_tag(domain_tag))
+ key_schedule_gc_links();
+}
+
/*
* Build the next index key chunk.
*
^ permalink raw reply related
* [PATCH 8/9] keys: Network namespace domain tag [ver #4]
From: David Howells @ 2019-06-19 16:47 UTC (permalink / raw)
To: ebiederm, keyrings
Cc: linux-cifs, linux-nfs, netdev, linux-afs, dhowells, dwalsh,
vgoyal, linux-security-module, linux-fsdevel, linux-kernel
In-Reply-To: <156096279115.28733.8761881995303698232.stgit@warthog.procyon.org.uk>
Create key domain tags for network namespaces and make it possible to
automatically tag keys that are used by networked services (e.g. AF_RXRPC,
AFS, DNS) with the default network namespace if not set by the caller.
This allows keys with the same description but in different namespaces to
coexist within a keyring.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: netdev@vger.kernel.org
cc: linux-nfs@vger.kernel.org
cc: linux-cifs@vger.kernel.org
cc: linux-afs@lists.infradead.org
---
include/linux/key-type.h | 3 +++
include/net/net_namespace.h | 3 +++
net/core/net_namespace.c | 19 +++++++++++++++++++
net/dns_resolver/dns_key.c | 1 +
net/rxrpc/key.c | 2 ++
security/keys/keyring.c | 7 ++++++-
6 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index e49d1de0614e..2148a6bf58f1 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -74,6 +74,9 @@ struct key_type {
*/
size_t def_datalen;
+ unsigned int flags;
+#define KEY_TYPE_NET_DOMAIN 0x00000001 /* Keys of this type have a net namespace domain */
+
/* vet a description */
int (*vet_description)(const char *description);
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 12689ddfc24c..a56bf7fc7c2b 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -71,6 +71,9 @@ struct net {
*/
struct llist_node cleanup_list; /* namespaces on death row */
+#ifdef CONFIG_KEYS
+ struct key_tag *key_domain; /* Key domain of operation tag */
+#endif
struct user_namespace *user_ns; /* Owning user namespace */
struct ucounts *ucounts;
spinlock_t nsid_lock;
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 711b161505ac..076a75c73c9e 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -38,9 +38,16 @@ EXPORT_SYMBOL_GPL(net_namespace_list);
DECLARE_RWSEM(net_rwsem);
EXPORT_SYMBOL_GPL(net_rwsem);
+#ifdef CONFIG_KEYS
+static struct key_tag init_net_key_domain = { .usage = REFCOUNT_INIT(1) };
+#endif
+
struct net init_net = {
.count = REFCOUNT_INIT(1),
.dev_base_head = LIST_HEAD_INIT(init_net.dev_base_head),
+#ifdef CONFIG_KEYS
+ .key_domain = &init_net_key_domain,
+#endif
};
EXPORT_SYMBOL(init_net);
@@ -386,10 +393,21 @@ static struct net *net_alloc(void)
if (!net)
goto out_free;
+#ifdef CONFIG_KEYS
+ net->key_domain = kzalloc(sizeof(struct key_tag), GFP_KERNEL);
+ if (!net->key_domain)
+ goto out_free_2;
+ refcount_set(&net->key_domain->usage, 1);
+#endif
+
rcu_assign_pointer(net->gen, ng);
out:
return net;
+#ifdef CONFIG_KEYS
+out_free_2:
+ kmem_cache_free(net_cachep, net);
+#endif
out_free:
kfree(ng);
goto out;
@@ -566,6 +584,7 @@ static void cleanup_net(struct work_struct *work)
list_for_each_entry_safe(net, tmp, &net_exit_list, exit_list) {
list_del_init(&net->exit_list);
dec_net_namespaces(net->ucounts);
+ key_remove_domain(net->key_domain);
put_user_ns(net->user_ns);
net_drop_ns(net);
}
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index a65d553e730d..3e1a90669006 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -314,6 +314,7 @@ static long dns_resolver_read(const struct key *key,
struct key_type key_type_dns_resolver = {
.name = "dns_resolver",
+ .flags = KEY_TYPE_NET_DOMAIN,
.preparse = dns_resolver_preparse,
.free_preparse = dns_resolver_free_preparse,
.instantiate = generic_key_instantiate,
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index e7f6b8823eb6..2722189ec273 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -43,6 +43,7 @@ static long rxrpc_read(const struct key *, char __user *, size_t);
*/
struct key_type key_type_rxrpc = {
.name = "rxrpc",
+ .flags = KEY_TYPE_NET_DOMAIN,
.preparse = rxrpc_preparse,
.free_preparse = rxrpc_free_preparse,
.instantiate = generic_key_instantiate,
@@ -58,6 +59,7 @@ EXPORT_SYMBOL(key_type_rxrpc);
*/
struct key_type key_type_rxrpc_s = {
.name = "rxrpc_s",
+ .flags = KEY_TYPE_NET_DOMAIN,
.vet_description = rxrpc_vet_description_s,
.preparse = rxrpc_preparse_s,
.free_preparse = rxrpc_free_preparse_s,
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index d3c86fda1510..bca070f6ab46 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -17,10 +17,12 @@
#include <linux/seq_file.h>
#include <linux/err.h>
#include <linux/user_namespace.h>
+#include <linux/nsproxy.h>
#include <keys/keyring-type.h>
#include <keys/user-type.h>
#include <linux/assoc_array_priv.h>
#include <linux/uaccess.h>
+#include <net/net_namespace.h>
#include "internal.h"
/*
@@ -220,7 +222,10 @@ void key_set_index_key(struct keyring_index_key *index_key)
memcpy(index_key->desc, index_key->description, n);
- index_key->domain_tag = &default_domain_tag;
+ if (index_key->type->flags & KEY_TYPE_NET_DOMAIN)
+ index_key->domain_tag = current->nsproxy->net_ns->key_domain;
+ else
+ index_key->domain_tag = &default_domain_tag;
hash_key_type_and_desc(index_key);
}
^ permalink raw reply related
* [PATCH 9/9] keys: Pass the network namespace into request_key mechanism [ver #4]
From: David Howells @ 2019-06-19 16:48 UTC (permalink / raw)
To: ebiederm, keyrings
Cc: linux-cifs, linux-nfs, netdev, linux-afs, dhowells, dwalsh,
vgoyal, linux-security-module, linux-fsdevel, linux-kernel
In-Reply-To: <156096279115.28733.8761881995303698232.stgit@warthog.procyon.org.uk>
Create a request_key_net() function and use it to pass the network
namespace domain tag into DNS revolver keys and rxrpc/AFS keys so that keys
for different domains can coexist in the same keyring.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: netdev@vger.kernel.org
cc: linux-nfs@vger.kernel.org
cc: linux-cifs@vger.kernel.org
cc: linux-afs@lists.infradead.org
---
Documentation/security/keys/core.rst | 28 +++++++++++++---
Documentation/security/keys/request-key.rst | 29 ++++++++++++-----
fs/afs/addr_list.c | 4 +-
fs/afs/dynroot.c | 8 +++--
fs/cifs/dns_resolve.c | 3 +-
fs/nfs/dns_resolve.c | 3 +-
fs/nfs/nfs4idmap.c | 2 +
include/linux/dns_resolver.h | 3 +-
include/linux/key.h | 47 +++++++++++++++++++++++++--
net/ceph/messenger.c | 3 +-
net/dns_resolver/dns_query.c | 7 +++-
net/rxrpc/key.c | 4 +-
security/keys/internal.h | 1 +
security/keys/keyctl.c | 2 +
security/keys/keyring.c | 11 ++++--
security/keys/request_key.c | 39 ++++++++++++++++------
16 files changed, 145 insertions(+), 49 deletions(-)
diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
index ae930ae9d590..0e74f372e58c 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -1102,26 +1102,42 @@ payload contents" for more information.
See also Documentation/security/keys/request-key.rst.
+ * To search for a key in a specific domain, call:
+
+ struct key *request_key_tag(const struct key_type *type,
+ const char *description,
+ struct key_tag *domain_tag,
+ const char *callout_info);
+
+ This is identical to request_key(), except that a domain tag may be
+ specifies that causes search algorithm to only match keys matching that
+ tag. The domain_tag may be NULL, specifying a global domain that is
+ separate from any nominated domain.
+
+
* To search for a key, passing auxiliary data to the upcaller, call::
struct key *request_key_with_auxdata(const struct key_type *type,
const char *description,
+ struct key_tag *domain_tag,
const void *callout_info,
size_t callout_len,
void *aux);
- This is identical to request_key(), except that the auxiliary data is
- passed to the key_type->request_key() op if it exists, and the callout_info
- is a blob of length callout_len, if given (the length may be 0).
+ This is identical to request_key_tag(), except that the auxiliary data is
+ passed to the key_type->request_key() op if it exists, and the
+ callout_info is a blob of length callout_len, if given (the length may be
+ 0).
* To search for a key under RCU conditions, call::
struct key *request_key_rcu(const struct key_type *type,
- const char *description);
+ const char *description,
+ struct key_tag *domain_tag);
- which is similar to request_key() except that it does not check for keys
- that are under construction and it will not call out to userspace to
+ which is similar to request_key_tag() except that it does not check for
+ keys that are under construction and it will not call out to userspace to
construct a key if it can't find a match.
diff --git a/Documentation/security/keys/request-key.rst b/Documentation/security/keys/request-key.rst
index 5a210baa583a..35f2296b704a 100644
--- a/Documentation/security/keys/request-key.rst
+++ b/Documentation/security/keys/request-key.rst
@@ -13,10 +13,18 @@ The process starts by either the kernel requesting a service by calling
const char *description,
const char *callout_info);
+or::
+
+ struct key *request_key_tag(const struct key_type *type,
+ const char *description,
+ const struct key_tag *domain_tag,
+ const char *callout_info);
+
or::
struct key *request_key_with_auxdata(const struct key_type *type,
const char *description,
+ const struct key_tag *domain_tag,
const char *callout_info,
size_t callout_len,
void *aux);
@@ -24,7 +32,8 @@ or::
or::
struct key *request_key_rcu(const struct key_type *type,
- const char *description);
+ const char *description,
+ const struct key_tag *domain_tag);
Or by userspace invoking the request_key system call::
@@ -38,14 +47,18 @@ does not need to link the key to a keyring to prevent it from being immediately
destroyed. The kernel interface returns a pointer directly to the key, and
it's up to the caller to destroy the key.
-The request_key_with_auxdata() calls is like the in-kernel request_key() call,
-except that they permit auxiliary data to be passed to the upcaller (the
-default is NULL). This is only useful for those key types that define their
-own upcall mechanism rather than using /sbin/request-key.
+The request_key_tag() call is like the in-kernel request_key(), except that it
+also takes a domain tag that allows keys to be separated by namespace and
+killed off as a group.
+
+The request_key_with_auxdata() calls is like the request_key_tag() call, except
+that they permit auxiliary data to be passed to the upcaller (the default is
+NULL). This is only useful for those key types that define their own upcall
+mechanism rather than using /sbin/request-key.
-The request_key_rcu() call is like the in-kernel request_key() call, except
-that it doesn't check for keys that are under construction and doesn't attempt
-to construct missing keys.
+The request_key_rcu() call is like the request_key_tag() call, except that it
+doesn't check for keys that are under construction and doesn't attempt to
+construct missing keys.
The userspace interface links the key to a keyring associated with the process
to prevent the key from going away, and returns the serial number of the key to
diff --git a/fs/afs/addr_list.c b/fs/afs/addr_list.c
index 9eaff55df7b4..6b1e8fc6c954 100644
--- a/fs/afs/addr_list.c
+++ b/fs/afs/addr_list.c
@@ -250,8 +250,8 @@ struct afs_vlserver_list *afs_dns_query(struct afs_cell *cell, time64_t *_expiry
_enter("%s", cell->name);
- ret = dns_query("afsdb", cell->name, cell->name_len, "srv=1",
- &result, _expiry, true);
+ ret = dns_query(cell->net->net, "afsdb", cell->name, cell->name_len,
+ "srv=1", &result, _expiry, true);
if (ret < 0) {
_leave(" = %d [dns]", ret);
return ERR_PTR(ret);
diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
index af1689d1f32e..b075605b0c45 100644
--- a/fs/afs/dynroot.c
+++ b/fs/afs/dynroot.c
@@ -28,6 +28,7 @@ const struct file_operations afs_dynroot_file_operations = {
static int afs_probe_cell_name(struct dentry *dentry)
{
struct afs_cell *cell;
+ struct afs_net *net = afs_d2net(dentry);
const char *name = dentry->d_name.name;
size_t len = dentry->d_name.len;
int ret;
@@ -40,13 +41,14 @@ static int afs_probe_cell_name(struct dentry *dentry)
len--;
}
- cell = afs_lookup_cell_rcu(afs_d2net(dentry), name, len);
+ cell = afs_lookup_cell_rcu(net, name, len);
if (!IS_ERR(cell)) {
- afs_put_cell(afs_d2net(dentry), cell);
+ afs_put_cell(net, cell);
return 0;
}
- ret = dns_query("afsdb", name, len, "srv=1", NULL, NULL, false);
+ ret = dns_query(net->net, "afsdb", name, len, "srv=1",
+ NULL, NULL, false);
if (ret == -ENODATA)
ret = -EDESTADDRREQ;
return ret;
diff --git a/fs/cifs/dns_resolve.c b/fs/cifs/dns_resolve.c
index 1e21b2528cfb..534cbba72789 100644
--- a/fs/cifs/dns_resolve.c
+++ b/fs/cifs/dns_resolve.c
@@ -77,7 +77,8 @@ dns_resolve_server_name_to_ip(const char *unc, char **ip_addr)
goto name_is_IP_address;
/* Perform the upcall */
- rc = dns_query(NULL, hostname, len, NULL, ip_addr, NULL, false);
+ rc = dns_query(current->nsproxy->net_ns, NULL, hostname, len,
+ NULL, ip_addr, NULL, false);
if (rc < 0)
cifs_dbg(FYI, "%s: unable to resolve: %*.*s\n",
__func__, len, len, hostname);
diff --git a/fs/nfs/dns_resolve.c b/fs/nfs/dns_resolve.c
index e6a700f01452..aec769a500a1 100644
--- a/fs/nfs/dns_resolve.c
+++ b/fs/nfs/dns_resolve.c
@@ -22,7 +22,8 @@ ssize_t nfs_dns_resolve_name(struct net *net, char *name, size_t namelen,
char *ip_addr = NULL;
int ip_len;
- ip_len = dns_query(NULL, name, namelen, NULL, &ip_addr, NULL, false);
+ ip_len = dns_query(net, NULL, name, namelen, NULL, &ip_addr, NULL,
+ false);
if (ip_len > 0)
ret = rpc_pton(net, ip_addr, ip_len, sa, salen);
else
diff --git a/fs/nfs/nfs4idmap.c b/fs/nfs/nfs4idmap.c
index 4884fdae28fb..1e7296395d71 100644
--- a/fs/nfs/nfs4idmap.c
+++ b/fs/nfs/nfs4idmap.c
@@ -291,7 +291,7 @@ static struct key *nfs_idmap_request_key(const char *name, size_t namelen,
if (IS_ERR(rkey)) {
mutex_lock(&idmap->idmap_mutex);
rkey = request_key_with_auxdata(&key_type_id_resolver_legacy,
- desc, "", 0, idmap);
+ desc, NULL, "", 0, idmap);
mutex_unlock(&idmap->idmap_mutex);
}
if (!IS_ERR(rkey))
diff --git a/include/linux/dns_resolver.h b/include/linux/dns_resolver.h
index f2b3ae22e6b7..976cbbdb2832 100644
--- a/include/linux/dns_resolver.h
+++ b/include/linux/dns_resolver.h
@@ -26,7 +26,8 @@
#include <uapi/linux/dns_resolver.h>
-extern int dns_query(const char *type, const char *name, size_t namelen,
+struct net;
+extern int dns_query(struct net *net, const char *type, const char *name, size_t namelen,
const char *options, char **_result, time64_t *_expiry,
bool invalidate);
diff --git a/include/linux/key.h b/include/linux/key.h
index 60c076c6e47f..18d7f62ab6b0 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -36,6 +36,7 @@ typedef int32_t key_serial_t;
typedef uint32_t key_perm_t;
struct key;
+struct net;
#ifdef CONFIG_KEYS
@@ -296,19 +297,57 @@ static inline void key_ref_put(key_ref_t key_ref)
key_put(key_ref_to_ptr(key_ref));
}
-extern struct key *request_key(struct key_type *type,
- const char *description,
- const char *callout_info);
+extern struct key *request_key_tag(struct key_type *type,
+ const char *description,
+ struct key_tag *domain_tag,
+ const char *callout_info);
extern struct key *request_key_rcu(struct key_type *type,
- const char *description);
+ const char *description,
+ struct key_tag *domain_tag);
extern struct key *request_key_with_auxdata(struct key_type *type,
const char *description,
+ struct key_tag *domain_tag,
const void *callout_info,
size_t callout_len,
void *aux);
+/**
+ * request_key - Request a key and wait for construction
+ * @type: Type of key.
+ * @description: The searchable description of the key.
+ * @callout_info: The data to pass to the instantiation upcall (or NULL).
+ *
+ * As for request_key_tag(), but with the default global domain tag.
+ */
+static inline struct key *request_key(struct key_type *type,
+ const char *description,
+ const char *callout_info)
+{
+ return request_key_tag(type, description, NULL, callout_info);
+}
+
+#ifdef CONFIG_NET
+/*
+ * request_key_net - Request a key for a net namespace and wait for construction
+ * @type: Type of key.
+ * @description: The searchable description of the key.
+ * @net: The network namespace that is the key's domain of operation.
+ * @callout_info: The data to pass to the instantiation upcall (or NULL).
+ *
+ * As for request_key() except that it does not add the returned key to a
+ * keyring if found, new keys are always allocated in the user's quota, the
+ * callout_info must be a NUL-terminated string and no auxiliary data can be
+ * passed. Only keys that operate the specified network namespace are used.
+ *
+ * Furthermore, it then works as wait_for_key_construction() to wait for the
+ * completion of keys undergoing construction with a non-interruptible wait.
+ */
+#define request_key_net(type, description, net, callout_info) \
+ request_key_tag(type, description, net->key_domain, callout_info);
+#endif /* CONFIG_NET */
+
extern int wait_for_key_construction(struct key *key, bool intr);
extern int key_validate(const struct key *key);
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index cd0b094468b6..a33402c99321 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1887,7 +1887,8 @@ static int ceph_dns_resolve_name(const char *name, size_t namelen,
return -EINVAL;
/* do dns_resolve upcall */
- ip_len = dns_query(NULL, name, end - name, NULL, &ip_addr, NULL, false);
+ ip_len = dns_query(current->nsproxy->net_ns,
+ NULL, name, end - name, NULL, &ip_addr, NULL, false);
if (ip_len > 0)
ret = ceph_pton(ip_addr, ip_len, addr, -1, NULL);
else
diff --git a/net/dns_resolver/dns_query.c b/net/dns_resolver/dns_query.c
index 2d260432b3be..cab4e0df924f 100644
--- a/net/dns_resolver/dns_query.c
+++ b/net/dns_resolver/dns_query.c
@@ -40,6 +40,7 @@
#include <linux/cred.h>
#include <linux/dns_resolver.h>
#include <linux/err.h>
+#include <net/net_namespace.h>
#include <keys/dns_resolver-type.h>
#include <keys/user-type.h>
@@ -48,6 +49,7 @@
/**
* dns_query - Query the DNS
+ * @net: The network namespace to operate in.
* @type: Query type (or NULL for straight host->IP lookup)
* @name: Name to look up
* @namelen: Length of name
@@ -69,7 +71,8 @@
*
* Returns the size of the result on success, -ve error code otherwise.
*/
-int dns_query(const char *type, const char *name, size_t namelen,
+int dns_query(struct net *net,
+ const char *type, const char *name, size_t namelen,
const char *options, char **_result, time64_t *_expiry,
bool invalidate)
{
@@ -122,7 +125,7 @@ int dns_query(const char *type, const char *name, size_t namelen,
* add_key() to preinstall malicious redirections
*/
saved_cred = override_creds(dns_resolver_cache);
- rkey = request_key(&key_type_dns_resolver, desc, options);
+ rkey = request_key_net(&key_type_dns_resolver, desc, net, options);
revert_creds(saved_cred);
kfree(desc);
if (IS_ERR(rkey)) {
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 2722189ec273..1cc6b0c6cc42 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -914,7 +914,7 @@ int rxrpc_request_key(struct rxrpc_sock *rx, char __user *optval, int optlen)
if (IS_ERR(description))
return PTR_ERR(description);
- key = request_key(&key_type_rxrpc, description, NULL);
+ key = request_key_net(&key_type_rxrpc, description, sock_net(&rx->sk), NULL);
if (IS_ERR(key)) {
kfree(description);
_leave(" = %ld", PTR_ERR(key));
@@ -945,7 +945,7 @@ int rxrpc_server_keyring(struct rxrpc_sock *rx, char __user *optval,
if (IS_ERR(description))
return PTR_ERR(description);
- key = request_key(&key_type_keyring, description, NULL);
+ key = request_key_net(&key_type_keyring, description, sock_net(&rx->sk), NULL);
if (IS_ERR(key)) {
kfree(description);
_leave(" = %ld", PTR_ERR(key));
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 5a561f5f199e..f1f2b076f3a1 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -156,6 +156,7 @@ extern int install_session_keyring_to_cred(struct cred *, struct key *);
extern struct key *request_key_and_link(struct key_type *type,
const char *description,
+ struct key_tag *domain_tag,
const void *callout_info,
size_t callout_len,
void *aux,
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index aa9be531e5f5..8d115825198c 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -224,7 +224,7 @@ SYSCALL_DEFINE4(request_key, const char __user *, _type,
}
/* do the search */
- key = request_key_and_link(ktype, description, callout_info,
+ key = request_key_and_link(ktype, description, NULL, callout_info,
callout_len, NULL, key_ref_to_ptr(dest_ref),
KEY_ALLOC_IN_QUOTA);
if (IS_ERR(key)) {
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index bca070f6ab46..29c31585ed61 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -222,10 +222,13 @@ void key_set_index_key(struct keyring_index_key *index_key)
memcpy(index_key->desc, index_key->description, n);
- if (index_key->type->flags & KEY_TYPE_NET_DOMAIN)
- index_key->domain_tag = current->nsproxy->net_ns->key_domain;
- else
- index_key->domain_tag = &default_domain_tag;
+ if (!index_key->domain_tag) {
+ if (index_key->type->flags & KEY_TYPE_NET_DOMAIN)
+ index_key->domain_tag = current->nsproxy->net_ns->key_domain;
+ else
+ index_key->domain_tag = &default_domain_tag;
+ }
+
hash_key_type_and_desc(index_key);
}
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 9201ca96c4df..aa589d3c90e2 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -17,6 +17,7 @@
#include <linux/err.h>
#include <linux/keyctl.h>
#include <linux/slab.h>
+#include <net/net_namespace.h>
#include "internal.h"
#include <keys/request_key_auth-type.h>
@@ -533,16 +534,18 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
* request_key_and_link - Request a key and cache it in a keyring.
* @type: The type of key we want.
* @description: The searchable description of the key.
+ * @domain_tag: The domain in which the key operates.
* @callout_info: The data to pass to the instantiation upcall (or NULL).
* @callout_len: The length of callout_info.
* @aux: Auxiliary data for the upcall.
* @dest_keyring: Where to cache the key.
* @flags: Flags to key_alloc().
*
- * A key matching the specified criteria is searched for in the process's
- * keyrings and returned with its usage count incremented if found. Otherwise,
- * if callout_info is not NULL, a key will be allocated and some service
- * (probably in userspace) will be asked to instantiate it.
+ * A key matching the specified criteria (type, description, domain_tag) is
+ * searched for in the process's keyrings and returned with its usage count
+ * incremented if found. Otherwise, if callout_info is not NULL, a key will be
+ * allocated and some service (probably in userspace) will be asked to
+ * instantiate it.
*
* If successfully found or created, the key will be linked to the destination
* keyring if one is provided.
@@ -558,6 +561,7 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
*/
struct key *request_key_and_link(struct key_type *type,
const char *description,
+ struct key_tag *domain_tag,
const void *callout_info,
size_t callout_len,
void *aux,
@@ -566,6 +570,7 @@ struct key *request_key_and_link(struct key_type *type,
{
struct keyring_search_context ctx = {
.index_key.type = type,
+ .index_key.domain_tag = domain_tag,
.index_key.description = description,
.index_key.desc_len = strlen(description),
.cred = current_cred(),
@@ -672,9 +677,10 @@ int wait_for_key_construction(struct key *key, bool intr)
EXPORT_SYMBOL(wait_for_key_construction);
/**
- * request_key - Request a key and wait for construction
+ * request_key_tag - Request a key and wait for construction
* @type: Type of key.
* @description: The searchable description of the key.
+ * @domain_tag: The domain in which the key operates.
* @callout_info: The data to pass to the instantiation upcall (or NULL).
*
* As for request_key_and_link() except that it does not add the returned key
@@ -685,9 +691,10 @@ EXPORT_SYMBOL(wait_for_key_construction);
* Furthermore, it then works as wait_for_key_construction() to wait for the
* completion of keys undergoing construction with a non-interruptible wait.
*/
-struct key *request_key(struct key_type *type,
- const char *description,
- const char *callout_info)
+struct key *request_key_tag(struct key_type *type,
+ const char *description,
+ struct key_tag *domain_tag,
+ const char *callout_info)
{
struct key *key;
size_t callout_len = 0;
@@ -695,7 +702,8 @@ struct key *request_key(struct key_type *type,
if (callout_info)
callout_len = strlen(callout_info);
- key = request_key_and_link(type, description, callout_info, callout_len,
+ key = request_key_and_link(type, description, domain_tag,
+ callout_info, callout_len,
NULL, NULL, KEY_ALLOC_IN_QUOTA);
if (!IS_ERR(key)) {
ret = wait_for_key_construction(key, false);
@@ -706,12 +714,13 @@ struct key *request_key(struct key_type *type,
}
return key;
}
-EXPORT_SYMBOL(request_key);
+EXPORT_SYMBOL(request_key_tag);
/**
* request_key_with_auxdata - Request a key with auxiliary data for the upcaller
* @type: The type of key we want.
* @description: The searchable description of the key.
+ * @domain_tag: The domain in which the key operates.
* @callout_info: The data to pass to the instantiation upcall (or NULL).
* @callout_len: The length of callout_info.
* @aux: Auxiliary data for the upcall.
@@ -724,6 +733,7 @@ EXPORT_SYMBOL(request_key);
*/
struct key *request_key_with_auxdata(struct key_type *type,
const char *description,
+ struct key_tag *domain_tag,
const void *callout_info,
size_t callout_len,
void *aux)
@@ -731,7 +741,8 @@ struct key *request_key_with_auxdata(struct key_type *type,
struct key *key;
int ret;
- key = request_key_and_link(type, description, callout_info, callout_len,
+ key = request_key_and_link(type, description, domain_tag,
+ callout_info, callout_len,
aux, NULL, KEY_ALLOC_IN_QUOTA);
if (!IS_ERR(key)) {
ret = wait_for_key_construction(key, false);
@@ -748,6 +759,7 @@ EXPORT_SYMBOL(request_key_with_auxdata);
* request_key_rcu - Request key from RCU-read-locked context
* @type: The type of key we want.
* @description: The name of the key we want.
+ * @domain_tag: The domain in which the key operates.
*
* Request a key from a context that we may not sleep in (such as RCU-mode
* pathwalk). Keys under construction are ignored.
@@ -755,10 +767,13 @@ EXPORT_SYMBOL(request_key_with_auxdata);
* Return a pointer to the found key if successful, -ENOKEY if we couldn't find
* a key or some other error if the key found was unsuitable or inaccessible.
*/
-struct key *request_key_rcu(struct key_type *type, const char *description)
+struct key *request_key_rcu(struct key_type *type,
+ const char *description,
+ struct key_tag *domain_tag)
{
struct keyring_search_context ctx = {
.index_key.type = type,
+ .index_key.domain_tag = domain_tag,
.index_key.description = description,
.index_key.desc_len = strlen(description),
.cred = current_cred(),
^ permalink raw reply related
* Re: [PATCH v2 00/25] LSM: Module stacking for AppArmor
From: Casey Schaufler @ 2019-06-19 16:48 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds, casey
In-Reply-To: <201906182214.6DC4C1DB9@keescook>
On 6/18/2019 10:21 PM, Kees Cook wrote:
> On Tue, Jun 18, 2019 at 04:05:26PM -0700, Casey Schaufler wrote:
>> Patches 0004-0014 replace system use of a "secid" with
>> a structure "lsmblob" containing information from the
>> security modules to be held and reused later. At this
>> point lsmblob contains an array of u32 secids, one "slot"
>> for each of the security modules compiled into the
>> kernel that used secids. A "slot" is allocated when
>> a security module registers a hook for one of the interfaces
>> that uses a secid or a security context. The infrastructure
>> is changed to use the slot number to pass the correct
>> secid to or from the security module hooks.
> I found 14/25 in your git tree. Very satisfying to see all the
> scaffolding vanish for process_measurement() :)
>
> I like this progression in 4-14; I find it much much easier to review.
> My only complaint is the variable names. I think I'd prefer "blob" over
> "le" or "l", which are both contain very little information about what
> they are.
I know what they are! OK, I get it. Using "blob" would make it
more obvious. It's an relatively easy change, so I'll incorporate
it going forward.
^ permalink raw reply
* Re: [PATCH v2 15/25] LSM: Specify which LSM to display
From: Casey Schaufler @ 2019-06-19 17:00 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds, casey
In-Reply-To: <201906182221.8E3938600D@keescook>
On 6/18/2019 10:28 PM, Kees Cook wrote:
> On Tue, Jun 18, 2019 at 04:05:41PM -0700, Casey Schaufler wrote:
>> Create a new entry "display" in /proc/.../attr for controlling
>> which LSM security information is displayed for a process.
>> The name of an active LSM that supplies hooks for human readable
>> data may be written to "display" to set the value. The name of
>> the LSM currently in use can be read from "display".
>> At this point there can only be one LSM capable of display
>> active.
>>
>> This affects /proc/.../attr/current and SO_PEERSEC.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> fs/proc/base.c | 1 +
>> security/security.c | 108 +++++++++++++++++++++++++++++++++++---------
>> 2 files changed, 88 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index ddef482f1334..7bf70e041315 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2618,6 +2618,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>> ATTR(NULL, "fscreate", 0666),
>> ATTR(NULL, "keycreate", 0666),
>> ATTR(NULL, "sockcreate", 0666),
>> + ATTR(NULL, "display", 0666),
>> #ifdef CONFIG_SECURITY_SMACK
>> DIR("smack", 0555,
>> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
>> diff --git a/security/security.c b/security/security.c
>> index 46f6cf21d33c..9cfdc664239e 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -46,7 +46,9 @@ static struct kmem_cache *lsm_file_cache;
>> static struct kmem_cache *lsm_inode_cache;
>>
>> char *lsm_names;
>> -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
>> +static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
>> + .lbs_task = sizeof(int),
>> +};
> This needs some comments. I know what's happening here only because I
> understand very well how the blob sizing works. :) Perhaps:
>
> .lbs_task = sizeof(int), /* storage for selected "display" LSM */
Point.
>>
>> /* Boot-time LSM user choice */
>> static __initdata const char *chosen_lsm_order;
>> @@ -578,6 +580,8 @@ int lsm_inode_alloc(struct inode *inode)
>> */
>> static int lsm_task_alloc(struct task_struct *task)
>> {
>> + int *display;
>> +
>> if (blob_sizes.lbs_task == 0) {
>> task->security = NULL;
>> return 0;
>> @@ -586,6 +590,10 @@ static int lsm_task_alloc(struct task_struct *task)
>> task->security = kzalloc(blob_sizes.lbs_task, GFP_KERNEL);
>> if (task->security == NULL)
>> return -ENOMEM;
>> +
>> + display = task->security;
>> + *display = LSMDATA_INVALID;
> Similarly I think a comment here would be nice. "Initialize currently
> selected "display" LSM to unselected" or something.
Point.
> Also: "int" is okay here for now, but if the LSM infrastructure wants to
> do more like this we'll want to convert to a struct of some kind at the
> start of the lbs_task.
We could go whole hog and include a lsm_info pointer (once
the slot number moves there) instead of an int, but I think
it best to leave it as is for now. I don't see a case where
more information would be required, and it would not be a
hard change to make later.
>> +
>> return 0;
>> }
>>
>> @@ -1574,14 +1582,27 @@ int security_file_open(struct file *file)
>>
>> int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
>> {
>> + int *odisplay = current->security;
>> + int *ndisplay;
>> int rc = lsm_task_alloc(task);
>>
>> - if (rc)
>> + if (unlikely(rc))
>> return rc;
>> +
>> rc = call_int_hook(task_alloc, 0, task, clone_flags);
>> - if (unlikely(rc))
>> + if (unlikely(rc)) {
>> security_task_free(task);
>> - return rc;
>> + return rc;
>> + }
>> +
>> + ndisplay = task->security;
>> + if (ndisplay == NULL)
>> + return 0;
>> +
>> + if (odisplay != NULL)
> Perhaps merge these into "if (ndisplay && odisplay)" to drop the early
> return 0 check?
Sure. The logic took a few iterations before it got to
what you see here.
>> + *ndisplay = *odisplay;
>> +
>> + return 0;
>> }
>>
>> void security_task_free(struct task_struct *task)
>> @@ -1967,10 +1988,28 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>> char **value)
>> {
>> struct security_hook_list *hp;
>> + int *display = current->security;
>> +
>> + if (!strcmp(name, "display")) {
>> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx,
>> + list) {
>> + if (*display == LSMDATA_INVALID ||
>> + hp->slot == *display) {
>> + *value = kstrdup(hp->lsm, GFP_KERNEL);
>> + if (*value)
>> + return strlen(hp->lsm);
>> + return -ENOMEM;
>> + }
>> + }
>> + return -EINVAL;
>> + }
>>
>> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsm))
>> continue;
>> + if (lsm == NULL && *display != LSMDATA_INVALID &&
>> + *display != hp->slot)
>> + continue;
>> return hp->hook.getprocattr(p, name, value);
>> }
>> return -EINVAL;
>> @@ -1980,10 +2019,27 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>> size_t size)
>> {
>> struct security_hook_list *hp;
>> + int *display = current->security;
>> + int len;
>> +
>> + if (!strcmp(name, "display")) {
>> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx,
>> + list) {
>> + len = strlen(hp->lsm);
>> + if (size >= len && !strncmp(value, hp->lsm, len)) {
>> + *display = hp->slot;
>> + return size;
>> + }
>> + }
>> + return -EINVAL;
>> + }
>>
>> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsm))
>> continue;
>> + if (lsm == NULL && *display != LSMDATA_INVALID &&
>> + *display != hp->slot)
>> + continue;
>> return hp->hook.setprocattr(name, value, size);
>> }
>> return -EINVAL;
>> @@ -2002,38 +2058,41 @@ EXPORT_SYMBOL(security_ismaclabel);
>>
>> int security_secid_to_secctx(struct lsmblob *l, char **secdata, u32 *seclen)
>> {
>> + int *display = current->security;
>> struct security_hook_list *hp;
>> - int rc;
>>
>> - hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
>> - rc = hp->hook.secid_to_secctx(l->secid[hp->slot],
>> - secdata, seclen);
>> - if (rc != 0)
>> - return rc;
>> - }
>> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list)
>> + if (*display == LSMDATA_INVALID || *display == hp->slot)
>> + return hp->hook.secid_to_secctx(l->secid[hp->slot],
>> + secdata, seclen);
>> return 0;
>> }
>> EXPORT_SYMBOL(security_secid_to_secctx);
>>
>> int security_secctx_to_secid(const char *secdata, u32 seclen, struct lsmblob *l)
>> {
>> + int *display = current->security;
>> struct security_hook_list *hp;
>> - int rc;
>>
>> lsmblob_init(l, 0);
>> - hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
>> - rc = hp->hook.secctx_to_secid(secdata, seclen,
>> - &l->secid[hp->slot]);
>> - if (rc != 0)
>> - return rc;
>> - }
>> + hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list)
>> + if (*display == LSMDATA_INVALID || *display == hp->slot)
>> + return hp->hook.secctx_to_secid(secdata, seclen,
>> + &l->secid[hp->slot]);
>> return 0;
>> }
>> EXPORT_SYMBOL(security_secctx_to_secid);
>>
>> void security_release_secctx(char *secdata, u32 seclen)
>> {
>> - call_void_hook(release_secctx, secdata, seclen);
>> + int *display = current->security;
>> + struct security_hook_list *hp;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
>> + if (*display == LSMDATA_INVALID || *display == hp->slot) {
>> + hp->hook.release_secctx(secdata, seclen);
>> + return;
>> + }
>> }
>> EXPORT_SYMBOL(security_release_secctx);
>>
>> @@ -2158,8 +2217,15 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
>> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
>> int __user *optlen, unsigned len)
>> {
>> - return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
>> - optval, optlen, len);
>> + int *display = current->security;
>> + struct security_hook_list *hp;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
>> + list)
>> + if (*display == LSMDATA_INVALID || *display == hp->slot)
>> + return hp->hook.socket_getpeersec_stream(sock, optval,
>> + optlen, len);
>> + return -ENOPROTOOPT;
>> }
>>
>> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
>> --
>> 2.20.1
>>
^ permalink raw reply
* Re: [PATCH v2 16/25] LSM: Ensure the correct LSM context releaser
From: Casey Schaufler @ 2019-06-19 17:10 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds, casey
In-Reply-To: <201906182231.349D240@keescook>
On 6/18/2019 10:34 PM, Kees Cook wrote:
> On Tue, Jun 18, 2019 at 04:05:42PM -0700, Casey Schaufler wrote:
>> Add a new lsmcontext data structure to hold all the information
>> about a "security context", including the string, its size and
>> which LSM allocated the string. The allocation information is
>> necessary because LSMs have different policies regarding the
>> lifecycle of these strings. SELinux allocates and destroys
>> them on each use, whereas Smack provides a pointer to an entry
>> in a list that never goes away.
>>
>> Change the security_release_secctx() interface to use the
>> lsmcontext and call only the appropiate LSM hook. Change
>> the callers of security_release_secctx() to provide the
>> correct type of data, introducing scaffolding where required.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> drivers/android/binder.c | 10 +++++--
>> fs/kernfs/dir.c | 9 ++++--
>> fs/kernfs/inode.c | 7 +++--
>> fs/nfs/nfs4proc.c | 8 ++++--
>> fs/nfsd/nfs4xdr.c | 7 +++--
>> include/linux/security.h | 37 +++++++++++++++++++++++--
>> include/net/scm.h | 4 ++-
>> kernel/audit.c | 14 +++++++---
>> kernel/auditsc.c | 12 ++++++--
>> net/ipv4/ip_sockglue.c | 4 ++-
>> net/netfilter/nf_conntrack_netlink.c | 4 ++-
>> net/netfilter/nf_conntrack_standalone.c | 4 ++-
>> net/netfilter/nfnetlink_queue.c | 13 ++++++---
>> net/netlabel/netlabel_unlabeled.c | 19 ++++++++++---
>> net/netlabel/netlabel_user.c | 4 ++-
>> security/security.c | 12 +++++---
>> security/smack/smack_lsm.c | 14 +++++++---
>> 17 files changed, 140 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 9eb790200fba..f11b5ca5bc30 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2876,6 +2876,7 @@ static void binder_transaction(struct binder_proc *proc,
>> int t_debug_id = atomic_inc_return(&binder_last_id);
>> char *secctx = NULL;
>> u32 secctx_sz = 0;
>> + struct lsmcontext scaff; /* scaffolding */
>>
>> e = binder_transaction_log_add(&binder_transaction_log);
>> e->debug_id = t_debug_id;
>> @@ -3158,7 +3159,8 @@ static void binder_transaction(struct binder_proc *proc,
>> binder_alloc_copy_to_buffer(&target_proc->alloc,
>> t->buffer, buf_offset,
>> secctx, secctx_sz);
>> - security_release_secctx(secctx, secctx_sz);
>> + lsmcontext_init(&scaff, secctx, secctx_sz, 0);
>> + security_release_secctx(&scaff);
>> secctx = NULL;
>> }
>> t->buffer->debug_id = t->debug_id;
>> @@ -3479,8 +3481,10 @@ static void binder_transaction(struct binder_proc *proc,
>> t->buffer->transaction = NULL;
>> binder_alloc_free_buf(&target_proc->alloc, t->buffer);
>> err_binder_alloc_buf_failed:
>> - if (secctx)
>> - security_release_secctx(secctx, secctx_sz);
>> + if (secctx) {
>> + lsmcontext_init(&scaff, secctx, secctx_sz, 0);
>> + security_release_secctx(&scaff);
>> + }
>> err_get_secctx_failed:
>> kfree(tcomplete);
>> binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
>> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
>> index b84d635567d3..92afad387237 100644
>> --- a/fs/kernfs/dir.c
>> +++ b/fs/kernfs/dir.c
>> @@ -532,9 +532,12 @@ void kernfs_put(struct kernfs_node *kn)
>> kfree_const(kn->name);
>>
>> if (kn->iattr) {
>> - if (kn->iattr->ia_secdata)
>> - security_release_secctx(kn->iattr->ia_secdata,
>> - kn->iattr->ia_secdata_len);
>> + struct lsmcontext scaff; /* scaffolding */
>> + if (kn->iattr->ia_secdata) {
>> + lsmcontext_init(&scaff, kn->iattr->ia_secdata,
>> + kn->iattr->ia_secdata_len, 0);
>> + security_release_secctx(&scaff);
>> + }
>> simple_xattrs_free(&kn->iattr->xattrs);
>> kmem_cache_free(kernfs_iattrs_cache, kn->iattr);
>> }
>> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
>> index 0c1fd945ce42..02cde9dac5ee 100644
>> --- a/fs/kernfs/inode.c
>> +++ b/fs/kernfs/inode.c
>> @@ -349,6 +349,7 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
>> {
>> struct kernfs_node *kn = inode->i_private;
>> struct kernfs_iattrs *attrs;
>> + struct lsmcontext context;
>> void *secdata;
>> u32 secdata_len = 0;
>> int error;
>> @@ -368,8 +369,10 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
>> error = kernfs_node_setsecdata(attrs, &secdata, &secdata_len);
>> mutex_unlock(&kernfs_mutex);
>>
>> - if (secdata)
>> - security_release_secctx(secdata, secdata_len);
>> + if (secdata) {
>> + lsmcontext_init(&context, secdata, secdata_len, 0);
>> + security_release_secctx(&context);
>> + }
>> return error;
>> }
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 4dbb0ee23432..af1c0db29c39 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -131,8 +131,12 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
>> static inline void
>> nfs4_label_release_security(struct nfs4_label *label)
>> {
>> - if (label)
>> - security_release_secctx(label->label, label->len);
>> + struct lsmcontext scaff; /* scaffolding */
>> +
>> + if (label) {
>> + lsmcontext_init(&scaff, label->label, label->len, 0);
>> + security_release_secctx(&scaff);
>> + }
>> }
>> static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
>> {
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 3de42a729093..bb3db033e144 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -2420,6 +2420,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>> __be32 status;
>> int err;
>> struct nfs4_acl *acl = NULL;
>> + struct lsmcontext scaff; /* scaffolding */
>> void *context = NULL;
>> int contextlen;
>> bool contextsupport = false;
>> @@ -2919,8 +2920,10 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>>
>> out:
>> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> - if (context)
>> - security_release_secctx(context, contextlen);
>> + if (context) {
>> + lsmcontext_init(&scaff, context, contextlen, 0); /*scaffolding*/
>> + security_release_secctx(&scaff);
>> + }
>> #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
>> kfree(acl);
>> if (tempfh) {
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 07a239292e02..8bd4f28ef532 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -76,6 +76,39 @@ enum lsm_event {
>> LSM_POLICY_CHANGE,
>> };
>>
>> +/*
>> + * A "security context" is the text representation of
>> + * the information used by LSMs.
>> + * This structure contains the string, its length, and which LSM
>> + * it is useful for.
>> + */
>> +struct lsmcontext {
>> + char *context; /* Provided by the module */
>> + u32 len;
>> + int slot; /* Identifies the module */
>> +};
>> +
>> +/**
>> + * lsmcontext_init - initialize an lsmcontext structure.
>> + * @cp: Pointer to the context to initialize
>> + * @context: Initial context, or NULL
>> + * @size: Size of context, or 0
>> + * @slot: Which LSM provided the context
>> + *
>> + * Fill in the lsmcontext from the provided information.
>> + */
>> +static inline void lsmcontext_init(struct lsmcontext *cp, char *context,
>> + u32 size, int slot)
>> +{
>> + cp->slot = slot;
>> + cp->context = context;
>> +
>> + if (context == NULL || size == 0)
>> + cp->len = 0;
>> + else
>> + cp->len = strlen(context);
>> +}
> I worry about the use of the "size" argument (or rather, lack of use).
> If all contexts are going to be NUL-terminated strings, there should be
> no "size" arg. If not, then "size" (or strneln(context, size)) should
> be used instead of strlen().
Once the scaffolding uses go away this is only ever called once,
with lsmcontext_init(cp, NULL, 0, 0) in security_release_secctx().
It probably makes the most sense to identify this as "for scaffolding"
and delete it when it is no longer needed.
>> +
>> /*
>> * Data exported by the security modules
>> */
>> @@ -445,7 +478,7 @@ int security_ismaclabel(const char *name);
>> int security_secid_to_secctx(struct lsmblob *l, char **secdata, u32 *seclen);
>> int security_secctx_to_secid(const char *secdata, u32 seclen,
>> struct lsmblob *l);
>> -void security_release_secctx(char *secdata, u32 seclen);
>> +void security_release_secctx(struct lsmcontext *cp);
>>
>> void security_inode_invalidate_secctx(struct inode *inode);
>> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
>> @@ -1236,7 +1269,7 @@ static inline int security_secctx_to_secid(const char *secdata,
>> return -EOPNOTSUPP;
>> }
>>
>> -static inline void security_release_secctx(char *secdata, u32 seclen)
>> +static inline void security_release_secctx(struct lsmcontext *cp)
>> {
>> }
>>
>> diff --git a/include/net/scm.h b/include/net/scm.h
>> index bcb0f8560cdf..d3e0ac961a11 100644
>> --- a/include/net/scm.h
>> +++ b/include/net/scm.h
>> @@ -92,6 +92,7 @@ static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
>> #ifdef CONFIG_SECURITY_NETWORK
>> static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
>> {
>> + struct lsmcontext context;
>> char *secdata;
>> u32 seclen;
>> int err;
>> @@ -101,7 +102,8 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
>>
>> if (!err) {
>> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
>> - security_release_secctx(secdata, seclen);
>> + lsmcontext_init(&context, secdata, seclen, 0);/*scaffolding*/
>> + security_release_secctx(&context);
>> }
>> }
>> }
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index a52f8772477f..0467b2d284fa 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -1193,6 +1193,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>> struct audit_sig_info *sig_data;
>> char *ctx = NULL;
>> u32 len;
>> + struct lsmcontext scaff; /* scaffolding */
>>
>> err = audit_netlink_ok(skb, msg_type);
>> if (err)
>> @@ -1437,15 +1438,18 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>> }
>> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
>> if (!sig_data) {
>> - if (lsmblob_is_set(&audit_sig_lsm))
>> - security_release_secctx(ctx, len);
>> + if (lsmblob_is_set(&audit_sig_lsm)) {
>> + lsmcontext_init(&scaff, ctx, len, 0);
>> + security_release_secctx(&scaff);
>> + }
>> return -ENOMEM;
>> }
>> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
>> sig_data->pid = audit_sig_pid;
>> if (lsmblob_is_set(&audit_sig_lsm)) {
>> memcpy(sig_data->ctx, ctx, len);
>> - security_release_secctx(ctx, len);
>> + lsmcontext_init(&scaff, ctx, len, 0);
>> + security_release_secctx(&scaff);
>> }
>> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
>> sig_data, sizeof(*sig_data) + len);
>> @@ -2074,6 +2078,7 @@ int audit_log_task_context(struct audit_buffer *ab)
>> unsigned len;
>> int error;
>> struct lsmblob le;
>> + struct lsmcontext scaff; /* scaffolding */
>>
>> security_task_getsecid(current, &le);
>> if (!lsmblob_is_set(&le))
>> @@ -2087,7 +2092,8 @@ int audit_log_task_context(struct audit_buffer *ab)
>> }
>>
>> audit_log_format(ab, " subj=%s", ctx);
>> - security_release_secctx(ctx, len);
>> + lsmcontext_init(&scaff, ctx, len, 0);
>> + security_release_secctx(&scaff);
>> return 0;
>>
>> error_path:
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index ebdd7eab9247..917e7550767a 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -942,6 +942,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>> struct lsmblob *l, char *comm)
>> {
>> struct audit_buffer *ab;
>> + struct lsmcontext lsmcxt;
>> char *ctx = NULL;
>> u32 len;
>> int rc = 0;
>> @@ -959,7 +960,8 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>> rc = 1;
>> } else {
>> audit_log_format(ab, " obj=%s", ctx);
>> - security_release_secctx(ctx, len);
>> + lsmcontext_init(&lsmcxt, ctx, len, 0); /*scaffolding*/
>> + security_release_secctx(&lsmcxt);
>> }
>> }
>> audit_log_format(ab, " ocomm=");
>> @@ -1171,6 +1173,7 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
>>
>> static void show_special(struct audit_context *context, int *call_panic)
>> {
>> + struct lsmcontext lsmcxt;
>> struct audit_buffer *ab;
>> int i;
>>
>> @@ -1203,7 +1206,8 @@ static void show_special(struct audit_context *context, int *call_panic)
>> *call_panic = 1;
>> } else {
>> audit_log_format(ab, " obj=%s", ctx);
>> - security_release_secctx(ctx, len);
>> + lsmcontext_init(&lsmcxt, ctx, len, 0);
>> + security_release_secctx(&lsmcxt);
>> }
>> }
>> if (context->ipc.has_perm) {
>> @@ -1350,6 +1354,7 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
>> char *ctx = NULL;
>> u32 len;
>> struct lsmblob le;
>> + struct lsmcontext lsmcxt;
>>
>> lsmblob_init(&le, n->osid);
>> if (security_secid_to_secctx(&le, &ctx, &len)) {
>> @@ -1358,7 +1363,8 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
>> *call_panic = 2;
>> } else {
>> audit_log_format(ab, " obj=%s", ctx);
>> - security_release_secctx(ctx, len);
>> + lsmcontext_init(&lsmcxt, ctx, len, 0); /* scaffolding */
>> + security_release_secctx(&lsmcxt);
>> }
>> }
>>
>> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
>> index e05f4ef68bd8..7834c357b60b 100644
>> --- a/net/ipv4/ip_sockglue.c
>> +++ b/net/ipv4/ip_sockglue.c
>> @@ -130,6 +130,7 @@ static void ip_cmsg_recv_checksum(struct msghdr *msg, struct sk_buff *skb,
>>
>> static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
>> {
>> + struct lsmcontext context;
>> struct lsmblob lb;
>> char *secdata;
>> u32 seclen;
>> @@ -144,7 +145,8 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
>> return;
>>
>> put_cmsg(msg, SOL_IP, SCM_SECURITY, seclen, secdata);
>> - security_release_secctx(secdata, seclen);
>> + lsmcontext_init(&context, secdata, seclen, 0); /* scaffolding */
>> + security_release_secctx(&context);
>> }
>>
>> static void ip_cmsg_recv_dstaddr(struct msghdr *msg, struct sk_buff *skb)
>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>> index 6098b586da07..93f308b5845d 100644
>> --- a/net/netfilter/nf_conntrack_netlink.c
>> +++ b/net/netfilter/nf_conntrack_netlink.c
>> @@ -331,6 +331,7 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
>> int len, ret;
>> char *secctx;
>> struct lsmblob le;
>> + struct lsmcontext context;
>>
>> lsmblob_init(&le, ct->secmark);
>> ret = security_secid_to_secctx(&le, &secctx, &len);
>> @@ -348,7 +349,8 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
>>
>> ret = 0;
>> nla_put_failure:
>> - security_release_secctx(secctx, len);
>> + lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
>> + security_release_secctx(&context);
>> return ret;
>> }
>> #else
>> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
>> index 6e6fb1f9f6ba..0bde6a4426e3 100644
>> --- a/net/netfilter/nf_conntrack_standalone.c
>> +++ b/net/netfilter/nf_conntrack_standalone.c
>> @@ -176,6 +176,7 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
>> u32 len;
>> char *secctx;
>> struct lsmblob le;
>> + struct lsmcontext context;
>>
>> lsmblob_init(&le, ct->secmark);
>> ret = security_secid_to_secctx(&le, &secctx, &len);
>> @@ -184,7 +185,8 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
>>
>> seq_printf(s, "secctx=%s ", secctx);
>>
>> - security_release_secctx(secctx, len);
>> + lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
>> + security_release_secctx(&context);
>> }
>> #else
>> static inline void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
>> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
>> index 105018d19318..ba767bdd1a9a 100644
>> --- a/net/netfilter/nfnetlink_queue.c
>> +++ b/net/netfilter/nfnetlink_queue.c
>> @@ -399,6 +399,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> enum ip_conntrack_info uninitialized_var(ctinfo);
>> struct nfnl_ct_hook *nfnl_ct;
>> bool csum_verify;
>> + struct lsmcontext scaff; /* scaffolding */
>> char *secdata = NULL;
>> u32 seclen = 0;
>>
>> @@ -629,8 +630,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> }
>>
>> nlh->nlmsg_len = skb->len;
>> - if (seclen)
>> - security_release_secctx(secdata, seclen);
>> + if (seclen) {
>> + lsmcontext_init(&scaff, secdata, seclen, 0);
>> + security_release_secctx(&scaff);
>> + }
>> return skb;
>>
>> nla_put_failure:
>> @@ -638,8 +641,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> kfree_skb(skb);
>> net_err_ratelimited("nf_queue: error creating packet message\n");
>> nlmsg_failure:
>> - if (seclen)
>> - security_release_secctx(secdata, seclen);
>> + if (seclen) {
>> + lsmcontext_init(&scaff, secdata, seclen, 0);
>> + security_release_secctx(&scaff);
>> + }
>> return NULL;
>> }
>>
>> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
>> index 57e0f81a2ec5..2f8c7415b6ff 100644
>> --- a/net/netlabel/netlabel_unlabeled.c
>> +++ b/net/netlabel/netlabel_unlabeled.c
>> @@ -387,6 +387,7 @@ int netlbl_unlhsh_add(struct net *net,
>> struct net_device *dev;
>> struct netlbl_unlhsh_iface *iface;
>> struct audit_buffer *audit_buf = NULL;
>> + struct lsmcontext context;
>> char *secctx = NULL;
>> u32 secctx_len;
>> struct lsmblob le;
>> @@ -457,7 +458,9 @@ int netlbl_unlhsh_add(struct net *net,
>> &secctx,
>> &secctx_len) == 0) {
>> audit_log_format(audit_buf, " sec_obj=%s", secctx);
>> - security_release_secctx(secctx, secctx_len);
>> + /* scaffolding */
>> + lsmcontext_init(&context, secctx, secctx_len, 0);
>> + security_release_secctx(&context);
>> }
>> audit_log_format(audit_buf, " res=%u", ret_val == 0 ? 1 : 0);
>> audit_log_end(audit_buf);
>> @@ -488,6 +491,7 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
>> struct netlbl_unlhsh_addr4 *entry;
>> struct audit_buffer *audit_buf;
>> struct net_device *dev;
>> + struct lsmcontext context;
>> char *secctx;
>> u32 secctx_len;
>> struct lsmblob le;
>> @@ -516,7 +520,9 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
>> security_secid_to_secctx(&le,
>> &secctx, &secctx_len) == 0) {
>> audit_log_format(audit_buf, " sec_obj=%s", secctx);
>> - security_release_secctx(secctx, secctx_len);
>> + /* scaffolding */
>> + lsmcontext_init(&context, secctx, secctx_len, 0);
>> + security_release_secctx(&context);
>> }
>> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
>> audit_log_end(audit_buf);
>> @@ -553,6 +559,7 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
>> struct netlbl_unlhsh_addr6 *entry;
>> struct audit_buffer *audit_buf;
>> struct net_device *dev;
>> + struct lsmcontext context;
>> char *secctx;
>> u32 secctx_len;
>> struct lsmblob le;
>> @@ -580,7 +587,8 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
>> security_secid_to_secctx(&le,
>> &secctx, &secctx_len) == 0) {
>> audit_log_format(audit_buf, " sec_obj=%s", secctx);
>> - security_release_secctx(secctx, secctx_len);
>> + lsmcontext_init(&context, secctx, secctx_len, 0);
>> + security_release_secctx(&context);
>> }
>> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
>> audit_log_end(audit_buf);
>> @@ -1094,6 +1102,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
>> int ret_val = -ENOMEM;
>> struct netlbl_unlhsh_walk_arg *cb_arg = arg;
>> struct net_device *dev;
>> + struct lsmcontext context;
>> void *data;
>> u32 secid;
>> char *secctx;
>> @@ -1161,7 +1170,9 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
>> NLBL_UNLABEL_A_SECCTX,
>> secctx_len,
>> secctx);
>> - security_release_secctx(secctx, secctx_len);
>> + /* scaffolding */
>> + lsmcontext_init(&context, secctx, secctx_len, 0);
>> + security_release_secctx(&context);
>> if (ret_val != 0)
>> goto list_cb_failure;
>>
>> diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
>> index 4145adf55a22..fba861c4ffbb 100644
>> --- a/net/netlabel/netlabel_user.c
>> +++ b/net/netlabel/netlabel_user.c
>> @@ -98,6 +98,7 @@ struct audit_buffer *netlbl_audit_start_common(int type,
>> struct netlbl_audit *audit_info)
>> {
>> struct audit_buffer *audit_buf;
>> + struct lsmcontext context;
>> char *secctx;
>> u32 secctx_len;
>> struct lsmblob le;
>> @@ -117,7 +118,8 @@ struct audit_buffer *netlbl_audit_start_common(int type,
>> if (audit_info->secid != 0 &&
>> security_secid_to_secctx(&le, &secctx, &secctx_len) == 0) {
>> audit_log_format(audit_buf, " subj=%s", secctx);
>> - security_release_secctx(secctx, secctx_len);
>> + lsmcontext_init(&context, secctx, secctx_len, 0);/*scaffolding*/
>> + security_release_secctx(&context);
>> }
>>
>> return audit_buf;
>> diff --git a/security/security.c b/security/security.c
>> index 9cfdc664239e..d25c099b46d1 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -458,6 +458,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>> &security_hook_heads.socket_getpeersec_dgram ||
>> hooks[i].head == &security_hook_heads.secctx_to_secid ||
>> hooks[i].head == &security_hook_heads.secid_to_secctx ||
>> + hooks[i].head == &security_hook_heads.release_secctx ||
>> hooks[i].head == &security_hook_heads.ipc_getsecid ||
>> hooks[i].head == &security_hook_heads.task_getsecid ||
>> hooks[i].head == &security_hook_heads.inode_getsecid ||
>> @@ -2083,16 +2084,19 @@ int security_secctx_to_secid(const char *secdata, u32 seclen, struct lsmblob *l)
>> }
>> EXPORT_SYMBOL(security_secctx_to_secid);
>>
>> -void security_release_secctx(char *secdata, u32 seclen)
>> +void security_release_secctx(struct lsmcontext *cp)
>> {
>> - int *display = current->security;
>> struct security_hook_list *hp;
>>
>> hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
>> - if (*display == LSMDATA_INVALID || *display == hp->slot) {
>> - hp->hook.release_secctx(secdata, seclen);
>> + if (cp->slot == hp->slot) {
>> + hp->hook.release_secctx(cp->context, cp->len);
>> + lsmcontext_init(cp, NULL, 0, 0);
>> return;
>> }
>> +
>> + pr_warn("%s context \"%s\" from slot %d not released\n", __func__,
>> + cp->context, cp->slot);
>> }
>> EXPORT_SYMBOL(security_release_secctx);
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index e9560b078efe..3834b751d1e9 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -4439,11 +4439,16 @@ static int smack_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
>> return 0;
>> }
>>
>> -/*
>> - * There used to be a smack_release_secctx hook
>> - * that did nothing back when hooks were in a vector.
>> - * Now that there's a list such a hook adds cost.
>> +/**
>> + * smack_release_secctx - do everything necessary to free a context
>> + * @secdata: Unused
>> + * @seclen: Unused
>> + *
>> + * Do nothing but hold a slot in the hooks list.
>> */
>> +static void smack_release_secctx(char *secdata, u32 seclen)
>> +{
>> +}
>>
>> static int smack_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
>> {
>> @@ -4683,6 +4688,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>> LSM_HOOK_INIT(ismaclabel, smack_ismaclabel),
>> LSM_HOOK_INIT(secid_to_secctx, smack_secid_to_secctx),
>> LSM_HOOK_INIT(secctx_to_secid, smack_secctx_to_secid),
>> + LSM_HOOK_INIT(release_secctx, smack_release_secctx),
>> LSM_HOOK_INIT(inode_notifysecctx, smack_inode_notifysecctx),
>> LSM_HOOK_INIT(inode_setsecctx, smack_inode_setsecctx),
>> LSM_HOOK_INIT(inode_getsecctx, smack_inode_getsecctx),
>> --
>> 2.20.1
>>
^ permalink raw reply
* Re: [PATCH v2 18/25] LSM: Use lsmcontext in security_dentry_init_security
From: Casey Schaufler @ 2019-06-19 17:31 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds, casey
In-Reply-To: <201906182238.4EBF8C17DB@keescook>
On 6/18/2019 10:41 PM, Kees Cook wrote:
> On Tue, Jun 18, 2019 at 04:05:44PM -0700, Casey Schaufler wrote:
>> Chance the security_dentry_init_security() interface to
Note to self: s/Chance/Change/
>> fill an lsmcontext structure instead of a void * data area
>> and a length. The lone caller of this interface is NFS4,
>> which may make copies of the data using its own mechanisms.
>> A rework of the nfs4 code to use the lsmcontext properly
>> is a significant project, so the coward's way out is taken,
>> and the lsmcontext data from security_dentry_init_security()
>> is copied, then released directly.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> fs/nfs/nfs4proc.c | 26 ++++++++++++++++----------
>> include/linux/security.h | 7 +++----
>> security/security.c | 20 ++++++++++++++++----
>> 3 files changed, 35 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index af1c0db29c39..952f805965bb 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -113,6 +113,7 @@ static inline struct nfs4_label *
>> nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
>> struct iattr *sattr, struct nfs4_label *label)
>> {
>> + struct lsmcontext context;
>> int err;
>>
>> if (label == NULL)
>> @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
>> return NULL;
>>
>> err = security_dentry_init_security(dentry, sattr->ia_mode,
>> - &dentry->d_name, (void **)&label->label, &label->len);
>> - if (err == 0)
>> - return label;
>> + &dentry->d_name, &context);
>> +
>> + if (err)
>> + return NULL;
>> +
>> + label->label = kmemdup(context.context, context.len, GFP_KERNEL);
> I think this is wrong: for NUL-terminated strings, "context.len" isn't
> currently including the NUL byte (it's set to strlen()).
>
> So, if kmemdup() is used here, it means strlen() isn't correct in the
> context init helper, it should be using the "size" argument, etc.
Would all be true if the context where being set by lsmcontext_init,
but it's not. It's coming from the dentry_init_security hook, and
the one instance of that, selinux_dentry_init_security() sets the
size to strlen() + 1. The kmemdup() will include the terminating NUL.
I too wish that the hooks and their use where more consistent.
My sincere hope is that this revision of the infrastructure will
help that to some extent.
>> + if (label->label == NULL)
>> + label = NULL;
>> + else
>> + label->len = context.len;
>> +
>> + security_release_secctx(&context);
>> +
>> + return label;
>>
>> - return NULL;
>> }
>> static inline void
>> nfs4_label_release_security(struct nfs4_label *label)
>> {
>> - struct lsmcontext scaff; /* scaffolding */
>> -
>> - if (label) {
>> - lsmcontext_init(&scaff, label->label, label->len, 0);
>> - security_release_secctx(&scaff);
>> - }
>> + kfree(label->label);
> Should label be set to NULL here and len reduced to 0?
It wasn't before, and I'd hate to make too many assumptions
about what might be fragile in the NFS code.
>> }
>> static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
>> {
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 1fd87e80656f..92c4960dd57f 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -346,8 +346,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb,
>> int security_add_mnt_opt(const char *option, const char *val,
>> int len, void **mnt_opts);
>> int security_dentry_init_security(struct dentry *dentry, int mode,
>> - const struct qstr *name, void **ctx,
>> - u32 *ctxlen);
>> + const struct qstr *name,
>> + struct lsmcontext *ctx);
>> int security_dentry_create_files_as(struct dentry *dentry, int mode,
>> struct qstr *name,
>> const struct cred *old,
>> @@ -718,8 +718,7 @@ static inline void security_inode_free(struct inode *inode)
>> static inline int security_dentry_init_security(struct dentry *dentry,
>> int mode,
>> const struct qstr *name,
>> - void **ctx,
>> - u32 *ctxlen)
>> + struct lsmcontext *ctx)
>> {
>> return -EOPNOTSUPP;
>> }
>> diff --git a/security/security.c b/security/security.c
>> index 2ea810fc4a45..23d8049ec0c1 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -453,6 +453,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>> * secid in the lsmblob structure.
>> */
>> if (hooks[i].head == &security_hook_heads.audit_rule_match ||
>> + hooks[i].head ==
>> + &security_hook_heads.dentry_init_security ||
>> hooks[i].head == &security_hook_heads.kernel_act_as ||
>> hooks[i].head ==
>> &security_hook_heads.socket_getpeersec_dgram ||
>> @@ -1030,11 +1032,21 @@ void security_inode_free(struct inode *inode)
>> }
>>
>> int security_dentry_init_security(struct dentry *dentry, int mode,
>> - const struct qstr *name, void **ctx,
>> - u32 *ctxlen)
>> + const struct qstr *name,
>> + struct lsmcontext *cp)
>> {
>> - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
>> - name, ctx, ctxlen);
>> + int *display = current->security;
>> + struct security_hook_list *hp;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security,
>> + list)
>> + if (*display == 0 || *display == hp->slot) {
>> + cp->slot = hp->slot;
>> + return hp->hook.dentry_init_security(dentry, mode,
>> + name, (void **)&cp->context, &cp->len);
>> + }
>> +
>> + return -EOPNOTSUPP;
>> }
>> EXPORT_SYMBOL(security_dentry_init_security);
>>
>> --
>> 2.20.1
>>
^ permalink raw reply
* Re: [PATCH v2 24/25] Fix slotted list and getpeersec_d
From: Casey Schaufler @ 2019-06-19 17:36 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds, casey
In-Reply-To: <201906182250.02BF2E47@keescook>
On 6/18/2019 10:50 PM, Kees Cook wrote:
> On Tue, Jun 18, 2019 at 04:05:50PM -0700, Casey Schaufler wrote:
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Was this supposed to be folded into patch 4?
The first hunk, yes. I'll do that in the next revision.
The second hunk I'm still debating whether this is the
right change, or whether the AppArmor socket_getpeersec_dgram
stub hook should just be deleted.
> -Kees
>
>> ---
>> security/security.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/security.c b/security/security.c
>> index 5a23ccec7c7b..8aca43ab3e81 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -461,6 +461,8 @@ int __init security_add_hooks(struct security_hook_list *hooks, int count,
>> hooks[i].head == &security_hook_heads.kernel_act_as ||
>> hooks[i].head ==
>> &security_hook_heads.socket_getpeersec_dgram ||
>> + hooks[i].head == &security_hook_heads.getprocattr ||
>> + hooks[i].head == &security_hook_heads.setprocattr ||
>> hooks[i].head == &security_hook_heads.secctx_to_secid ||
>> hooks[i].head == &security_hook_heads.release_secctx ||
>> hooks[i].head == &security_hook_heads.ipc_getsecid ||
>> @@ -2269,7 +2271,9 @@ int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
>> list) {
>> rc = hp->hook.socket_getpeersec_dgram(sock, skb,
>> &l->secid[hp->slot]);
>> - if (rc != 0)
>> + if (rc == -ENOPROTOOPT)
>> + rc = 0;
>> + else if (rc != 0)
>> break;
>> }
>> return rc;
>> --
>> 2.20.1
>>
^ permalink raw reply
* Re: [PATCH V9 1/3] IMA:Define a new hook to measure the kexec boot command line arguments
From: prakhar srivastava @ 2019-06-19 17:46 UTC (permalink / raw)
To: linux-integrity, linux-security-module, linux-kernel
Cc: Mimi Zohar, Roberto Sassu
In-Reply-To: <20190617183738.14484-2-prsriva02@gmail.com>
On Mon, Jun 17, 2019 at 11:37 AM Prakhar Srivastava <prsriva02@gmail.com> wrote:
>
> Currently during soft reboot(kexec_file_load) boot command line
> arguments are not measured. Define hooks needed to measure kexec
> command line arguments during soft reboot(kexec_file_load).
>
> - A new ima hook ima_kexec_cmdline is defined to be called by the
> kexec code.
> - A new function process_buffer_measurement is defined to measure
> the buffer hash into the IMA measurement list.
> - A new func policy KEXEC_CMDLINE is defined to control the
> measurement.[Suggested by Mimi]
>
> Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
> ---
> Documentation/ABI/testing/ima_policy | 1 +
> include/linux/ima.h | 2 +
> security/integrity/ima/ima.h | 1 +
> security/integrity/ima/ima_api.c | 1 +
> security/integrity/ima/ima_main.c | 74 ++++++++++++++++++++++++++++
> security/integrity/ima/ima_policy.c | 7 +++
> 6 files changed, 86 insertions(+)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index b383c1763610..fc376a323908 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -28,6 +28,7 @@ Description:
> base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> + [KEXEC_CMDLINE]
> mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
> [[^]MAY_EXEC]
> fsmagic:= hex value
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index fd9f7cf4cdf5..b42f5a006042 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -26,6 +26,7 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
> extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> enum kernel_read_file_id id);
> extern void ima_post_path_mknod(struct dentry *dentry);
> +extern void ima_kexec_cmdline(const void *buf, int size);
>
> #ifdef CONFIG_IMA_KEXEC
> extern void ima_add_kexec_buffer(struct kimage *image);
> @@ -92,6 +93,7 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
> return;
> }
>
> +static inline void ima_kexec_cmdline(const void *buf, int size) {}
> #endif /* CONFIG_IMA */
>
> #ifndef CONFIG_IMA_KEXEC
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 18b48a6d0b80..a4ad1270bffa 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -185,6 +185,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
> hook(KEXEC_KERNEL_CHECK) \
> hook(KEXEC_INITRAMFS_CHECK) \
> hook(POLICY_CHECK) \
> + hook(KEXEC_CMDLINE) \
> hook(MAX_CHECK)
> #define __ima_hook_enumify(ENUM) ENUM,
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 78eb11c7ac07..ea7d8cbf712f 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -176,6 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
> * subj=, obj=, type=, func=, mask=, fsmagic=
> * subj,obj, and type: are LSM specific.
> * func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
> + * | KEXEC_CMDLINE
> * mask: contains the permission mask
> * fsmagic: hex value
> *
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index af341a80118f..1e233417a7af 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -605,6 +605,80 @@ int ima_load_data(enum kernel_load_data_id id)
> return 0;
> }
>
> +/*
> + * process_buffer_measurement - Measure the buffer to ima log.
> + * @buf: pointer to the buffer that needs to be added to the log.
> + * @size: size of buffer(in bytes).
> + * @eventname: event name to be used for the buffer entry.
> + * @cred: a pointer to a credentials structure for user validation.
> + * @secid: the secid of the task to be validated.
> + *
> + * Based on policy, the buffer is measured into the ima log.
> + */
> +static void process_buffer_measurement(const void *buf, int size,
> + const char *eventname,
> + const struct cred *cred, u32 secid)
> +{
> + int ret = 0;
> + struct ima_template_entry *entry = NULL;
> + struct integrity_iint_cache iint = {};
> + struct ima_event_data event_data = {.iint = &iint };
> + struct ima_template_desc *template_desc = NULL;
> + struct {
> + struct ima_digest_data hdr;
> + char digest[IMA_MAX_DIGEST_SIZE];
> + } hash = {};
> + int violation = 0;
> + int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> + int action = 0;
> +
> + action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
> + &template_desc);
> + if (!(action & IMA_MEASURE))
> + goto out;
> +
> + event_data.filename = eventname;
> +
> + iint.ima_hash = &hash.hdr;
> + iint.ima_hash->algo = ima_hash_algo;
> + iint.ima_hash->length = hash_digest_size[ima_hash_algo];
> +
> + ret = ima_calc_buffer_hash(buf, size, iint.ima_hash);
> + if (ret < 0)
> + goto out;
> +
> + ret = ima_alloc_init_template(&event_data, &entry, template_desc);
> + if (ret < 0)
> + goto out;
> +
> + if (action & IMA_MEASURE)
> + ret = ima_store_template(entry, violation, NULL, buf, pcr);
> +
> + if (ret < 0)
> + ima_free_template_entry(entry);
> +
> +out:
> + return;
> +}
> +
> +/**
> + * ima_kexec_cmdline - measure kexec cmdline boot args
> + * @buf: pointer to buffer
> + * @size: size of buffer
> + *
> + * Buffers can only be measured, not appraised.
> + */
> +void ima_kexec_cmdline(const void *buf, int size)
> +{
> + u32 secid;
> +
> + if (buf && size != 0) {
> + security_task_getsecid(current, &secid);
> + process_buffer_measurement(buf, size, "kexec-cmdline",
> + current_cred(), secid);
> + }
> +}
> +
> static int __init init_ima(void)
> {
> int error;
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index fd9b01881d17..4e8bb7eecd08 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -292,6 +292,11 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> {
> int i;
>
> + if (func == KEXEC_CMDLINE) {
> + if ((rule->flags & IMA_FUNC) && (rule->func == func))
> + return true;
> + return false;
> + }
> if ((rule->flags & IMA_FUNC) &&
> (rule->func != func && func != POST_SETATTR))
> return false;
> @@ -880,6 +885,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> entry->func = KEXEC_INITRAMFS_CHECK;
> else if (strcmp(args[0].from, "POLICY_CHECK") == 0)
> entry->func = POLICY_CHECK;
> + else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0)
> + entry->func = KEXEC_CMDLINE;
> else
> result = -EINVAL;
> if (!result)
> --
> 2.19.1
>
Hi Mimi,
Can you Ack this patch?
I want to make sure this looks okay to you.
Thanks,
Prakhar Srivastava
^ permalink raw reply
* Re: [PATCH 2/3] IMA:Define a new template field buf
From: prakhar srivastava @ 2019-06-19 18:08 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <1560952466.3975.40.camel@linux.ibm.com>
<snip>
> > if (iint->measured_pcrs & (0x1 << pcr))
> > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> > index 993d0f1915ff..c8591406c0e2 100644
> > --- a/security/integrity/ima/ima_init.c
> > +++ b/security/integrity/ima/ima_init.c
> > @@ -50,7 +50,7 @@ static int __init ima_add_boot_aggregate(void)
> > struct ima_template_entry *entry;
> > struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
> > struct ima_event_data event_data = {iint, NULL, boot_aggregate_name,
> > - NULL, 0, NULL};
> > + NULL, 0, NULL, NULL, 0};
> > int result = -ENOMEM;
> > int violation = 0;
> > struct {
> >
>
> These changes shouldn't be necessary. Please rebase these patches on
> top of the latest next-queued-testing branch (git remote update). "IMA: support for per
> policy rule template formats" is still changing.
>
> Minor nit. When re-posting the patches please update the patch titles
> so that there is a space between the subsystem name and the patch
> title (eg. "ima: define ...").
>
I believe the above event_data changes are needed, to store/read the
buffer length and buffer itself. The only exception will be if needed will be to
remove ima-buf as a template instead used a template_fmt in the policy
with KEXEC_CMDLINE from the "IMA: support for per
policy rule template formats" is still changing.".
In my view even ima-buf is needed as it simplifies the usage.
Please let me know if I misunderstood your comment.
> Mimi
>
^ permalink raw reply
* Re: [PATCH 2/3] IMA:Define a new template field buf
From: Mimi Zohar @ 2019-06-19 18:37 UTC (permalink / raw)
To: prakhar srivastava, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <CAEFn8qK9Tg99PA_=Ukm=CwSE6ajjUL2FxLs0ZiVdGLvG_baK_A@mail.gmail.com>
On Wed, 2019-06-19 at 11:08 -0700, prakhar srivastava wrote:
> <snip>
> > > if (iint->measured_pcrs & (0x1 << pcr))
> > > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> > > index 993d0f1915ff..c8591406c0e2 100644
> > > --- a/security/integrity/ima/ima_init.c
> > > +++ b/security/integrity/ima/ima_init.c
> > > @@ -50,7 +50,7 @@ static int __init ima_add_boot_aggregate(void)
> > > struct ima_template_entry *entry;
> > > struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
> > > struct ima_event_data event_data = {iint, NULL, boot_aggregate_name,
> > > - NULL, 0, NULL};
> > > + NULL, 0, NULL, NULL, 0};
> > > int result = -ENOMEM;
> > > int violation = 0;
> > > struct {
> > >
> >
> > These changes shouldn't be necessary. Please rebase these patches on
> > top of the latest next-queued-testing branch (git remote update). "IMA: support for per
> > policy rule template formats" is still changing.
> >
> > Minor nit. When re-posting the patches please update the patch titles
> > so that there is a space between the subsystem name and the patch
> > title (eg. "ima: define ...").
> >
> I believe the above event_data changes are needed, to store/read the
> buffer length and buffer itself. The only exception will be if needed will be to
> remove ima-buf as a template instead used a template_fmt in the policy
> with KEXEC_CMDLINE from the "IMA: support for per
> policy rule template formats" is still changing.".
> In my view even ima-buf is needed as it simplifies the usage.
>
> Please let me know if I misunderstood your comment.
The tip of next-queued-testing branch is commit 687d57f90461 ("IMA:
support for per policy rule template formats"). The current code is:
struct ima_event_data event_data = { .iint = iint,
.filename = boot_aggregate_name };
Mimi
^ permalink raw reply
* Re: [GIT PULL] apparmor bug fixes for v5.3-rc6
From: pr-tracker-bot @ 2019-06-19 19:00 UTC (permalink / raw)
To: John Johansen; +Cc: Linus Torvalds, LKLM, open list:SECURITY SUBSYSTEM
In-Reply-To: <cfc8f629-4ffa-e64e-f23f-2f4cffca4f18@canonical.com>
The pull request you sent on Tue, 18 Jun 2019 18:44:31 -0700:
> git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2019-06-18
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/c3c0d546d73ad53c85789154872b8c92d1f96ba1
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-06-19 19:10 UTC (permalink / raw)
To: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel
Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka,
gmazyland
This patch set adds in-kernel pkcs7 signature checking for the roothash of
the dm-verity hash tree.
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.
Why we are doing validation in the Kernel?
The reason is to still be secure in cases where the attacker is able to
compromise the user mode application in which case the user mode validation
could not have been trusted.
The root hash signature validation in the kernel along with existing
dm-verity implementation gives a higher level of confidence in the
executable code or the protected data. Before allowing the creation of
the device mapper block device the kernel code will check that the detached
pkcs7 signature passed to it validates the roothash and the signature is
trusted by builtin keys set at kernel creation. The kernel should be
secured using Verified boot, UEFI Secure Boot or similar technologies so we
can trust it.
What about attacker mounting non dm-verity volumes to run executable
code?
This verification can be used to have a security architecture where a LSM
can enforce this verification for all the volumes and by doing this it can
ensure that all executable code runs from signed and trusted dm-verity
volumes.
Further patches will be posted that build on this and enforce this
verification based on policy for all the volumes on the system.
How are these changes tested?
To generate and sign the roothash, dump the roothash returned by
veritysetup format in a text file, say roothash.txt and then sign using
the openssl command:
openssl smime -sign -nocerts -noattr -binary -in <roothash.txt>
-inkey <keyfile> -signer <certfile> -outform der -out <out_sigfile>
To pass the roothash signature to dm-verity, veritysetup part of cryptsetup
library was modified to take a optional root-hash-sig parameter.
Commandline used to test the changes:
Use the signature file from above step as a parameter to veritysetup.
veritysetup open <data_device> <name> <hash_device> <root_hash>
--root-hash-sig=<root_hash_pkcs7_detached_sig>
The changes for veritysetup are in a topic branch for now at:
https://github.com/jaskarankhurana/veritysetup/tree/veritysetup_add_sig
Set kernel commandline dm_verity.verify_sig=1 or 2 for check/force
dm-verity to do root hash signature validation.
Changelog:
v5 (since previous):
- Code review feedback given by Milan Broz.
- Remove the Kconfig for root hash verification and instead add a
commandline parameter(dm_verity.verify_sig) that determines whether to
check or enforce root hash signature validation.
- Fixed a small issue when dm-verity was built sepaerately as a module.
- Added the openssl commandline that can be used to sign the roothash
in the cover letter.
v4:
- Code review feedback given by Milan Broz.
- Add documentation about the root hash signature parameter.
- Bump up the dm-verity target version.
- Provided way to sign and test with veritysetup in cover letter.
v3:
- Code review feedback given by Sasha Levin.
- Removed EXPORT_SYMBOL_GPL since this was not required.
- Removed "This file is released under the GPLv2" since we have SPDX
identifier.
- Inside verity_verify_root_hash changed EINVAL to ENOKEY when the key
descriptor is not specified but due to force option being set it is
expected.
- Moved CONFIG check to inside verity_verify_get_sig_from_key.
(Did not move the sig_opts_cleanup to inside verity_dtr as the
sig_opts do not need to be allocated for the entire duration the block
device is active unlike the verity structure, note verity_dtr is
called only if verity_ctr fails or after the lifetime of the
block device.)
v2:
- Code review feedback to pass the signature binary blob as a key that
can be looked up in the kernel and be used to verify the roothash.
[Suggested by Milan Broz]
- Made the code related change suggested in review of v1.
[Suggested by Balbir Singh]
v1:
- Add kconfigs to control dm-verity root has signature verification and
use the signature if specified to verify the root hash.
Jaskaran Khurana (1):
Adds in-kernel pkcs7 sig check dmverity roothash
Documentation/device-mapper/verity.txt | 7 ++
drivers/md/Kconfig | 1 +
drivers/md/Makefile | 2 +-
drivers/md/dm-verity-target.c | 36 ++++++-
drivers/md/dm-verity-verify-sig.c | 139 +++++++++++++++++++++++++
drivers/md/dm-verity-verify-sig.h | 37 +++++++
6 files changed, 216 insertions(+), 6 deletions(-)
create mode 100644 drivers/md/dm-verity-verify-sig.c
create mode 100644 drivers/md/dm-verity-verify-sig.h
--
2.17.1
^ permalink raw reply
* [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-06-19 19:10 UTC (permalink / raw)
To: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel
Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka,
gmazyland
In-Reply-To: <20190619191048.20365-1-jaskarankhurana@linux.microsoft.com>
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.
The signature being provided for verification must verify the root hash and
must be trusted by the builtin keyring for verification to succeed.
The hash is added as a key of type "user" and the description is passed to
the kernel so it can look it up and use it for verification.
Kernel commandline parameter will indicate whether to check (only if
specified) or force (for all dm verity volumes) roothash signature
verification.
Kernel commandline: dm_verity.verify_sig=1 or 2 for check/force root hash
signature validation respectively.
Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
---
Documentation/device-mapper/verity.txt | 7 ++
drivers/md/Kconfig | 1 +
drivers/md/Makefile | 2 +-
drivers/md/dm-verity-target.c | 36 ++++++-
drivers/md/dm-verity-verify-sig.c | 139 +++++++++++++++++++++++++
drivers/md/dm-verity-verify-sig.h | 37 +++++++
6 files changed, 216 insertions(+), 6 deletions(-)
create mode 100644 drivers/md/dm-verity-verify-sig.c
create mode 100644 drivers/md/dm-verity-verify-sig.h
diff --git a/Documentation/device-mapper/verity.txt b/Documentation/device-mapper/verity.txt
index b3d2e4a42255..df7ef1d553cc 100644
--- a/Documentation/device-mapper/verity.txt
+++ b/Documentation/device-mapper/verity.txt
@@ -121,6 +121,13 @@ check_at_most_once
blocks, and a hash block will not be verified any more after all the data
blocks it covers have been verified anyway.
+root_hash_sig_key_desc <key_description>
+ This is the description of the USER_KEY that the kernel will lookup to get
+ the pkcs7 signature of the roothash. The pkcs7 signature is used to validate
+ the root hash during the creation of the device mapper block device.
+ Verification of roothash depends on the config DM_VERITY_VERIFY_ROOTHASH_SIG
+ being set in the kernel.
+
Theory of operation
===================
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index db269a348b20..2d658a3512cb 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -475,6 +475,7 @@ config DM_VERITY
select CRYPTO
select CRYPTO_HASH
select DM_BUFIO
+ select SYSTEM_DATA_VERIFICATION
---help---
This device-mapper target creates a read-only device that
transparently validates the data on one underlying device against
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index be7a6eb92abc..3b47b256b15e 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -18,7 +18,7 @@ dm-cache-y += dm-cache-target.o dm-cache-metadata.o dm-cache-policy.o \
dm-cache-background-tracker.o
dm-cache-smq-y += dm-cache-policy-smq.o
dm-era-y += dm-era-target.o
-dm-verity-y += dm-verity-target.o
+dm-verity-y += dm-verity-target.o dm-verity-verify-sig.o
md-mod-y += md.o md-bitmap.o
raid456-y += raid5.o raid5-cache.o raid5-ppl.o
dm-zoned-y += dm-zoned-target.o dm-zoned-metadata.o dm-zoned-reclaim.o
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index f4c31ffaa88e..adf7f376be7d 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -16,7 +16,7 @@
#include "dm-verity.h"
#include "dm-verity-fec.h"
-
+#include "dm-verity-verify-sig.h"
#include <linux/module.h>
#include <linux/reboot.h>
@@ -34,7 +34,8 @@
#define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks"
#define DM_VERITY_OPT_AT_MOST_ONCE "check_at_most_once"
-#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC)
+#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC + \
+ DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
@@ -855,7 +856,8 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
return r;
}
-static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
+static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *verify_args)
{
int r;
unsigned argc;
@@ -904,6 +906,14 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
if (r)
return r;
continue;
+ } else if (verity_verify_is_sig_opt_arg(arg_name)) {
+ r = verity_verify_sig_parse_opt_args(as, v,
+ verify_args,
+ &argc, arg_name);
+ if (r)
+ return r;
+ continue;
+
}
ti->error = "Unrecognized verity feature request";
@@ -930,6 +940,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
{
struct dm_verity *v;
+ struct dm_verity_sig_opts verify_args = {0};
struct dm_arg_set as;
unsigned int num;
unsigned long long num_ll;
@@ -937,6 +948,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
int i;
sector_t hash_position;
char dummy;
+ char *root_hash_digest_to_validate;
v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
if (!v) {
@@ -1070,6 +1082,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
r = -EINVAL;
goto bad;
}
+ root_hash_digest_to_validate = argv[8];
if (strcmp(argv[9], "-")) {
v->salt_size = strlen(argv[9]) / 2;
@@ -1095,11 +1108,20 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
as.argc = argc;
as.argv = argv;
- r = verity_parse_opt_args(&as, v);
+ r = verity_parse_opt_args(&as, v, &verify_args);
if (r < 0)
goto bad;
}
+ /* Root hash signature is a optional parameter*/
+ r = verity_verify_root_hash(root_hash_digest_to_validate,
+ strlen(root_hash_digest_to_validate),
+ verify_args.sig,
+ verify_args.sig_size);
+ if (r < 0) {
+ ti->error = "Root hash verification failed";
+ goto bad;
+ }
v->hash_per_block_bits =
__fls((1 << v->hash_dev_block_bits) / v->digest_size);
@@ -1165,9 +1187,13 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
ti->per_io_data_size = roundup(ti->per_io_data_size,
__alignof__(struct dm_verity_io));
+ verity_verify_sig_opts_cleanup(&verify_args);
+
return 0;
bad:
+
+ verity_verify_sig_opts_cleanup(&verify_args);
verity_dtr(ti);
return r;
@@ -1175,7 +1201,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
static struct target_type verity_target = {
.name = "verity",
- .version = {1, 4, 0},
+ .version = {1, 5, 0},
.module = THIS_MODULE,
.ctr = verity_ctr,
.dtr = verity_dtr,
diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
new file mode 100644
index 000000000000..189b321d4ee6
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#include <linux/device-mapper.h>
+#include <linux/verification.h>
+#include <keys/user-type.h>
+#include <linux/module.h>
+#include "dm-verity.h"
+#include "dm-verity-verify-sig.h"
+
+#define DM_VERITY_VERIFY_ERR(s) DM_VERITY_ROOT_HASH_VERIFICATION " " s
+
+static int verify_sig;
+module_param(verify_sig, int, 0);
+MODULE_PARM_DESC(verify_sig,
+ "Verify the roothash of dm-verity hash tree");
+
+#define DM_VERITY_IS_SIG_CHECK_ENABLED() \
+ (verify_sig == DM_VERITY_VERIFY_SIG_CHECK || \
+ verify_sig == DM_VERITY_VERIFY_SIG_FORCE)
+
+#define DM_VERITY_IS_SIG_FORCE_ENABLED() \
+ (verify_sig == DM_VERITY_VERIFY_SIG_FORCE)
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name)
+{
+ return (!strcasecmp(arg_name,
+ DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY));
+}
+
+static int verity_verify_get_sig_from_key(const char *key_desc,
+ struct dm_verity_sig_opts *sig_opts)
+{
+ struct key *key;
+ const struct user_key_payload *ukp;
+ int ret = 0;
+
+ if (!DM_VERITY_IS_SIG_CHECK_ENABLED())
+ return 0;
+
+ key = request_key(&key_type_user,
+ key_desc, NULL);
+ if (IS_ERR(key))
+ return PTR_ERR(key);
+
+ down_read(&key->sem);
+
+ ukp = user_key_payload_locked(key);
+ if (!ukp) {
+ ret = -EKEYREVOKED;
+ goto end;
+ }
+
+ sig_opts->sig = kmalloc(ukp->datalen, GFP_KERNEL);
+ if (!sig_opts->sig) {
+ ret = -ENOMEM;
+ goto end;
+ }
+ sig_opts->sig_size = ukp->datalen;
+
+ memcpy(sig_opts->sig, ukp->data, sig_opts->sig_size);
+
+end:
+ up_read(&key->sem);
+ key_put(key);
+
+ return ret;
+}
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as,
+ struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc,
+ const char *arg_name)
+{
+ struct dm_target *ti = v->ti;
+ int ret = 0;
+ const char *sig_key = NULL;
+
+ if (!*argc) {
+ ti->error = DM_VERITY_VERIFY_ERR("Signature key not specified");
+ return -EINVAL;
+ }
+
+ sig_key = dm_shift_arg(as);
+ (*argc)--;
+
+ ret = verity_verify_get_sig_from_key(sig_key, sig_opts);
+ if (ret < 0)
+ ti->error = DM_VERITY_VERIFY_ERR("Invalid key specified");
+
+ return ret;
+}
+
+/*
+ * verify_verify_roothash - Verify the root hash of the verity hash device
+ * using builtin trusted keys.
+ *
+ * @root_hash: For verity, the roothash/data to be verified.
+ * @root_hash_len: Size of the roothash/data to be verified.
+ * @sig_data: The trusted signature that verifies the roothash/data.
+ * @sig_len: Size of the signature.
+ *
+ */
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+ const void *sig_data, size_t sig_len)
+{
+ int ret;
+
+ if (!DM_VERITY_IS_SIG_CHECK_ENABLED())
+ return 0;
+
+ if (!root_hash || root_hash_len == 0)
+ return -EINVAL;
+
+ if (!sig_data || sig_len == 0) {
+ if (DM_VERITY_IS_SIG_FORCE_ENABLED())
+ return -ENOKEY;
+ else
+ return 0;
+ }
+
+ ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
+ sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
+ NULL, NULL);
+
+ return ret;
+}
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
+{
+ kfree(sig_opts->sig);
+ sig_opts->sig = NULL;
+ sig_opts->sig_size = 0;
+}
diff --git a/drivers/md/dm-verity-verify-sig.h b/drivers/md/dm-verity-verify-sig.h
new file mode 100644
index 000000000000..3ac750996efb
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.h
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#ifndef DM_VERITY_SIG_VERIFICATION_H
+#define DM_VERITY_SIG_VERIFICATION_H
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION "DM Verity Sig Verification"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY "root_hash_sig_key_desc"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 2
+
+enum dm_verity_verif_opt {
+ DM_VERITY_VERIFY_SIG_NONE = 0,
+ DM_VERITY_VERIFY_SIG_CHECK,
+ DM_VERITY_VERIFY_SIG_FORCE,
+ DM_VERITY_VERIFY_SIG_MAX,
+};
+
+struct dm_verity_sig_opts {
+ unsigned int sig_size;
+ u8 *sig;
+};
+int verity_verify_root_hash(const void *data, size_t data_len,
+ const void *sig_data, size_t sig_len);
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name);
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc, const char *arg_name);
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts);
+
+#endif /* DM_VERITY_SIG_VERIFICATION_H */
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 1/3] IMA:Define a new hook to measure the kexec boot command line arguments
From: Mimi Zohar @ 2019-06-19 19:21 UTC (permalink / raw)
To: Prakhar Srivastava, linux-integrity, linux-security-module,
linux-kernel
Cc: roberto.sassu
In-Reply-To: <20190617183507.14160-2-prsriva02@gmail.com>
On Mon, 2019-06-17 at 11:35 -0700, Prakhar Srivastava wrote:
> Currently during soft reboot(kexec_file_load) boot command line
> arguments are not measured. Define hooks needed to measure kexec
> command line arguments during soft reboot(kexec_file_load).
>
> - A new ima hook ima_kexec_cmdline is defined to be called by the
> kexec code.
> - A new function process_buffer_measurement is defined to measure
> the buffer hash into the IMA measurement list.
> - A new func policy KEXEC_CMDLINE is defined to control the
> measurement.[Suggested by Mimi]
>
> Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
With minor changes below,
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index af341a80118f..1e233417a7af 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -605,6 +605,80 @@ int ima_load_data(enum kernel_load_data_id id)
> return 0;
> }
>
> +/*
> + * process_buffer_measurement - Measure the buffer to ima log.
> + * @buf: pointer to the buffer that needs to be added to the log.
> + * @size: size of buffer(in bytes).
> + * @eventname: event name to be used for the buffer entry.
> + * @cred: a pointer to a credentials structure for user validation.
> + * @secid: the secid of the task to be validated.
> + *
> + * Based on policy, the buffer is measured into the ima log.
> + */
> +static void process_buffer_measurement(const void *buf, int size,
> + const char *eventname,
> + const struct cred *cred, u32 secid)
> +{
> + int ret = 0;
> + struct ima_template_entry *entry = NULL;
> + struct integrity_iint_cache iint = {};
> + struct ima_event_data event_data = {.iint = &iint };
> + struct ima_template_desc *template_desc = NULL;
> + struct {
> + struct ima_digest_data hdr;
> + char digest[IMA_MAX_DIGEST_SIZE];
> + } hash = {};
> + int violation = 0;
> + int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> + int action = 0;
> +
> + action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
> + &template_desc);
> + if (!(action & IMA_MEASURE))
> + goto out;
"out:" is a simple return, no freeing memory. Just return here.
> +
> + event_data.filename = eventname;
No need to initialize even_data.filename, here initialize it when it
is defined.
> +
> + iint.ima_hash = &hash.hdr;
> + iint.ima_hash->algo = ima_hash_algo;
> + iint.ima_hash->length = hash_digest_size[ima_hash_algo];
> +
> + ret = ima_calc_buffer_hash(buf, size, iint.ima_hash);
> + if (ret < 0)
> + goto out;
> +
> + ret = ima_alloc_init_template(&event_data, &entry, template_desc);
> + if (ret < 0)
> + goto out;
> +
> + if (action & IMA_MEASURE)
Why is this test needed again?
Mimi
> + ret = ima_store_template(entry, violation, NULL, buf, pcr);
> +
> + if (ret < 0)
> + ima_free_template_entry(entry);
> +
> +out:
> + return;
> +}
> +
> +/**
> + * ima_kexec_cmdline - measure kexec cmdline boot args
> + * @buf: pointer to buffer
> + * @size: size of buffer
> + *
> + * Buffers can only be measured, not appraised.
> + */
> +void ima_kexec_cmdline(const void *buf, int size)
> +{
> + u32 secid;
> +
> + if (buf && size != 0) {
> + security_task_getsecid(current, &secid);
> + process_buffer_measurement(buf, size, "kexec-cmdline",
> + current_cred(), secid);
> + }
> +}
> +
> static int __init init_ima(void)
> {
> int error;
>
^ permalink raw reply
* Re: [PATCH v2 00/25] LSM: Module stacking for AppArmor
From: James Morris @ 2019-06-19 20:08 UTC (permalink / raw)
To: Kees Cook
Cc: Casey Schaufler, casey.schaufler, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <201906182133.EBF2C78D@keescook>
On Tue, 18 Jun 2019, Kees Cook wrote:
> On Tue, Jun 18, 2019 at 04:05:26PM -0700, Casey Schaufler wrote:
> > Patches 0001-0003 complete the process of moving managment
> > of security blobs that might be shared from the individual
> > modules to the infrastructure.
>
> I think these are happy stand-alone patches and should just go into the
> common LSM tree for v5.3.
We extended stacking support in March to allow Landlock and SARA to be
merged and have not seen anything from them since.
Is there any point in adding more of the same for v5.3 before the current
AppArmor stacking changes are fully ready? This seems to carry risk but no
concrete benefit for the release.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* [RFC PATCH v4 03/12] selftests: x86/sgx: Mark the enclave loader as not needing an exec stack
From: Sean Christopherson @ 2019-06-19 22:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190619222401.14942-1-sean.j.christopherson@intel.com>
The SGX enclave loader doesn't need an executable stack, but linkers
will assume it does due to the lack of .note.GNU-stack sections in the
loader's assembly code. As a result, the kernel tags the loader as
having "read implies exec", and so adds PROT_EXEC to all mmap()s, even
those for mapping EPC regions. This will cause problems in the future
when userspace needs to explicit state a page's protection bits when the
page is added to an enclave, e.g. adding TCS pages as R+W will cause
mmap() to fail when the kernel tacks on +X.
Explicitly tell the linker that an executable stack is not needed.
Alternatively, each .S file could add .note.GNU-stack, but the loader
should never need an executable stack so zap it in one fell swoop.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
tools/testing/selftests/x86/sgx/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/x86/sgx/Makefile b/tools/testing/selftests/x86/sgx/Makefile
index 1fd6f2708e81..10136b73096b 100644
--- a/tools/testing/selftests/x86/sgx/Makefile
+++ b/tools/testing/selftests/x86/sgx/Makefile
@@ -2,7 +2,7 @@ top_srcdir = ../../../../..
include ../../lib.mk
-HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC
+HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC -z noexecstack
ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIC \
-fno-stack-protector -mrdrnd $(INCLUDES)
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Sean Christopherson @ 2019-06-19 22:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
For those of you whom I neglected to cc on v3, here's a quick recap:
My original plan was for my next RFC to be an implementation of Andy's
proposed "dynamic tracking" model, but I was completely flummoxed by the
auditing[1]. Cedric's RFC has the same auditing complexities, so I
I ended up back at the "make userspace state its intentions" approach.
There are no significant LSM changes in v4, e.g. a bug fix and some
renaming. I'm spinning v4 early to get the cc list correct, and also
because I'm about to disappear on vacation for two weeks.
Except for patch 12 (see below), the SGX changes have been fully tested,
including updating the kernel's selftest as well as my own fork of (an old
version of) Intel's SDK to use the new UAPI. The LSM changes have been
smoke tested, but I haven't actually configured AppArmor or SELinux to
verify the permissions work as intended.
Patches 1-3 are not directly related to LSM support. They're included
here as the actual LSM RFC patches are essentially untestable without
them, and so that the patches apply to Jarkko's tree. Ignore patches
1-3 unless you actually want to run code.
Patches 4-11 are the meat of the RFC.
Patch 12 is purely to show how we might implement SGX2 support. It's not
intended to be included in the initial upstreaming of SGX.
The full code is available at https://github.com/sean-jc/linux.git in
a few forms (tagged);
sgx-lsm-v4 - Jarkko's full tree plus patches 1-11
sgx-lsm-v4-eaug - Everything above plus patch 12
<boilerplate>
This series is a delta to Jarkko's ongoing SGX series and applies on
Jarkko's current master at https://github.com/jsakkine-intel/linux-sgx.git:
91f3aa6d241d ("docs: x86/sgx: Document the enclave API")
The basic gist of the approach is to track an enclave's page protections
separately from any vmas that map the page, and separate from the hardware
enforced protections. The SGX UAPI is modified to require userspace to
explicitly define the protections for each enclave page, i.e. the ioctl
to add pages to an enclave is extended to take PROT_{READ,WRITE,EXEC}
flags.
An enclave page's protections are the maximal protections that userspace
can use to map the page, e.g. mprotect() and mmap() are rejected if the
protections for the vma would be more permissible than those of the
associated enclave page.
Tracking protections for an enclave page (in additional to vmas) allows
SGX to invoke LSM upcalls while the enclave is being built. This is
critical to enabling LSMs to implement policies for enclave pages that
are functionally equivalent to existing policies for normal pages.
</boilerplate>
[1] https://lkml.kernel.org/r/20190614003759.GE18385@linux.intel.com
v4:
- Rename SGX__EXECMEM and SGX__EXECMOD to SGX__MAPWX and SGX_EXECDIRTY
respectively [Stephen].
- Fix an inverted check on IS_PRIVATE file check [Stephen].
- Take a '__u8 prot' in SGX_IOC_ENCLAVE_ADD_PAGE [Jarkko].
- Rebased to Jarkko's latest code base.
- Replace patch 1 with a variant that does encl_mm tracking via
mmu_notifier and SRCU. Not relevant for most people, but I wanted
to show the end state if we get rid of the per-vma tracking.
v3: https://patchwork.kernel.org/cover/11000601/
- Clear VM_MAY* flags instead of using .may_mprotect() to enforce
maximal enclave page protections.
- Update the SGX selftest to work with the new API.
- Rewrite SELinux code to use SGX specific permissions, with the goal
of addressing Andy's feedback regarding what people will actually
care about when it comes to SGX, e.g. add permissions for restricing
unmeasured code and stop trying to infer permissions from the source
of each enclave page.
- Add a (very minimal) AppArmor patch.
- Show line of sight to SGX2 support.
- Rebased to Jarkko's latest code base.
v2: https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherson@intel.com
- Dropped the patch(es) to extend the SGX UAPI to allow adding multiple
enclave pages in a single syscall [Jarkko].
- Reject ioctl() immediately on LSM denial [Stephen].
- Rework SELinux code to avoid checking EXEMEM multiple times [Stephen].
- Adding missing equivalents to existing selinux_file_protect() checks
[Stephen].
- Hold mmap_sem across copy_to_user() to prevent a TOCTOU race when
checking the source vma [Stephen].
- Stubify security_enclave_load() if !CONFIG_SECURITY [Stephen].
- Make flags a 32-bit field [Andy].
- Don't validate the SECINFO protection flags against the enclave
page's protection flags [Andy].
- Rename mprotect() hook to may_mprotect() [Andy].
- Test 'vma->vm_flags & VM_MAYEXEC' instead of manually checking for
a noexec path [Jarkko].
- Drop the SGX defined flags (use PROT_*) [Jarkko].
- Improve comments and changelogs [Jarkko].
v1: https://lkml.kernel.org/r/20190531233159.30992-1-sean.j.christopherson@intel.com
Sean Christopherson (12):
x86/sgx: Use mmu_notifier.release() instead of per-vma refcounting
x86/sgx: Do not naturally align MAP_FIXED address
selftests: x86/sgx: Mark the enclave loader as not needing an exec
stack
x86/sgx: Require userspace to define enclave pages' protection bits
x86/sgx: Enforce noexec filesystem restriction for enclaves
mm: Introduce vm_ops->may_mprotect()
LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX
security/selinux: Require SGX_MAPWX to map enclave page WX
LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
security/selinux: Add enclave_load() implementation
security/apparmor: Add enclave_load() implementation
LSM: x86/sgx: Show line of sight to LSM support SGX2's EAUG
arch/x86/Kconfig | 2 +
arch/x86/include/uapi/asm/sgx.h | 6 +-
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 69 ++++--
arch/x86/kernel/cpu/sgx/driver/main.c | 106 ++++++++-
arch/x86/kernel/cpu/sgx/encl.c | 277 ++++++++++++-----------
arch/x86/kernel/cpu/sgx/encl.h | 22 +-
arch/x86/kernel/cpu/sgx/reclaim.c | 71 ++----
include/linux/lsm_hooks.h | 20 ++
include/linux/mm.h | 2 +
include/linux/security.h | 18 ++
mm/mprotect.c | 15 +-
security/apparmor/include/audit.h | 2 +
security/apparmor/lsm.c | 14 ++
security/security.c | 12 +
security/selinux/hooks.c | 72 ++++++
security/selinux/include/classmap.h | 6 +-
tools/testing/selftests/x86/sgx/Makefile | 2 +-
tools/testing/selftests/x86/sgx/main.c | 32 ++-
18 files changed, 532 insertions(+), 216 deletions(-)
--
2.21.0
^ permalink raw reply
* [RFC PATCH v4 04/12] x86/sgx: Require userspace to define enclave pages' protection bits
From: Sean Christopherson @ 2019-06-19 22:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190619222401.14942-1-sean.j.christopherson@intel.com>
Existing Linux Security Module policies restrict userspace's ability to
map memory, e.g. may require priveleged permissions to map a page that
is simultaneously writable and executable. Said permissions are often
tied to the file which backs the mapped memory, i.e. vm_file.
For reasons explained below, SGX does not allow LSMs to enforce policies
using existing LSM hooks such as file_mprotect(). Explicitly track the
protection bits for an enclave page (separate from the vma/pte bits) and
require userspace to explicit define a page's protection bits when the
page is added to the enclave. Enclave page protection bits paves the
way to adding security_enclave_load() LSM hook as an SGX equivalent to
security_file_mprotect(), e.g. SGX can pass the page's protection bits
and source vma to LSMs. The source vma will allow LSMs to tie
permissions to files, e.g. the file containing the enclave's code and
initial data, and the protection bits will allow LSMs to make decisions
based on the capabilities of the process, e.g. if a process is allowed
to load unmeasured code or load code from anonymous memory.
Due to the nature of the Enclave Page Cache, and because the EPC is
manually managed by SGX, all enclave vmas are backed by the same file,
i.e. /dev/sgx/enclave. Specifically, a single file allows SGX to use
file op hooks to move pages in/out of the EPC.
Furthermore, EPC pages for any given enclave are fundamentally shared
between processes, i.e. CoW semantics are not possible with EPC pages
due to hardware restrictions such as 1:1 mappings between virtual and
physical addresses (within the enclave).
Lastly, all real world enclaves will need read, write and execute
permissions to EPC pages.
As a result, SGX does not play nice with existing LSM behavior as it is
impossible to apply policies to enclaves with reasonable granularity,
e.g. an LSM can deny access to EPC altogether, but can't deny
potentially unwanted behavior such as mapping pages WX, loading code
from anonymous memory, loading unmeasured code, etc...
For example, because all (practical) enclaves need RW pages for data and
RX pages for code, SELinux's existing policies will require all enclaves
to have FILE__READ, FILE__WRITE and FILE__EXECUTE permissions on
/dev/sgx/enclave. Witholding FILE__WRITE or FILE__EXECUTE in an attempt
to deny RW->RX or RWX would prevent running *any* enclave, even those
that cleanly separate RW and RX pages. And because /dev/sgx/enclave
requires MAP_SHARED, the anonymous/CoW checks that would trigger
FILE__EXECMOD or PROCESS__EXECMEM permissions will never fire.
Taking protection bits has a second use in that it can be used to
prevent loading an enclave from a noexec file system. On SGX2 hardware,
regardless of kernel support for SGX2, userspace could EADD a page from
a noexec path using read-only permissions and later mprotect() and
ENCLU[EMODPE] the page to gain execute permissions. By requiring
the enclave's page protections up front, SGX will be able to enforce
noexec paths when building enclaves.
To prevent userspace from circumventing the allowed protections, do not
allow PROT_{READ,WRITE,EXEC} mappings to an enclave without an
associated enclave page, i.e. prevent creating a mapping with unchecked
protection bits.
Many alternatives[1][2] have been explored, most notably the concept of
having SGX check (at load time) and save the permissions of the enclave
loader. The permissions would then be enforced by SGX at run time, e.g.
via mmap()/mprotect() hooks of some form. The basic functionality of
pre-checking permissions is relatively straightforward, but supporting
LSM auditing is complex and fraught with pitfalls. If auditing is done
at the time of denial then the audit logs will potentially show a large
number of false positives. Auditing when the denial is enforced, e.g.
at mprotect(), suffers from its own problems, e.g.:
- Requires LSMs to pre-generate audit messages so that they can be
replayed by SGX when the denial is actually enforced.
- System changes can result in stale audit messages, e.g. if files
are removed from the system, an LSM profile is modified, etc...
- A process could log what is essentially a false positive denial,
e.g. if the current process has the requisite capability but the
original enclave loader did not.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/include/uapi/asm/sgx.h | 6 ++--
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 15 +++++---
arch/x86/kernel/cpu/sgx/driver/main.c | 49 ++++++++++++++++++++++++++
arch/x86/kernel/cpu/sgx/encl.h | 1 +
tools/testing/selftests/x86/sgx/main.c | 32 +++++++++++++++--
5 files changed, 94 insertions(+), 9 deletions(-)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 6dba9f282232..67a3babbb24d 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -35,15 +35,17 @@ struct sgx_enclave_create {
* @src: address for the page data
* @secinfo: address for the SECINFO data
* @mrmask: bitmask for the measured 256 byte chunks
+ * @prot: maximal PROT_{READ,WRITE,EXEC} protections for the page
*/
struct sgx_enclave_add_page {
__u64 addr;
__u64 src;
__u64 secinfo;
- __u64 mrmask;
+ __u16 mrmask;
+ __u8 prot;
+ __u8 pad;
};
-
/**
* struct sgx_enclave_init - parameter structure for the
* %SGX_IOC_ENCLAVE_INIT ioctl
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 3552d642b26f..e18d2afd2aad 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -2,6 +2,7 @@
// Copyright(c) 2016-19 Intel Corporation.
#include <asm/mman.h>
+#include <linux/mman.h>
#include <linux/delay.h>
#include <linux/file.h>
#include <linux/hashtable.h>
@@ -235,7 +236,8 @@ static int sgx_validate_secs(const struct sgx_secs *secs,
}
static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
- unsigned long addr)
+ unsigned long addr,
+ unsigned long prot)
{
struct sgx_encl_page *encl_page;
int ret;
@@ -247,6 +249,7 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
return ERR_PTR(-ENOMEM);
encl_page->desc = addr;
encl_page->encl = encl;
+ encl_page->vm_prot_bits = calc_vm_prot_bits(prot, 0);
ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
encl_page);
if (ret) {
@@ -517,7 +520,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
void *data, struct sgx_secinfo *secinfo,
- unsigned int mrmask)
+ unsigned int mrmask, unsigned long prot)
{
u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
struct sgx_encl_page *encl_page;
@@ -543,7 +546,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
goto out;
}
- encl_page = sgx_encl_page_alloc(encl, addr);
+ encl_page = sgx_encl_page_alloc(encl, addr, prot);
if (IS_ERR(encl_page)) {
ret = PTR_ERR(encl_page);
goto out;
@@ -584,6 +587,7 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
struct sgx_enclave_add_page addp;
struct sgx_secinfo secinfo;
struct page *data_page;
+ unsigned long prot;
void *data;
int ret;
@@ -605,7 +609,10 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
goto out;
}
- ret = sgx_encl_add_page(encl, addp.addr, data, &secinfo, addp.mrmask);
+ prot = addp.prot & (PROT_READ | PROT_WRITE | PROT_EXEC);
+
+ ret = sgx_encl_add_page(encl, addp.addr, data, &secinfo, addp.mrmask,
+ prot);
if (ret)
goto out;
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 29384cdd0842..dabfe2a7245a 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -93,15 +93,64 @@ static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
}
#endif
+/*
+ * Returns the AND of VM_{READ,WRITE,EXEC} permissions across all pages
+ * covered by the specific VMA. A non-existent (or yet to be added) enclave
+ * page is considered to have no RWX permissions, i.e. is inaccessible.
+ */
+static unsigned long sgx_allowed_rwx(struct sgx_encl *encl,
+ struct vm_area_struct *vma)
+{
+ unsigned long allowed_rwx = VM_READ | VM_WRITE | VM_EXEC;
+ unsigned long idx, idx_start, idx_end;
+ struct sgx_encl_page *page;
+
+ idx_start = PFN_DOWN(vma->vm_start);
+ idx_end = PFN_DOWN(vma->vm_end - 1);
+
+ for (idx = idx_start; idx <= idx_end; ++idx) {
+ /*
+ * No need to take encl->lock, vm_prot_bits is set prior to
+ * insertion and never changes, and racing with adding pages is
+ * a userspace bug.
+ */
+ rcu_read_lock();
+ page = radix_tree_lookup(&encl->page_tree, idx);
+ rcu_read_unlock();
+
+ /* Do not allow R|W|X to a non-existent page. */
+ if (!page)
+ allowed_rwx = 0;
+ else
+ allowed_rwx &= page->vm_prot_bits;
+ if (!allowed_rwx)
+ break;
+ }
+
+ return allowed_rwx;
+}
+
static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
{
struct sgx_encl *encl = file->private_data;
+ unsigned long allowed_rwx;
int ret;
+ allowed_rwx = sgx_allowed_rwx(encl, vma);
+ if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) & ~allowed_rwx)
+ return -EACCES;
+
ret = sgx_encl_mm_add(encl, vma->vm_mm);
if (ret)
return ret;
+ if (!(allowed_rwx & VM_READ))
+ vma->vm_flags &= ~VM_MAYREAD;
+ if (!(allowed_rwx & VM_WRITE))
+ vma->vm_flags &= ~VM_MAYWRITE;
+ if (!(allowed_rwx & VM_EXEC))
+ vma->vm_flags &= ~VM_MAYEXEC;
+
vma->vm_ops = &sgx_vm_ops;
vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
vma->vm_private_data = encl;
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index 0904b3c20ed0..5ad018c8d74c 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -43,6 +43,7 @@ enum sgx_encl_page_desc {
struct sgx_encl_page {
unsigned long desc;
+ unsigned long vm_prot_bits;
struct sgx_epc_page *epc_page;
struct sgx_va_page *va_page;
struct sgx_encl *encl;
diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index e2265f841fb0..77e93f8e8a59 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -2,6 +2,7 @@
// Copyright(c) 2016-18 Intel Corporation.
#include <elf.h>
+#include <errno.h>
#include <fcntl.h>
#include <stdbool.h>
#include <stdio.h>
@@ -18,6 +19,8 @@
#include "../../../../../arch/x86/kernel/cpu/sgx/arch.h"
#include "../../../../../arch/x86/include/uapi/asm/sgx.h"
+#define PAGE_SIZE 4096
+
static const uint64_t MAGIC = 0x1122334455667788ULL;
struct vdso_symtab {
@@ -135,8 +138,7 @@ static bool encl_create(int dev_fd, unsigned long bin_size,
for (secs->size = 4096; secs->size < bin_size; )
secs->size <<= 1;
- base = mmap(NULL, secs->size, PROT_READ | PROT_WRITE | PROT_EXEC,
- MAP_SHARED, dev_fd, 0);
+ base = mmap(NULL, secs->size, PROT_NONE, MAP_SHARED, dev_fd, 0);
if (base == MAP_FAILED) {
perror("mmap");
return false;
@@ -147,7 +149,7 @@ static bool encl_create(int dev_fd, unsigned long bin_size,
ioc.src = (unsigned long)secs;
rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_CREATE, &ioc);
if (rc) {
- fprintf(stderr, "ECREATE failed rc=%d.\n", rc);
+ fprintf(stderr, "ECREATE failed rc=%d, err=%d.\n", rc, errno);
munmap(base, secs->size);
return false;
}
@@ -160,8 +162,14 @@ static bool encl_add_page(int dev_fd, unsigned long addr, void *data,
{
struct sgx_enclave_add_page ioc;
struct sgx_secinfo secinfo;
+ unsigned long prot;
int rc;
+ if (flags == SGX_SECINFO_TCS)
+ prot = PROT_READ | PROT_WRITE;
+ else
+ prot = PROT_READ | PROT_WRITE | PROT_EXEC;
+
memset(&secinfo, 0, sizeof(secinfo));
secinfo.flags = flags;
@@ -169,6 +177,7 @@ static bool encl_add_page(int dev_fd, unsigned long addr, void *data,
ioc.mrmask = 0xFFFF;
ioc.addr = addr;
ioc.src = (uint64_t)data;
+ ioc.prot = prot;
rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_ADD_PAGE, &ioc);
if (rc) {
@@ -184,6 +193,7 @@ static bool encl_load(struct sgx_secs *secs, unsigned long bin_size)
struct sgx_enclave_init ioc;
uint64_t offset;
uint64_t flags;
+ void *addr;
int dev_fd;
int rc;
@@ -215,6 +225,22 @@ static bool encl_load(struct sgx_secs *secs, unsigned long bin_size)
goto out_map;
}
+ addr = mmap((void *)secs->base, PAGE_SIZE, PROT_READ | PROT_WRITE,
+ MAP_SHARED | MAP_FIXED, dev_fd, 0);
+ if (addr == MAP_FAILED) {
+ fprintf(stderr, "mmap() failed on TCS, errno=%d.\n", errno);
+ return false;
+ }
+
+ addr = mmap((void *)(secs->base + PAGE_SIZE), bin_size - PAGE_SIZE,
+ PROT_READ | PROT_WRITE | PROT_EXEC,
+ MAP_SHARED | MAP_FIXED, dev_fd, 0);
+ if (addr == MAP_FAILED) {
+ fprintf(stderr, "mmap() failed, errno=%d.\n", errno);
+ return false;
+ }
+
+
close(dev_fd);
return true;
out_map:
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v4 05/12] x86/sgx: Enforce noexec filesystem restriction for enclaves
From: Sean Christopherson @ 2019-06-19 22:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190619222401.14942-1-sean.j.christopherson@intel.com>
Do not allow an enclave page to be mapped with PROT_EXEC if the source
vma does not have VM_MAYEXEC. This effectively enforces noexec as
do_mmap() clears VM_MAYEXEC if the vma is being loaded from a noexec
path, i.e. prevents executing a file by loading it into an enclave.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 42 +++++++++++++++++++++++---
1 file changed, 37 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index e18d2afd2aad..1fca70a36ce3 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -564,6 +564,39 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
return ret;
}
+static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot)
+{
+ struct vm_area_struct *vma;
+ int ret;
+
+ /* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */
+ down_read(¤t->mm->mmap_sem);
+
+ /* Query vma's VM_MAYEXEC as an indirect path_noexec() check. */
+ if (prot & PROT_EXEC) {
+ vma = find_vma(current->mm, src);
+ if (!vma) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if (!(vma->vm_flags & VM_MAYEXEC)) {
+ ret = -EACCES;
+ goto out;
+ }
+ }
+
+ if (copy_from_user(dst, (void __user *)src, PAGE_SIZE))
+ ret = -EFAULT;
+ else
+ ret = 0;
+
+out:
+ up_read(¤t->mm->mmap_sem);
+
+ return ret;
+}
+
/**
* sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE
*
@@ -604,13 +637,12 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
data = kmap(data_page);
- if (copy_from_user((void *)data, (void __user *)addp.src, PAGE_SIZE)) {
- ret = -EFAULT;
- goto out;
- }
-
prot = addp.prot & (PROT_READ | PROT_WRITE | PROT_EXEC);
+ ret = sgx_encl_page_copy(data, addp.src, prot);
+ if (ret)
+ goto out;
+
ret = sgx_encl_add_page(encl, addp.addr, data, &secinfo, addp.mrmask,
prot);
if (ret)
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v4 02/12] x86/sgx: Do not naturally align MAP_FIXED address
From: Sean Christopherson @ 2019-06-19 22:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190619222401.14942-1-sean.j.christopherson@intel.com>
SGX enclaves have an associated Enclave Linear Range (ELRANGE) that is
tracked and enforced by the CPU using a base+mask approach, similar to
how hardware range registers such as the variable MTRRs. As a result,
the ELRANGE must be naturally sized and aligned.
To reduce boilerplate code that would be needed in every userspace
enclave loader, the SGX driver naturally aligns the mmap() address and
also requires the range to be naturally sized. Unfortunately, SGX fails
to grant a waiver to the MAP_FIXED case, e.g. incorrectly rejects mmap()
if userspace is attempting to map a small slice of an existing enclave.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kernel/cpu/sgx/driver/main.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 07aa5f91b2dd..29384cdd0842 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -115,7 +115,13 @@ static unsigned long sgx_get_unmapped_area(struct file *file,
unsigned long pgoff,
unsigned long flags)
{
- if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE)
+ if (flags & MAP_PRIVATE)
+ return -EINVAL;
+
+ if (flags & MAP_FIXED)
+ return addr;
+
+ if (len < 2 * PAGE_SIZE || len & (len - 1))
return -EINVAL;
addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v4 07/12] LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX
From: Sean Christopherson @ 2019-06-19 22:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190619222401.14942-1-sean.j.christopherson@intel.com>
enclave_map() is an SGX specific variant of file_mprotect() and
mmap_file(), and is provided so that LSMs can apply W^X restrictions to
enclaves.
Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
MAP_SHARED. Furthermore, all enclaves need read, write and execute
VMAs. As a result, applying W^X restrictions on /dev/sgx/enclave using
existing LSM hooks is for all intents and purposes impossible, e.g.
denying either W or X would deny access to any enclave.
Note, extensive discussion yielded no sane alternative to some form of
SGX specific LSM hook[1].
[1] https://lkml.kernel.org/r/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kernel/cpu/sgx/driver/main.c | 9 ++++++++-
arch/x86/kernel/cpu/sgx/encl.c | 12 ++++++++++++
include/linux/lsm_hooks.h | 12 ++++++++++++
include/linux/security.h | 11 +++++++++++
security/security.c | 7 +++++++
5 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index dabfe2a7245a..4379a2fb1f82 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -133,13 +133,20 @@ static unsigned long sgx_allowed_rwx(struct sgx_encl *encl,
static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
{
struct sgx_encl *encl = file->private_data;
- unsigned long allowed_rwx;
+ unsigned long allowed_rwx, prot;
int ret;
allowed_rwx = sgx_allowed_rwx(encl, vma);
if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) & ~allowed_rwx)
return -EACCES;
+ prot = _calc_vm_trans(vma->vm_flags, VM_READ, PROT_READ) |
+ _calc_vm_trans(vma->vm_flags, VM_WRITE, PROT_WRITE) |
+ _calc_vm_trans(vma->vm_flags, VM_EXEC, PROT_EXEC);
+ ret = security_enclave_map(prot);
+ if (ret)
+ return ret;
+
ret = sgx_encl_mm_add(encl, vma->vm_mm);
if (ret)
return ret;
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index c6436bbd4a68..059d90dcaa27 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -2,6 +2,7 @@
// Copyright(c) 2016-18 Intel Corporation.
#include <linux/mm.h>
+#include <linux/security.h>
#include <linux/shmem_fs.h>
#include <linux/suspend.h>
#include <linux/sched/mm.h>
@@ -387,9 +388,20 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
return ret < 0 ? ret : i;
}
+#ifdef CONFIG_SECURITY
+static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start,
+ unsigned long end, unsigned long prot)
+{
+ return security_enclave_map(prot);
+}
+#endif
+
const struct vm_operations_struct sgx_vm_ops = {
.fault = sgx_vma_fault,
.access = sgx_vma_access,
+#ifdef CONFIG_SECURITY
+ .may_mprotect = sgx_vma_mprotect,
+#endif
};
/**
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..7c1357105e61 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1446,6 +1446,11 @@
* @bpf_prog_free_security:
* Clean up the security information stored inside bpf prog.
*
+ * Security hooks for Intel SGX enclaves.
+ *
+ * @enclave_map:
+ * @prot contains the protection that will be applied by the kernel.
+ * Return 0 if permission is granted.
*/
union security_list_options {
int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1807,6 +1812,10 @@ union security_list_options {
int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
#endif /* CONFIG_BPF_SYSCALL */
+
+#ifdef CONFIG_INTEL_SGX
+ int (*enclave_map)(unsigned long prot);
+#endif /* CONFIG_INTEL_SGX */
};
struct security_hook_heads {
@@ -2046,6 +2055,9 @@ struct security_hook_heads {
struct hlist_head bpf_prog_alloc_security;
struct hlist_head bpf_prog_free_security;
#endif /* CONFIG_BPF_SYSCALL */
+#ifdef CONFIG_INTEL_SGX
+ struct hlist_head enclave_map;
+#endif /* CONFIG_INTEL_SGX */
} __randomize_layout;
/*
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..6a1f54ba6794 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1829,5 +1829,16 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
#endif /* CONFIG_SECURITY */
#endif /* CONFIG_BPF_SYSCALL */
+#ifdef CONFIG_INTEL_SGX
+#ifdef CONFIG_SECURITY
+int security_enclave_map(unsigned long prot);
+#else
+static inline int security_enclave_map(unsigned long prot)
+{
+ return 0;
+}
+#endif /* CONFIG_SECURITY */
+#endif /* CONFIG_INTEL_SGX */
+
#endif /* ! __LINUX_SECURITY_H */
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..03951e08bdfc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2359,3 +2359,10 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
call_void_hook(bpf_prog_free_security, aux);
}
#endif /* CONFIG_BPF_SYSCALL */
+
+#ifdef CONFIG_INTEL_SGX
+int security_enclave_map(unsigned long prot)
+{
+ return call_int_hook(enclave_map, 0, prot);
+}
+#endif /* CONFIG_INTEL_SGX */
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v4 11/12] security/apparmor: Add enclave_load() implementation
From: Sean Christopherson @ 2019-06-19 22:24 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190619222401.14942-1-sean.j.christopherson@intel.com>
Require execute permissions when loading an enclave from a file.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
security/apparmor/include/audit.h | 2 ++
security/apparmor/lsm.c | 14 ++++++++++++++
2 files changed, 16 insertions(+)
diff --git a/security/apparmor/include/audit.h b/security/apparmor/include/audit.h
index ee559bc2acb8..84470483e04d 100644
--- a/security/apparmor/include/audit.h
+++ b/security/apparmor/include/audit.h
@@ -107,6 +107,8 @@ enum audit_type {
#define OP_PROF_LOAD "profile_load"
#define OP_PROF_RM "profile_remove"
+#define OP_ENCL_LOAD "enclave_load"
+
struct apparmor_audit_data {
int error;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 87500bde5a92..2ed1157e1f58 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -517,6 +517,17 @@ static int apparmor_file_mprotect(struct vm_area_struct *vma,
!(vma->vm_flags & VM_SHARED) ? MAP_PRIVATE : 0);
}
+#ifdef CONFIG_INTEL_SGX
+static int apparmor_enclave_load(struct vm_area_struct *vma, unsigned long prot,
+ bool measured)
+{
+ if (!(prot & PROT_EXEC))
+ return 0;
+
+ return common_file_perm(OP_ENCL_LOAD, vma->vm_file, AA_EXEC_MMAP);
+}
+#endif
+
static int apparmor_sb_mount(const char *dev_name, const struct path *path,
const char *type, unsigned long flags, void *data)
{
@@ -1243,6 +1254,9 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(secid_to_secctx, apparmor_secid_to_secctx),
LSM_HOOK_INIT(secctx_to_secid, apparmor_secctx_to_secid),
LSM_HOOK_INIT(release_secctx, apparmor_release_secctx),
+#ifdef CONFIG_INTEL_SGX
+ LSM_HOOK_INIT(enclave_load, apparmor_enclave_load),
+#endif
};
/*
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
From: Sean Christopherson @ 2019-06-19 22:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190619222401.14942-1-sean.j.christopherson@intel.com>
The goal of selinux_enclave_load() is to provide a facsimile of the
existing selinux_file_mprotect() and file_map_prot_check() policies,
but tailored to the unique properties of SGX.
For example, an enclave page is technically backed by a MAP_SHARED file,
but the "file" is essentially shared memory that is never persisted
anywhere and also requires execute permissions (for some pages).
Enclaves are also less priveleged than normal user code, e.g. SYSCALL
instructions #UD if attempted in an enclave. For this reason, add SGX
specific permissions instead of reusing existing permissions such as
FILE__EXECUTE so that policies can allow running code in an enclave, or
allow dynamically loading code in an enclave without having to grant the
same capability to normal user code outside of the enclave.
Intended use of each permission:
- SGX_EXECDIRTY: dynamically load code within the enclave itself
- SGX_EXECUNMR: load unmeasured code into the enclave, e.g. Graphene
- SGX_EXECANON: load code from anonymous memory (likely Graphene)
- SGX_EXECUTE: load an enclave from a file, i.e. normal behavior
Note, equivalents to FILE__READ and FILE__WRITE are intentionally never
required. Writes to the enclave page are contained to the EPC, i.e.
never hit the original file, and read permissions have already been
vetted (or the VMA doesn't have PROT_READ, in which case loading the
page into the enclave will fail).
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
security/selinux/hooks.c | 55 +++++++++++++++++++++++++++--
security/selinux/include/classmap.h | 5 +--
2 files changed, 55 insertions(+), 5 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index fc239e541b62..8a431168e454 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6727,6 +6727,12 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
#endif
#ifdef CONFIG_INTEL_SGX
+static inline int sgx_has_perm(u32 sid, u32 requested)
+{
+ return avc_has_perm(&selinux_state, sid, sid,
+ SECCLASS_PROCESS2, requested, NULL);
+}
+
static int selinux_enclave_map(unsigned long prot)
{
const struct cred *cred = current_cred();
@@ -6736,11 +6742,53 @@ static int selinux_enclave_map(unsigned long prot)
WARN_ON_ONCE(!default_noexec);
if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
- return avc_has_perm(&selinux_state, sid, sid,
- SECCLASS_PROCESS2, PROCESS2__SGX_MAPWX,
- NULL);
+ return sgx_has_perm(sid, PROCESS2__SGX_MAPWX);
+
return 0;
}
+
+static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
+ bool measured)
+{
+ const struct cred *cred = current_cred();
+ u32 sid = cred_sid(cred);
+ int ret;
+
+ /* SGX is supported only in 64-bit kernels. */
+ WARN_ON_ONCE(!default_noexec);
+
+ /* Only executable enclave pages are restricted in any way. */
+ if (!(prot & PROT_EXEC))
+ return 0;
+
+ /*
+ * WX at load time only requires EXECDIRTY, e.g. to allow W->X. Actual
+ * WX mappings require MAPWX (see selinux_enclave_map()).
+ */
+ if (prot & PROT_WRITE) {
+ ret = sgx_has_perm(sid, PROCESS2__SGX_EXECDIRTY);
+ if (ret)
+ goto out;
+ }
+ if (!measured) {
+ ret = sgx_has_perm(sid, PROCESS2__SGX_EXECUNMR);
+ if (ret)
+ goto out;
+ }
+
+ if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file)) ||
+ vma->anon_vma)
+ /*
+ * Loading enclave code from an anonymous mapping or from a
+ * modified private file mapping.
+ */
+ ret = sgx_has_perm(sid, PROCESS2__SGX_EXECANON);
+ else
+ /* Loading from a shared or unmodified private file mapping. */
+ ret = file_has_perm(cred, vma->vm_file, FILE__SGX_EXECUTE);
+out:
+ return ret;
+}
#endif
struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
@@ -6988,6 +7036,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
#ifdef CONFIG_INTEL_SGX
LSM_HOOK_INIT(enclave_map, selinux_enclave_map),
+ LSM_HOOK_INIT(enclave_load, selinux_enclave_load),
#endif
};
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index cfd91e879bdf..baa1757be46a 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -7,7 +7,7 @@
#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
"rename", "execute", "quotaon", "mounton", "audit_access", \
- "open", "execmod"
+ "open", "execmod", "sgx_execute"
#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
"listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
@@ -52,7 +52,8 @@ struct security_class_mapping secclass_map[] = {
"setsockcreate", "getrlimit", NULL } },
{ "process2",
{ "nnp_transition", "nosuid_transition",
- "sgx_mapwx", NULL } },
+ "sgx_mapwx", "sgx_execdirty", "sgx_execanon", "sgx_execunmr",
+ NULL } },
{ "system",
{ "ipc_info", "syslog_read", "syslog_mod",
"syslog_console", "module_request", "module_load", NULL } },
--
2.21.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox