* [PATCH v6 3/3] Wire UFFD up to SELinux
From: Lokesh Gidra @ 2020-08-07 22:49 UTC (permalink / raw)
To: Alexander Viro, James Morris, Stephen Smalley, Casey Schaufler,
Eric Biggers
Cc: Serge E. Hallyn, Paul Moore, Eric Paris, Lokesh Gidra,
Daniel Colascione, Kees Cook, Eric W. Biederman, KP Singh,
David Howells, Thomas Cedeno, Anders Roxell, Sami Tolvanen,
Matthew Garrett, Aaron Goidel, Randy Dunlap,
Joel Fernandes (Google), YueHaibing, Christian Brauner,
Alexei Starovoitov, Alexey Budankov, Adrian Reber, Aleksa Sarai,
linux-fsdevel, linux-kernel, linux-security-module, selinux,
kaleshsingh, calin, surenb, nnk, jeffv, kernel-team,
Daniel Colascione
In-Reply-To: <20200807224941.3440722-1-lokeshgidra@google.com>
From: Daniel Colascione <dancol@google.com>
This change gives userfaultfd file descriptors a real security
context, allowing policy to act on them.
Signed-off-by: Daniel Colascione <dancol@google.com>
[Remove owner inode from userfaultfd_ctx]
[Use anon_inode_getfd_secure() instead of anon_inode_getfile_secure()
in userfaultfd syscall]
[Use inode of file in userfaultfd_read() in resolve_userfault_fork()]
Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
fs/userfaultfd.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 6e264dded46e..23c8618ebe35 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -978,14 +978,16 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
static const struct file_operations userfaultfd_fops;
-static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
- struct userfaultfd_ctx *new,
+static int resolve_userfault_fork(struct userfaultfd_ctx *new,
+ struct inode *inode,
struct uffd_msg *msg)
{
int fd;
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, new,
- O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS));
+ fd = anon_inode_getfd_secure(
+ "[userfaultfd]", &userfaultfd_fops, new,
+ O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS),
+ inode);
if (fd < 0)
return fd;
@@ -995,7 +997,7 @@ static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
}
static ssize_t userfaultfd_ctx_read(struct userfaultfd_ctx *ctx, int no_wait,
- struct uffd_msg *msg)
+ struct uffd_msg *msg, struct inode *inode)
{
ssize_t ret;
DECLARE_WAITQUEUE(wait, current);
@@ -1106,7 +1108,7 @@ static ssize_t userfaultfd_ctx_read(struct userfaultfd_ctx *ctx, int no_wait,
spin_unlock_irq(&ctx->fd_wqh.lock);
if (!ret && msg->event == UFFD_EVENT_FORK) {
- ret = resolve_userfault_fork(ctx, fork_nctx, msg);
+ ret = resolve_userfault_fork(fork_nctx, inode, msg);
spin_lock_irq(&ctx->event_wqh.lock);
if (!list_empty(&fork_event)) {
/*
@@ -1166,6 +1168,7 @@ static ssize_t userfaultfd_read(struct file *file, char __user *buf,
ssize_t _ret, ret = 0;
struct uffd_msg msg;
int no_wait = file->f_flags & O_NONBLOCK;
+ struct inode *inode = file_inode(file);
if (ctx->state == UFFD_STATE_WAIT_API)
return -EINVAL;
@@ -1173,7 +1176,7 @@ static ssize_t userfaultfd_read(struct file *file, char __user *buf,
for (;;) {
if (count < sizeof(msg))
return ret ? ret : -EINVAL;
- _ret = userfaultfd_ctx_read(ctx, no_wait, &msg);
+ _ret = userfaultfd_ctx_read(ctx, no_wait, &msg, inode);
if (_ret < 0)
return ret ? ret : _ret;
if (copy_to_user((__u64 __user *) buf, &msg, sizeof(msg)))
@@ -1995,8 +1998,10 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
/* prevent the mm struct to be freed */
mmgrab(ctx->mm);
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, ctx,
- O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS));
+ fd = anon_inode_getfd_secure("[userfaultfd]",
+ &userfaultfd_fops, ctx,
+ O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS),
+ NULL);
if (fd < 0) {
mmdrop(ctx->mm);
kmem_cache_free(userfaultfd_ctx_cachep, ctx);
--
2.28.0.236.gb10cc79966-goog
^ permalink raw reply related
* [PATCH v6 1/3] Add a new LSM-supporting anonymous inode interface
From: Lokesh Gidra @ 2020-08-07 22:49 UTC (permalink / raw)
To: Alexander Viro, James Morris, Stephen Smalley, Casey Schaufler,
Eric Biggers
Cc: Serge E. Hallyn, Paul Moore, Eric Paris, Lokesh Gidra,
Daniel Colascione, Kees Cook, Eric W. Biederman, KP Singh,
David Howells, Thomas Cedeno, Anders Roxell, Sami Tolvanen,
Matthew Garrett, Aaron Goidel, Randy Dunlap,
Joel Fernandes (Google), YueHaibing, Christian Brauner,
Alexei Starovoitov, Alexey Budankov, Adrian Reber, Aleksa Sarai,
linux-fsdevel, linux-kernel, linux-security-module, selinux,
kaleshsingh, calin, surenb, nnk, jeffv, kernel-team,
Daniel Colascione
In-Reply-To: <20200807224941.3440722-1-lokeshgidra@google.com>
From: Daniel Colascione <dancol@google.com>
This change adds two new functions, anon_inode_getfile_secure and
anon_inode_getfd_secure, that create anonymous-node files with
individual non-S_PRIVATE inodes to which security modules can apply
policy. Existing callers continue using the original singleton-inode
kind of anonymous-inode file. We can transition anonymous inode users
to the new kind of anonymous inode in individual patches for the sake
of bisection and review.
The new functions accept an optional context_inode parameter that
callers can use to provide additional contextual information to
security modules, e.g., indicating that one anonymous struct file is a
logical child of another, allowing a security model to propagate
security information from one to the other.
Signed-off-by: Daniel Colascione <dancol@google.com>
[Fix comment documenting return values of inode_init_security_anon()]
[Add context_inode description in comments to anon_inode_getfd_secure()
and anon_inode_getfile_secure()]
[Make _anon_inode_getfile() static]
[Use correct error cast in _anon_inode_getfile()]
Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
fs/anon_inodes.c | 193 ++++++++++++++++++++++++++--------
include/linux/anon_inodes.h | 13 +++
include/linux/lsm_hook_defs.h | 2 +
include/linux/lsm_hooks.h | 7 ++
include/linux/security.h | 3 +
security/security.c | 9 ++
6 files changed, 186 insertions(+), 41 deletions(-)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b..4e09915fe847 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -55,61 +55,109 @@ static struct file_system_type anon_inode_fs_type = {
.kill_sb = kill_anon_super,
};
-/**
- * anon_inode_getfile - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
- *
- * @name: [in] name of the "class" of the new file
- * @fops: [in] file operations for the new file
- * @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
- *
- * Creates a new file by hooking it on a single inode. This is useful for files
- * that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfile() will share a single inode,
- * hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns the newly created file* or an error pointer.
- */
-struct file *anon_inode_getfile(const char *name,
- const struct file_operations *fops,
- void *priv, int flags)
+static struct inode *anon_inode_make_secure_inode(
+ const char *name,
+ const struct inode *context_inode)
{
+ struct inode *inode;
+ const struct qstr qname = QSTR_INIT(name, strlen(name));
+ int error;
+
+ inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
+ if (IS_ERR(inode))
+ return inode;
+ inode->i_flags &= ~S_PRIVATE;
+ error = security_inode_init_security_anon(
+ inode, &qname, context_inode);
+ if (error) {
+ iput(inode);
+ return ERR_PTR(error);
+ }
+ return inode;
+}
+
+static struct file *_anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
+{
+ struct inode *inode;
struct file *file;
- if (IS_ERR(anon_inode_inode))
- return ERR_PTR(-ENODEV);
+ if (secure) {
+ inode = anon_inode_make_secure_inode(
+ name, context_inode);
+ if (IS_ERR(inode))
+ return ERR_CAST(inode);
+ } else {
+ inode = anon_inode_inode;
+ if (IS_ERR(inode))
+ return ERR_PTR(-ENODEV);
+ /*
+ * We know the anon_inode inode count is always
+ * greater than zero, so ihold() is safe.
+ */
+ ihold(inode);
+ }
- if (fops->owner && !try_module_get(fops->owner))
- return ERR_PTR(-ENOENT);
+ if (fops->owner && !try_module_get(fops->owner)) {
+ file = ERR_PTR(-ENOENT);
+ goto err;
+ }
- /*
- * We know the anon_inode inode count is always greater than zero,
- * so ihold() is safe.
- */
- ihold(anon_inode_inode);
- file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name,
+ file = alloc_file_pseudo(inode, anon_inode_mnt, name,
flags & (O_ACCMODE | O_NONBLOCK), fops);
if (IS_ERR(file))
goto err;
- file->f_mapping = anon_inode_inode->i_mapping;
+ file->f_mapping = inode->i_mapping;
file->private_data = priv;
return file;
err:
- iput(anon_inode_inode);
+ iput(inode);
module_put(fops->owner);
return file;
}
-EXPORT_SYMBOL_GPL(anon_inode_getfile);
/**
- * anon_inode_getfd - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
+ * anon_inode_getfile_secure - creates a new file instance by hooking
+ * it up to a new anonymous inode and a
+ * dentry that describe the "class" of the
+ * file. Make it possible to use security
+ * modules to control access to the
+ * new file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ * @context_inode [in] inode for additional contextual info to security modules
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged filesystem
+ * to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns the
+ * newly created file* or an error pointer.
+ */
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfile(
+ name, fops, priv, flags, context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile_secure);
+
+/**
+ * anon_inode_getfile - creates a new file instance by hooking it up to an
+ * anonymous inode, and a dentry that describe the "class"
+ * of the file
*
* @name: [in] name of the "class" of the new file
* @fops: [in] file operations for the new file
@@ -118,12 +166,23 @@ EXPORT_SYMBOL_GPL(anon_inode_getfile);
*
* Creates a new file by hooking it on a single inode. This is useful for files
* that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfd() will share a single inode,
+ * All the files created with anon_inode_getfile() will share a single inode,
* hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns new descriptor or an error code.
+ * setup. Returns the newly created file* or an error pointer.
*/
-int anon_inode_getfd(const char *name, const struct file_operations *fops,
- void *priv, int flags)
+struct file *anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfile(name, fops, priv, flags, NULL, false);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile);
+
+static int _anon_inode_getfd(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
int error, fd;
struct file *file;
@@ -133,7 +192,8 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
return error;
fd = error;
- file = anon_inode_getfile(name, fops, priv, flags);
+ file = _anon_inode_getfile(name, fops, priv, flags, context_inode,
+ secure);
if (IS_ERR(file)) {
error = PTR_ERR(file);
goto err_put_unused_fd;
@@ -146,6 +206,58 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
put_unused_fd(fd);
return error;
}
+
+/**
+ * anon_inode_getfd_secure - creates a new file instance by hooking it
+ * up to a new anonymous inode and a dentry
+ * that describe the "class" of the file.
+ * Make it possible to use security modules
+ * to control access to the new file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ * @context_inode [in] inode for additional contextual info to security modules
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged filesystem
+ * to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns a newly
+ * created file descriptor or an error code.
+ */
+int anon_inode_getfd_secure(const char *name, const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfd(name, fops, priv, flags,
+ context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);
+
+/**
+ * anon_inode_getfd - creates a new file instance by hooking it up to
+ * an anonymous inode and a dentry that describe
+ * the "class" of the file
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on a single inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile() will use the same singleton inode, reducing
+ * memory use and avoiding code duplication for the file/inode/dentry
+ * setup. Returns a newly created file descriptor or an error code.
+ */
+int anon_inode_getfd(const char *name, const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfd(name, fops, priv, flags, NULL, false);
+}
EXPORT_SYMBOL_GPL(anon_inode_getfd);
static int __init anon_inode_init(void)
@@ -162,4 +274,3 @@ static int __init anon_inode_init(void)
}
fs_initcall(anon_inode_init);
-
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261ad..67bd85d92dca 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -10,12 +10,25 @@
#define _LINUX_ANON_INODES_H
struct file_operations;
+struct inode;
+
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+
+int anon_inode_getfd_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
+
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
+
#endif /* _LINUX_ANON_INODES_H */
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index af998f93d256..613a3d6306c2 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -113,6 +113,8 @@ LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
struct inode *dir, const struct qstr *qstr, const char **name,
void **value, size_t *len)
+LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
+ const struct qstr *name, const struct inode *context_inode)
LSM_HOOK(int, 0, inode_create, struct inode *dir, struct dentry *dentry,
umode_t mode)
LSM_HOOK(int, 0, inode_link, struct dentry *old_dentry, struct inode *dir,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 95b7c1d32062..22847380c26c 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -233,6 +233,13 @@
* Returns 0 if @name and @value have been successfully set,
* -EOPNOTSUPP if no security attribute is needed, or
* -ENOMEM on memory allocation failure.
+ * @inode_init_security_anon:
+ * Set up a secure anonymous inode.
+ * @inode contains the inode structure
+ * @name name of the anonymous inode class
+ * @context_inode optional related inode
+ * Returns 0 on success, -EACCESS if the security module denies the
+ * creation of this inode, or another -errno upon other errors.
* @inode_create:
* Check permission to create a regular file.
* @dir contains inode structure of the parent of the new file.
diff --git a/include/linux/security.h b/include/linux/security.h
index 0a0a03b36a3b..95c133a8f8bb 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -322,6 +322,9 @@ void security_inode_free(struct inode *inode);
int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
+int security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len);
diff --git a/security/security.c b/security/security.c
index 70a7ad357bc6..149b3f024e2d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1057,6 +1057,15 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
}
EXPORT_SYMBOL(security_inode_init_security);
+int
+security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode)
+{
+ return call_int_hook(inode_init_security_anon, 0, inode, name,
+ context_inode);
+}
+
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len)
--
2.28.0.236.gb10cc79966-goog
^ permalink raw reply related
* [PATCH v6 2/3] Teach SELinux about anonymous inodes
From: Lokesh Gidra @ 2020-08-07 22:49 UTC (permalink / raw)
To: Alexander Viro, James Morris, Stephen Smalley, Casey Schaufler,
Eric Biggers
Cc: Serge E. Hallyn, Paul Moore, Eric Paris, Lokesh Gidra,
Daniel Colascione, Kees Cook, Eric W. Biederman, KP Singh,
David Howells, Thomas Cedeno, Anders Roxell, Sami Tolvanen,
Matthew Garrett, Aaron Goidel, Randy Dunlap,
Joel Fernandes (Google), YueHaibing, Christian Brauner,
Alexei Starovoitov, Alexey Budankov, Adrian Reber, Aleksa Sarai,
linux-fsdevel, linux-kernel, linux-security-module, selinux,
kaleshsingh, calin, surenb, nnk, jeffv, kernel-team,
Daniel Colascione, Andrew Morton
In-Reply-To: <20200807224941.3440722-1-lokeshgidra@google.com>
From: Daniel Colascione <dancol@google.com>
This change uses the anon_inodes and LSM infrastructure introduced in
the previous patch to give SELinux the ability to control
anonymous-inode files that are created using the new _secure()
anon_inodes functions.
A SELinux policy author detects and controls these anonymous inodes by
adding a name-based type_transition rule that assigns a new security
type to anonymous-inode files created in some domain. The name used
for the name-based transition is the name associated with the
anonymous inode for file listings --- e.g., "[userfaultfd]" or
"[perf_event]".
Example:
type uffd_t;
type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
allow sysadm_t uffd_t:anon_inode { create };
(The next patch in this series is necessary for making userfaultfd
support this new interface. The example above is just
for exposition.)
Signed-off-by: Daniel Colascione <dancol@google.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: James Morris <jmorris@namei.org>
---
security/selinux/hooks.c | 53 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 ++
2 files changed, 55 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ca901025802a..5b403ad44aad 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2926,6 +2926,58 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
return 0;
}
+static int selinux_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode)
+{
+ const struct task_security_struct *tsec = selinux_cred(current_cred());
+ struct common_audit_data ad;
+ struct inode_security_struct *isec;
+ int rc;
+
+ if (unlikely(!selinux_state.initialized))
+ return 0;
+
+ isec = selinux_inode(inode);
+
+ /*
+ * We only get here once per ephemeral inode. The inode has
+ * been initialized via inode_alloc_security but is otherwise
+ * untouched.
+ */
+
+ if (context_inode) {
+ struct inode_security_struct *context_isec =
+ selinux_inode(context_inode);
+ isec->sclass = context_isec->sclass;
+ isec->sid = context_isec->sid;
+ } else {
+ isec->sclass = SECCLASS_ANON_INODE;
+ rc = security_transition_sid(
+ &selinux_state, tsec->sid, tsec->sid,
+ isec->sclass, name, &isec->sid);
+ if (rc)
+ return rc;
+ }
+
+ isec->initialized = LABEL_INITIALIZED;
+
+ /*
+ * Now that we've initialized security, check whether we're
+ * allowed to actually create this type of anonymous inode.
+ */
+
+ ad.type = LSM_AUDIT_DATA_INODE;
+ ad.u.inode = inode;
+
+ return avc_has_perm(&selinux_state,
+ tsec->sid,
+ isec->sid,
+ isec->sclass,
+ FILE__CREATE,
+ &ad);
+}
+
static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
{
return may_create(dir, dentry, SECCLASS_FILE);
@@ -6993,6 +7045,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_free_security, selinux_inode_free_security),
LSM_HOOK_INIT(inode_init_security, selinux_inode_init_security),
+ LSM_HOOK_INIT(inode_init_security_anon, selinux_inode_init_security_anon),
LSM_HOOK_INIT(inode_create, selinux_inode_create),
LSM_HOOK_INIT(inode_link, selinux_inode_link),
LSM_HOOK_INIT(inode_unlink, selinux_inode_unlink),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 40cebde62856..ba2e01a6955c 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -249,6 +249,8 @@ struct security_class_mapping secclass_map[] = {
{"open", "cpu", "kernel", "tracepoint", "read", "write"} },
{ "lockdown",
{ "integrity", "confidentiality", NULL } },
+ { "anon_inode",
+ { COMMON_FILE_PERMS, NULL } },
{ NULL }
};
--
2.28.0.236.gb10cc79966-goog
^ permalink raw reply related
* [PATCH v6 0/3] SELinux support for anonymous inodes and UFFD
From: Lokesh Gidra @ 2020-08-07 22:49 UTC (permalink / raw)
To: Alexander Viro, James Morris, Stephen Smalley, Casey Schaufler,
Eric Biggers
Cc: Serge E. Hallyn, Paul Moore, Eric Paris, Lokesh Gidra,
Daniel Colascione, Kees Cook, Eric W. Biederman, KP Singh,
David Howells, Thomas Cedeno, Anders Roxell, Sami Tolvanen,
Matthew Garrett, Aaron Goidel, Randy Dunlap,
Joel Fernandes (Google), YueHaibing, Christian Brauner,
Alexei Starovoitov, Alexey Budankov, Adrian Reber, Aleksa Sarai,
linux-fsdevel, linux-kernel, linux-security-module, selinux,
kaleshsingh, calin, surenb, nnk, jeffv, kernel-team
Userfaultfd in unprivileged contexts could be potentially very
useful. We'd like to harden userfaultfd to make such unprivileged use
less risky. This patch series allows SELinux to manage userfaultfd
file descriptors and in the future, other kinds of
anonymous-inode-based file descriptor. SELinux policy authors can
apply policy types to anonymous inodes by providing name-based
transition rules keyed off the anonymous inode internal name (
"[userfaultfd]" in the case of userfaultfd(2) file descriptors) and
applying policy to the new SIDs thus produced.
Inside the kernel, a pair of new anon_inodes interface,
anon_inode_getfile_secure and anon_inode_getfd_secure, allow callers
to opt into this SELinux management. In this new "secure" mode,
anon_inodes creates new ephemeral inodes for anonymous file objects
instead of reusing the normal anon_inodes singleton dummy inode. A new
LSM hook gives security modules an opportunity to configure and veto
these ephemeral inodes.
This patch series is one of two fork of [1] and is an
alternative to [2].
The primary difference between the two patch series is that this
partch series creates a unique inode for each "secure" anonymous
inode, while the other patch series ([2]) continues using the
singleton dummy anonymous inode and adds a way to attach SELinux
security information directly to file objects.
I prefer the approach in this patch series because 1) it's a smaller
patch than [2], and 2) it produces a more regular security
architecture: in this patch series, secure anonymous inodes aren't
S_PRIVATE and they maintain the SELinux property that the label for a
file is in its inode. We do need an additional inode per anonymous
file, but per-struct-file inode creation doesn't seem to be a problem
for pipes and sockets.
The previous version of this feature ([1]) created a new SELinux
security class for userfaultfd file descriptors. This version adopts
the generic transition-based approach of [2].
This patch series also differs from [2] in that it doesn't affect all
anonymous inodes right away --- instead requiring anon_inodes callers
to opt in --- but this difference isn't one of basic approach. The
important question to resolve is whether we should be creating new
inodes or enhancing per-file data.
Changes from the first version of the patch:
- Removed some error checks
- Defined a new anon_inode SELinux class to resolve the
ambiguity in [3]
- Inherit sclass as well as descriptor from context inode
Changes from the second version of the patch:
- Fixed example policy in the commit message to reflect the use of
the new anon_inode class.
Changes from the third version of the patch:
- Dropped the fops parameter to the LSM hook
- Documented hook parameters
- Fixed incorrect class used for SELinux transition
- Removed stray UFFD changed early in the series
- Removed a redundant ERR_PTR(PTR_ERR())
Changes from the fourth version of the patch:
- Removed an unused parameter from an internal function
- Fixed function documentation
Changes from the fifth version of the patch:
- Fixed function documentation in fs/anon_inodes.c and
include/linux/lsm_hooks.h
- Used anon_inode_getfd_secure() in userfaultfd() syscall and removed
owner from userfaultfd_ctx.
[1] https://lore.kernel.org/lkml/20200211225547.235083-1-dancol@google.com/
[2] https://lore.kernel.org/linux-fsdevel/20200213194157.5877-1-sds@tycho.nsa.gov/
[3] https://lore.kernel.org/lkml/23f725ca-5b5a-5938-fcc8-5bbbfc9ba9bc@tycho.nsa.gov/
Daniel Colascione (3):
Add a new LSM-supporting anonymous inode interface
Teach SELinux about anonymous inodes
Wire UFFD up to SELinux
fs/anon_inodes.c | 193 ++++++++++++++++++++++------
fs/userfaultfd.c | 23 ++--
include/linux/anon_inodes.h | 13 ++
include/linux/lsm_hook_defs.h | 2 +
include/linux/lsm_hooks.h | 7 +
include/linux/security.h | 3 +
security/security.c | 9 ++
security/selinux/hooks.c | 53 ++++++++
security/selinux/include/classmap.h | 2 +
9 files changed, 255 insertions(+), 50 deletions(-)
--
2.28.0.236.gb10cc79966-goog
^ permalink raw reply
* Re: [apparmor] [PATCH] security: apparmor: delete repeated words in comments
From: Seth Arnold @ 2020-08-07 21:27 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-kernel, apparmor, James Morris, linux-security-module,
Serge E. Hallyn
In-Reply-To: <20200807165055.3756-1-rdunlap@infradead.org>
[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]
On Fri, Aug 07, 2020 at 09:50:55AM -0700, Randy Dunlap wrote:
> Drop repeated words in comments.
> {a, then, to}
>
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Cc: John Johansen <john.johansen@canonical.com>
> Cc: apparmor@lists.ubuntu.com
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
Reviewed-By: Seth Arnold <seth.arnold@canonical.com>
Thanks
> ---
> security/apparmor/include/file.h | 2 +-
> security/apparmor/path.c | 2 +-
> security/apparmor/policy_unpack.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> --- linux-next-20200731.orig/security/apparmor/include/file.h
> +++ linux-next-20200731/security/apparmor/include/file.h
> @@ -167,7 +167,7 @@ int aa_audit_file(struct aa_profile *pro
> * @perms: permission table indexed by the matched state accept entry of @dfa
> * @trans: transition table for indexed by named x transitions
> *
> - * File permission are determined by matching a path against @dfa and then
> + * File permission are determined by matching a path against @dfa and
> * then using the value of the accept entry for the matching state as
> * an index into @perms. If a named exec transition is required it is
> * looked up in the transition table.
> --- linux-next-20200731.orig/security/apparmor/path.c
> +++ linux-next-20200731/security/apparmor/path.c
> @@ -83,7 +83,7 @@ static int disconnect(const struct path
> *
> * Returns: %0 else error code if path lookup fails
> * When no error the path name is returned in @name which points to
> - * to a position in @buf
> + * a position in @buf
> */
> static int d_namespace_path(const struct path *path, char *buf, char **name,
> int flags, const char *disconnected)
> --- linux-next-20200731.orig/security/apparmor/policy_unpack.c
> +++ linux-next-20200731/security/apparmor/policy_unpack.c
> @@ -39,7 +39,7 @@
>
> /*
> * The AppArmor interface treats data as a type byte followed by the
> - * actual data. The interface has the notion of a a named entry
> + * actual data. The interface has the notion of a named entry
> * which has a name (AA_NAME typecode followed by name string) followed by
> * the entries typecode and data. Named types allow for optional
> * elements and extensions to be added and tested for without breaking
>
> --
> AppArmor mailing list
> AppArmor@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [dm-devel] [RFC PATCH v5 00/11] Integrity Policy Enforcement LSM (IPE)
From: Mimi Zohar @ 2020-08-07 18:40 UTC (permalink / raw)
To: James Morris
Cc: James Bottomley, Deven Bowers, Pavel Machek, Sasha Levin, snitzer,
dm-devel, tyhicks, agk, paul, corbet, nramas, serge,
pasha.tatashin, jannh, linux-block, viro, axboe, mdsakib,
linux-kernel, eparis, linux-security-module, linux-audit,
linux-fsdevel, linux-integrity, jaskarankhurana
In-Reply-To: <4a764c86a824a4b931dd7f130ce7afce7df140e4.camel@linux.ibm.com>
On Fri, 2020-08-07 at 13:31 -0400, Mimi Zohar wrote:
> On Sat, 2020-08-08 at 02:41 +1000, James Morris wrote:
> > On Thu, 6 Aug 2020, Mimi Zohar wrote:
> >
> > > On Thu, 2020-08-06 at 09:51 +1000, James Morris wrote:
> > > > On Wed, 5 Aug 2020, Mimi Zohar wrote:
> > > >
> > > > > If block layer integrity was enough, there wouldn't have been a need
> > > > > for fs-verity. Even fs-verity is limited to read only filesystems,
> > > > > which makes validating file integrity so much easier. From the
> > > > > beginning, we've said that fs-verity signatures should be included in
> > > > > the measurement list. (I thought someone signed on to add that support
> > > > > to IMA, but have not yet seen anything.)
> > > > >
> > > > > Going forward I see a lot of what we've accomplished being incorporated
> > > > > into the filesystems. When IMA will be limited to defining a system
> > > > > wide policy, I'll have completed my job.
> > > >
> > > > What are your thoughts on IPE being a standalone LSM? Would you prefer to
> > > > see its functionality integrated into IMA?
> > >
> > > Improving the integrity subsystem would be preferred.
> > >
> >
> > Are you planning to attend Plumbers? Perhaps we could propose a BoF
> > session on this topic.
>
> That sounds like a good idea.
Other than it is already sold out.
Mimi
^ permalink raw reply
* Re: [PATCH] security: selinux: delete repeated words in comments
From: Paul Moore @ 2020-08-07 18:09 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-kernel, Stephen Smalley, Eric Paris, selinux, James Morris,
Serge E. Hallyn, linux-security-module
In-Reply-To: <20200807165134.3913-1-rdunlap@infradead.org>
On Fri, Aug 7, 2020 at 12:51 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> Drop a repeated word in comments.
> {open, is, then}
>
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: selinux@vger.kernel.org
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> ---
> security/selinux/hooks.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
This obviously looks fine, but it will need to wait until after the
merge window closes. I'll send another reply once it is merged.
> --- linux-next-20200731.orig/security/selinux/hooks.c
> +++ linux-next-20200731/security/selinux/hooks.c
> @@ -1978,7 +1978,7 @@ static inline u32 file_to_av(struct file
> }
>
> /*
> - * Convert a file to an access vector and include the correct open
> + * Convert a file to an access vector and include the correct
> * open permission.
> */
> static inline u32 open_file_to_av(struct file *file)
> @@ -4444,7 +4444,7 @@ static int selinux_skb_peerlbl_sid(struc
> *
> * If @skb_sid is valid then the user:role:type information from @sk_sid is
> * combined with the MLS information from @skb_sid in order to create
> - * @conn_sid. If @skb_sid is not valid then then @conn_sid is simply a copy
> + * @conn_sid. If @skb_sid is not valid then @conn_sid is simply a copy
> * of @sk_sid. Returns zero on success, negative values on failure.
> *
> */
> @@ -5314,7 +5314,7 @@ static int selinux_sctp_bind_connect(str
>
> /* As selinux_sctp_bind_connect() is called by the
> * SCTP protocol layer, the socket is already locked,
> - * therefore selinux_netlbl_socket_connect_locked() is
> + * therefore selinux_netlbl_socket_connect_locked()
> * is called here. The situations handled are:
> * sctp_connectx(3), sctp_sendmsg(3), sendmsg(2),
> * whenever a new IP address is added or when a new
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [dm-devel] [RFC PATCH v5 00/11] Integrity Policy Enforcement LSM (IPE)
From: Mimi Zohar @ 2020-08-07 17:31 UTC (permalink / raw)
To: James Morris
Cc: James Bottomley, Deven Bowers, Pavel Machek, Sasha Levin, snitzer,
dm-devel, tyhicks, agk, paul, corbet, nramas, serge,
pasha.tatashin, jannh, linux-block, viro, axboe, mdsakib,
linux-kernel, eparis, linux-security-module, linux-audit,
linux-fsdevel, linux-integrity, jaskarankhurana
In-Reply-To: <alpine.LRH.2.21.2008080240350.13040@namei.org>
On Sat, 2020-08-08 at 02:41 +1000, James Morris wrote:
> On Thu, 6 Aug 2020, Mimi Zohar wrote:
>
> > On Thu, 2020-08-06 at 09:51 +1000, James Morris wrote:
> > > On Wed, 5 Aug 2020, Mimi Zohar wrote:
> > >
> > > > If block layer integrity was enough, there wouldn't have been a need
> > > > for fs-verity. Even fs-verity is limited to read only filesystems,
> > > > which makes validating file integrity so much easier. From the
> > > > beginning, we've said that fs-verity signatures should be included in
> > > > the measurement list. (I thought someone signed on to add that support
> > > > to IMA, but have not yet seen anything.)
> > > >
> > > > Going forward I see a lot of what we've accomplished being incorporated
> > > > into the filesystems. When IMA will be limited to defining a system
> > > > wide policy, I'll have completed my job.
> > >
> > > What are your thoughts on IPE being a standalone LSM? Would you prefer to
> > > see its functionality integrated into IMA?
> >
> > Improving the integrity subsystem would be preferred.
> >
>
> Are you planning to attend Plumbers? Perhaps we could propose a BoF
> session on this topic.
That sounds like a good idea.
Mimi
^ permalink raw reply
* [PATCH] security: selinux: delete repeated words in comments
From: Randy Dunlap @ 2020-08-07 16:51 UTC (permalink / raw)
To: linux-kernel
Cc: Randy Dunlap, Paul Moore, Stephen Smalley, Eric Paris, selinux,
James Morris, Serge E. Hallyn, linux-security-module
Drop a repeated word in comments.
{open, is, then}
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: Eric Paris <eparis@parisplace.org>
Cc: selinux@vger.kernel.org
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
---
security/selinux/hooks.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- linux-next-20200731.orig/security/selinux/hooks.c
+++ linux-next-20200731/security/selinux/hooks.c
@@ -1978,7 +1978,7 @@ static inline u32 file_to_av(struct file
}
/*
- * Convert a file to an access vector and include the correct open
+ * Convert a file to an access vector and include the correct
* open permission.
*/
static inline u32 open_file_to_av(struct file *file)
@@ -4444,7 +4444,7 @@ static int selinux_skb_peerlbl_sid(struc
*
* If @skb_sid is valid then the user:role:type information from @sk_sid is
* combined with the MLS information from @skb_sid in order to create
- * @conn_sid. If @skb_sid is not valid then then @conn_sid is simply a copy
+ * @conn_sid. If @skb_sid is not valid then @conn_sid is simply a copy
* of @sk_sid. Returns zero on success, negative values on failure.
*
*/
@@ -5314,7 +5314,7 @@ static int selinux_sctp_bind_connect(str
/* As selinux_sctp_bind_connect() is called by the
* SCTP protocol layer, the socket is already locked,
- * therefore selinux_netlbl_socket_connect_locked() is
+ * therefore selinux_netlbl_socket_connect_locked()
* is called here. The situations handled are:
* sctp_connectx(3), sctp_sendmsg(3), sendmsg(2),
* whenever a new IP address is added or when a new
^ permalink raw reply
* [PATCH] security: keys: delete repeated words in comments
From: Randy Dunlap @ 2020-08-07 16:51 UTC (permalink / raw)
To: linux-kernel
Cc: Randy Dunlap, David Howells, Jarkko Sakkinen, keyrings,
James Morris, Serge E. Hallyn, linux-security-module
Drop repeated words in comments.
{to, will, the}
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: keyrings@vger.kernel.org
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
---
security/keys/keyctl.c | 2 +-
security/keys/keyring.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
--- linux-next-20200731.orig/security/keys/keyctl.c
+++ linux-next-20200731/security/keys/keyctl.c
@@ -506,7 +506,7 @@ error:
* keyring, otherwise replace the link to the matching key with a link to the
* new key.
*
- * The key must grant the caller Link permission and the the keyring must grant
+ * The key must grant the caller Link permission and the keyring must grant
* the caller Write permission. Furthermore, if an additional link is created,
* the keyring's quota will be extended.
*
--- linux-next-20200731.orig/security/keys/keyring.c
+++ linux-next-20200731/security/keys/keyring.c
@@ -881,7 +881,7 @@ found:
*
* Keys are matched to the type provided and are then filtered by the match
* function, which is given the description to use in any way it sees fit. The
- * match function may use any attributes of a key that it wishes to to
+ * match function may use any attributes of a key that it wishes to
* determine the match. Normally the match function from the key type would be
* used.
*
@@ -1204,7 +1204,7 @@ static int keyring_detect_cycle_iterator
}
/*
- * See if a cycle will will be created by inserting acyclic tree B in acyclic
+ * See if a cycle will be created by inserting acyclic tree B in acyclic
* tree A at the topmost level (ie: as a direct child of A).
*
* Since we are adding B to A at the top level, checking for cycles should just
^ permalink raw reply
* [PATCH] security: ima: delete a repeated word in comments
From: Randy Dunlap @ 2020-08-07 16:51 UTC (permalink / raw)
To: linux-kernel
Cc: Randy Dunlap, Mimi Zohar, Dmitry Kasatkin, linux-integrity,
James Morris, Serge E. Hallyn, linux-security-module
Drop a repeated word in comments.
{the}
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: linux-integrity@vger.kernel.org
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
---
security/integrity/ima/ima_policy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-next-20200731.orig/security/integrity/ima/ima_policy.c
+++ linux-next-20200731/security/integrity/ima/ima_policy.c
@@ -719,7 +719,7 @@ static int __init ima_init_arch_policy(v
* ima_init_policy - initialize the default measure rules.
*
* ima_rules points to either the ima_default_rules or the
- * the new ima_policy_rules.
+ * new ima_policy_rules.
*/
void __init ima_init_policy(void)
{
^ permalink raw reply
* [PATCH] security: apparmor: delete repeated words in comments
From: Randy Dunlap @ 2020-08-07 16:50 UTC (permalink / raw)
To: linux-kernel
Cc: Randy Dunlap, John Johansen, apparmor, James Morris,
Serge E. Hallyn, linux-security-module
Drop repeated words in comments.
{a, then, to}
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: John Johansen <john.johansen@canonical.com>
Cc: apparmor@lists.ubuntu.com
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
---
security/apparmor/include/file.h | 2 +-
security/apparmor/path.c | 2 +-
security/apparmor/policy_unpack.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
--- linux-next-20200731.orig/security/apparmor/include/file.h
+++ linux-next-20200731/security/apparmor/include/file.h
@@ -167,7 +167,7 @@ int aa_audit_file(struct aa_profile *pro
* @perms: permission table indexed by the matched state accept entry of @dfa
* @trans: transition table for indexed by named x transitions
*
- * File permission are determined by matching a path against @dfa and then
+ * File permission are determined by matching a path against @dfa and
* then using the value of the accept entry for the matching state as
* an index into @perms. If a named exec transition is required it is
* looked up in the transition table.
--- linux-next-20200731.orig/security/apparmor/path.c
+++ linux-next-20200731/security/apparmor/path.c
@@ -83,7 +83,7 @@ static int disconnect(const struct path
*
* Returns: %0 else error code if path lookup fails
* When no error the path name is returned in @name which points to
- * to a position in @buf
+ * a position in @buf
*/
static int d_namespace_path(const struct path *path, char *buf, char **name,
int flags, const char *disconnected)
--- linux-next-20200731.orig/security/apparmor/policy_unpack.c
+++ linux-next-20200731/security/apparmor/policy_unpack.c
@@ -39,7 +39,7 @@
/*
* The AppArmor interface treats data as a type byte followed by the
- * actual data. The interface has the notion of a a named entry
+ * actual data. The interface has the notion of a named entry
* which has a name (AA_NAME typecode followed by name string) followed by
* the entries typecode and data. Named types allow for optional
* elements and extensions to be added and tested for without breaking
^ permalink raw reply
* Re: [dm-devel] [RFC PATCH v5 00/11] Integrity Policy Enforcement LSM (IPE)
From: James Morris @ 2020-08-07 16:41 UTC (permalink / raw)
To: Mimi Zohar
Cc: James Bottomley, Deven Bowers, Pavel Machek, Sasha Levin, snitzer,
dm-devel, tyhicks, agk, paul, corbet, nramas, serge,
pasha.tatashin, jannh, linux-block, viro, axboe, mdsakib,
linux-kernel, eparis, linux-security-module, linux-audit,
linux-fsdevel, linux-integrity, jaskarankhurana
In-Reply-To: <eb7a2f5b5cd22cf9231aa0fd8fdb77c729a83428.camel@linux.ibm.com>
On Thu, 6 Aug 2020, Mimi Zohar wrote:
> On Thu, 2020-08-06 at 09:51 +1000, James Morris wrote:
> > On Wed, 5 Aug 2020, Mimi Zohar wrote:
> >
> > > If block layer integrity was enough, there wouldn't have been a need
> > > for fs-verity. Even fs-verity is limited to read only filesystems,
> > > which makes validating file integrity so much easier. From the
> > > beginning, we've said that fs-verity signatures should be included in
> > > the measurement list. (I thought someone signed on to add that support
> > > to IMA, but have not yet seen anything.)
> > >
> > > Going forward I see a lot of what we've accomplished being incorporated
> > > into the filesystems. When IMA will be limited to defining a system
> > > wide policy, I'll have completed my job.
> >
> > What are your thoughts on IPE being a standalone LSM? Would you prefer to
> > see its functionality integrated into IMA?
>
> Improving the integrity subsystem would be preferred.
>
Are you planning to attend Plumbers? Perhaps we could propose a BoF
session on this topic.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* [PATCH 4.19/4.14/4.9/4.4] Smack: fix use-after-free in smk_write_relabel_self()
From: Eric Biggers @ 2020-08-07 16:13 UTC (permalink / raw)
To: stable; +Cc: linux-security-module, syzbot+e6416dabb497a650da40,
Casey Schaufler
From: Eric Biggers <ebiggers@google.com>
commit beb4ee6770a89646659e6a2178538d2b13e2654e upstream.
smk_write_relabel_self() frees memory from the task's credentials with
no locking, which can easily cause a use-after-free because multiple
tasks can share the same credentials structure.
Fix this by using prepare_creds() and commit_creds() to correctly modify
the task's credentials.
Reproducer for "BUG: KASAN: use-after-free in smk_write_relabel_self":
#include <fcntl.h>
#include <pthread.h>
#include <unistd.h>
static void *thrproc(void *arg)
{
int fd = open("/sys/fs/smackfs/relabel-self", O_WRONLY);
for (;;) write(fd, "foo", 3);
}
int main()
{
pthread_t t;
pthread_create(&t, NULL, thrproc, NULL);
thrproc(NULL);
}
Reported-by: syzbot+e6416dabb497a650da40@syzkaller.appspotmail.com
Fixes: 38416e53936e ("Smack: limited capability for changing process label")
Cc: <stable@vger.kernel.org> # v4.4+
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/smack/smackfs.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 371ae368da35..10ee51d04492 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -2746,7 +2746,6 @@ static int smk_open_relabel_self(struct inode *inode, struct file *file)
static ssize_t smk_write_relabel_self(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
- struct task_smack *tsp = current_security();
char *data;
int rc;
LIST_HEAD(list_tmp);
@@ -2771,11 +2770,21 @@ static ssize_t smk_write_relabel_self(struct file *file, const char __user *buf,
kfree(data);
if (!rc || (rc == -EINVAL && list_empty(&list_tmp))) {
+ struct cred *new;
+ struct task_smack *tsp;
+
+ new = prepare_creds();
+ if (!new) {
+ rc = -ENOMEM;
+ goto out;
+ }
+ tsp = new->security;
smk_destroy_label_list(&tsp->smk_relabel);
list_splice(&list_tmp, &tsp->smk_relabel);
+ commit_creds(new);
return count;
}
-
+out:
smk_destroy_label_list(&list_tmp);
return rc;
}
--
2.28.0.236.gb10cc79966-goog
^ permalink raw reply related
* Re: [PATCH v4 12/17] LSM: Add "contents" flag to kernel_read_file hook
From: Mimi Zohar @ 2020-08-07 0:23 UTC (permalink / raw)
To: Kees Cook, Greg Kroah-Hartman
Cc: Scott Branden, Luis Chamberlain, Takashi Iwai, Jessica Yu,
SeongJae Park, KP Singh, linux-efi, linux-security-module,
linux-integrity, selinux, linux-kselftest, linux-kernel
In-Reply-To: <20200729175845.1745471-13-keescook@chromium.org>
On Wed, 2020-07-29 at 10:58 -0700, Kees Cook wrote:
> As with the kernel_load_data LSM hook, add a "contents" flag to the
> kernel_read_file LSM hook that indicates whether the LSM can expect
> a matching call to the kernel_post_read_file LSM hook with the full
> contents of the file. With the coming addition of partial file read
> support for kernel_read_file*() API, the LSM will no longer be able
> to always see the entire contents of a file during the read calls.
>
> For cases where the LSM must read examine the complete file contents,
> it will need to do so on its own every time the kernel_read_file
> hook is called with contents=false (or reject such cases). Adjust all
> existing LSMs to retain existing behavior.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH v4 11/17] module: Call security_kernel_post_load_data()
From: KP Singh @ 2020-08-07 0:22 UTC (permalink / raw)
To: Jessica Yu
Cc: Kees Cook, Greg Kroah-Hartman, Scott Branden, Mimi Zohar,
Luis Chamberlain, Takashi Iwai, SeongJae Park, linux-efi,
Linux Security Module list, linux-integrity, selinux,
linux-kselftest, open list
In-Reply-To: <20200805145342.GA22100@linux-8ccs>
On Wed, Aug 5, 2020 at 4:53 PM Jessica Yu <jeyu@kernel.org> wrote:
>
> +++ Kees Cook [29/07/20 10:58 -0700]:
> >Now that there is an API for checking loaded contents for modules
> >loaded without a file, call into the LSM hooks.
> >
> >Cc: Jessica Yu <jeyu@kernel.org>
> >Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Acked-by: Jessica Yu <jeyu@kernel.org>
Thanks!
Reviewed-by: KP Singh <kpsingh@google.com>
^ permalink raw reply
* Re: [PATCH v4 09/17] LSM: Introduce kernel_post_load_data() hook
From: KP Singh @ 2020-08-07 0:21 UTC (permalink / raw)
To: Kees Cook
Cc: Greg Kroah-Hartman, Scott Branden, Mimi Zohar, Luis Chamberlain,
Takashi Iwai, Jessica Yu, SeongJae Park, linux-efi,
Linux Security Module list, linux-integrity, selinux,
linux-kselftest, open list
In-Reply-To: <20200729175845.1745471-10-keescook@chromium.org>
On Wed, Jul 29, 2020 at 7:59 PM Kees Cook <keescook@chromium.org> wrote:
>
> There are a few places in the kernel where LSMs would like to have
> visibility into the contents of a kernel buffer that has been loaded or
> read. While security_kernel_post_read_file() (which includes the
> buffer) exists as a pairing for security_kernel_read_file(), no such
> hook exists to pair with security_kernel_load_data().
>
> Earlier proposals for just using security_kernel_post_read_file() with a
> NULL file argument were rejected (i.e. "file" should always be valid for
> the security_..._file hooks, but it appears at least one case was
> left in the kernel during earlier refactoring. (This will be fixed in
> a subsequent patch.)
>
> Since not all cases of security_kernel_load_data() can have a single
> contiguous buffer made available to the LSM hook (e.g. kexec image
> segments are separately loaded), there needs to be a way for the LSM to
> reason about its expectations of the hook coverage. In order to handle
> this, add a "contents" argument to the "kernel_load_data" hook that
> indicates if the newly added "kernel_post_load_data" hook will be called
> with the full contents once loaded. That way, LSMs requiring full contents
> can choose to unilaterally reject "kernel_load_data" with contents=false
> (which is effectively the existing hook coverage), but when contents=true
> they can allow it and later evaluate the "kernel_post_load_data" hook
> once the buffer is loaded.
>
> With this change, LSMs can gain coverage over non-file-backed data loads
> (e.g. init_module(2) and firmware userspace helper), which will happen
> in subsequent patches.
>
> Additionally prepare IMA to start processing these cases.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Thanks for adding this! Would be really useful for us.
Reviewed-by: KP Singh <kpsingh@google.com>
> ---
> drivers/base/firmware_loader/fallback.c | 2 +-
[...]
> index 5de45010fb1a..1a5c68196faf 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4019,7 +4019,7 @@ static int selinux_kernel_read_file(struct file *file,
> return rc;
> }
>
> -static int selinux_kernel_load_data(enum kernel_load_data_id id)
> +static int selinux_kernel_load_data(enum kernel_load_data_id id, bool contents)
> {
> int rc = 0;
>
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH v4 10/17] firmware_loader: Use security_post_load_data()
From: Mimi Zohar @ 2020-08-06 22:07 UTC (permalink / raw)
To: Kees Cook, Greg Kroah-Hartman
Cc: Scott Branden, Luis Chamberlain, Takashi Iwai, Jessica Yu,
SeongJae Park, KP Singh, linux-efi, linux-security-module,
linux-integrity, selinux, linux-kselftest, linux-kernel
In-Reply-To: <20200729175845.1745471-11-keescook@chromium.org>
On Wed, 2020-07-29 at 10:58 -0700, Kees Cook wrote:
> Now that security_post_load_data() is wired up, use it instead
> of the NULL file argument style of security_post_read_file(),
> and update the security_kernel_load_data() call to indicate that a
> security_kernel_post_load_data() call is expected.
>
> Wire up the IMA check to match earlier logic. Perhaps a generalized
> change to ima_post_load_data() might look something like this:
>
> return process_buffer_measurement(buf, size,
> kernel_load_data_id_str(load_id),
> read_idmap[load_id] ?: FILE_CHECK,
> 0, NULL);
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Other than one change and one question below, it looks good.
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
<snip>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 85000dc8595c..1a7bc4c7437d 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -706,7 +697,7 @@ int ima_load_data(enum kernel_load_data_id id, bool contents)
> }
> break;
> case LOADING_FIRMWARE:
> - if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE)) {
> + if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE) && !contents) {
> pr_err("Prevent firmware sysfs fallback loading.\n");
Appended signatures are limited to kernel modules and, more recently,
to the kexec kernel image, not firmware. Without a file descriptor,
file signatures stored as an xattrs are not applicable either. We
might as well fail earlier, rather than later. Adding "!contents" is
unnecessary.
> return -EACCES; /* INTEGRITY_UNKNOWN */
> }
> @@ -739,6 +730,15 @@ int ima_load_data(enum kernel_load_data_id id, bool contents)
> */
> int ima_post_load_data(char *buf, loff_t size, enum kernel_load_data_id load_id)
> {
> + if (load_id == LOADING_FIRMWARE) {
> + if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> + (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> + pr_err("Prevent firmware loading_store.\n");
> + return -EACCES; /* INTEGRITY_UNKNOWN */
> + }
> + return 0;
> + }
Even with failing LOADING_FIRMWARE early in ima_load_data(), is this
still needed for fw_sysfs_loading()?
thanks,
Mimi
> +
> return 0;
> }
>
^ permalink raw reply
* Re: [PATCH v4 09/17] LSM: Introduce kernel_post_load_data() hook
From: Mimi Zohar @ 2020-08-06 21:59 UTC (permalink / raw)
To: Kees Cook, Greg Kroah-Hartman
Cc: Scott Branden, Luis Chamberlain, Takashi Iwai, Jessica Yu,
SeongJae Park, KP Singh, linux-efi, linux-security-module,
linux-integrity, selinux, linux-kselftest, linux-kernel
In-Reply-To: <20200729175845.1745471-10-keescook@chromium.org>
On Wed, 2020-07-29 at 10:58 -0700, Kees Cook wrote:
> There are a few places in the kernel where LSMs would like to have
> visibility into the contents of a kernel buffer that has been loaded or
> read. While security_kernel_post_read_file() (which includes the
> buffer) exists as a pairing for security_kernel_read_file(), no such
> hook exists to pair with security_kernel_load_data().
>
> Earlier proposals for just using security_kernel_post_read_file() with a
> NULL file argument were rejected (i.e. "file" should always be valid for
> the security_..._file hooks, but it appears at least one case was
> left in the kernel during earlier refactoring. (This will be fixed in
> a subsequent patch.)
>
> Since not all cases of security_kernel_load_data() can have a single
> contiguous buffer made available to the LSM hook (e.g. kexec image
> segments are separately loaded), there needs to be a way for the LSM to
> reason about its expectations of the hook coverage. In order to handle
> this, add a "contents" argument to the "kernel_load_data" hook that
> indicates if the newly added "kernel_post_load_data" hook will be called
> with the full contents once loaded. That way, LSMs requiring full contents
> can choose to unilaterally reject "kernel_load_data" with contents=false
> (which is effectively the existing hook coverage), but when contents=true
> they can allow it and later evaluate the "kernel_post_load_data" hook
> once the buffer is loaded.
>
> With this change, LSMs can gain coverage over non-file-backed data loads
> (e.g. init_module(2) and firmware userspace helper), which will happen
> in subsequent patches.
>
> Additionally prepare IMA to start processing these cases.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Thanks, Kees. Other than a missing "name" field, it looks good.
The security_kernel_post_load_data hook may be used to verify appended
signatures and to measure the buffer data. Passing the kernel module
(load_info.name) and firmware (fw_name) names is critical at least for
IMA-measurement.
thanks,
Mimi
^ permalink raw reply
* Re: [GIT PULL] Smack patches for v5.9
From: pr-tracker-bot @ 2020-08-06 18:09 UTC (permalink / raw)
To: Casey Schaufler
Cc: Linus Torvalds, Linux Security Module list, LKML, Casey Schaufler
In-Reply-To: <8ce85723-5656-0ee8-67a7-35597d9df0dd@schaufler-ca.com>
The pull request you sent on Tue, 4 Aug 2020 10:49:09 -0700:
> https://github.com/cschaufler/smack-next smack-for-5.9
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/bfdd5aaa54b0a44d9df550fe4c9db7e1470a11b8
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
* Re: [dm-devel] [RFC PATCH v5 00/11] Integrity Policy Enforcement LSM (IPE)
From: Mimi Zohar @ 2020-08-06 14:33 UTC (permalink / raw)
To: James Morris
Cc: James Bottomley, Deven Bowers, Pavel Machek, Sasha Levin, snitzer,
dm-devel, tyhicks, agk, paul, corbet, nramas, serge,
pasha.tatashin, jannh, linux-block, viro, axboe, mdsakib,
linux-kernel, eparis, linux-security-module, linux-audit,
linux-fsdevel, linux-integrity, jaskarankhurana
In-Reply-To: <alpine.LRH.2.21.2008060949410.20084@namei.org>
On Thu, 2020-08-06 at 09:51 +1000, James Morris wrote:
> On Wed, 5 Aug 2020, Mimi Zohar wrote:
>
> > If block layer integrity was enough, there wouldn't have been a need
> > for fs-verity. Even fs-verity is limited to read only filesystems,
> > which makes validating file integrity so much easier. From the
> > beginning, we've said that fs-verity signatures should be included in
> > the measurement list. (I thought someone signed on to add that support
> > to IMA, but have not yet seen anything.)
> >
> > Going forward I see a lot of what we've accomplished being incorporated
> > into the filesystems. When IMA will be limited to defining a system
> > wide policy, I'll have completed my job.
>
> What are your thoughts on IPE being a standalone LSM? Would you prefer to
> see its functionality integrated into IMA?
Improving the integrity subsystem would be preferred.
Mimi
^ permalink raw reply
* Re: [PATCH v36 15/24] x86/sgx: Allow a limited use of ATTRIBUTE.PROVISIONKEY for attestation
From: Darren Kenny @ 2020-08-06 17:00 UTC (permalink / raw)
To: Jarkko Sakkinen, x86, linux-sgx
Cc: linux-kernel, Jarkko Sakkinen, linux-security-module,
Jethro Beekman, Andy Lutomirski, akpm, andriy.shevchenko, asapek,
bp, cedric.xing, chenalexchen, conradparker, cyhanish,
dave.hansen, haitao.huang, josh, kai.huang, kai.svahn, kmoy,
ludloff, nhorman, npmccallum, puiterwijk, rientjes,
sean.j.christopherson, tglx, yaozhangx
In-Reply-To: <20200716135303.276442-16-jarkko.sakkinen@linux.intel.com>
On Thursday, 2020-07-16 at 16:52:54 +03, Jarkko Sakkinen wrote:
> Provisioning Certification Enclave (PCE), the root of trust for other
> enclaves, generates a signing key from a fused key called Provisioning
> Certification Key. PCE can then use this key to certify an attestation key
> of a Quoting Enclave (QE), e.g. we get the chain of trust down to the
> hardware if the Intel signed PCE is used.
>
> To use the needed keys, ATTRIBUTE.PROVISIONKEY is required but should be
> only allowed for those who actually need it so that only the trusted
> parties can certify QE's.
>
> Obviously the attestation service should know the public key of the used
> PCE and that way detect illegit attestation, but whitelisting the legit
> users still adds an additional layer of defence.
>
> Add new device file called /dev/sgx/provision. The sole purpose of this
> file is to provide file descriptors that act as privilege tokens to allow
> to build enclaves with ATTRIBUTE.PROVISIONKEY set. A new ioctl called
> SGX_IOC_ENCLAVE_SET_ATTRIBUTE is used to assign this token to an enclave.
>
> Cc: linux-security-module@vger.kernel.org
> Acked-by: Jethro Beekman <jethro@fortanix.com>
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
> arch/x86/include/uapi/asm/sgx.h | 11 ++++++++
> arch/x86/kernel/cpu/sgx/driver.c | 18 ++++++++++++
> arch/x86/kernel/cpu/sgx/driver.h | 2 ++
> arch/x86/kernel/cpu/sgx/ioctl.c | 47 ++++++++++++++++++++++++++++++++
> 4 files changed, 78 insertions(+)
>
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 5edb08ab8fd0..57d0d30c79b3 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -25,6 +25,8 @@ enum sgx_page_flags {
> _IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages)
> #define SGX_IOC_ENCLAVE_INIT \
> _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> + _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
>
> /**
> * struct sgx_enclave_create - parameter structure for the
> @@ -63,4 +65,13 @@ struct sgx_enclave_init {
> __u64 sigstruct;
> };
>
> +/**
> + * struct sgx_enclave_set_attribute - parameter structure for the
> + * %SGX_IOC_ENCLAVE_SET_ATTRIBUTE ioctl
> + * @attribute_fd: file handle of the attribute file in the securityfs
> + */
> +struct sgx_enclave_set_attribute {
> + __u64 attribute_fd;
> +};
> +
> #endif /* _UAPI_ASM_X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index 5559bc18de41..b9af330a16fa 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -138,6 +138,10 @@ static const struct file_operations sgx_encl_fops = {
> .get_unmapped_area = sgx_get_unmapped_area,
> };
>
> +const struct file_operations sgx_provision_fops = {
> + .owner = THIS_MODULE,
> +};
> +
> static struct miscdevice sgx_dev_enclave = {
> .minor = MISC_DYNAMIC_MINOR,
> .name = "enclave",
> @@ -145,6 +149,13 @@ static struct miscdevice sgx_dev_enclave = {
> .fops = &sgx_encl_fops,
> };
>
> +static struct miscdevice sgx_dev_provision = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "provision",
> + .nodename = "sgx/provision",
> + .fops = &sgx_provision_fops,
> +};
> +
> int __init sgx_drv_init(void)
> {
> unsigned int eax, ebx, ecx, edx;
> @@ -185,5 +196,12 @@ int __init sgx_drv_init(void)
> return ret;
> }
>
> + ret = misc_register(&sgx_dev_provision);
> + if (ret) {
> + pr_err("Creating /dev/sgx/provision failed with %d.\n", ret);
> + misc_deregister(&sgx_dev_enclave);
> + return ret;
> + }
> +
> return 0;
> }
> diff --git a/arch/x86/kernel/cpu/sgx/driver.h b/arch/x86/kernel/cpu/sgx/driver.h
> index e4063923115b..72747d01c046 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.h
> +++ b/arch/x86/kernel/cpu/sgx/driver.h
> @@ -23,6 +23,8 @@ extern u64 sgx_attributes_reserved_mask;
> extern u64 sgx_xfrm_reserved_mask;
> extern u32 sgx_xsave_size_tbl[64];
>
> +extern const struct file_operations sgx_provision_fops;
> +
> long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
>
> int sgx_drv_init(void);
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index 3444de955191..95b0a1e62ea7 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -669,6 +669,50 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> return ret;
> }
>
> +/**
> + * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
> + * @filep: open file to /dev/sgx
> + * @arg: userspace pointer to a struct sgx_enclave_set_attribute instance
> + *
> + * Mark the enclave as being allowed to access a restricted attribute bit.
> + * The requested attribute is specified via the attribute_fd field in the
> + * provided struct sgx_enclave_set_attribute. The attribute_fd must be a
> + * handle to an SGX attribute file, e.g. "/dev/sgx/provision".
> + *
> + * Failure to explicitly request access to a restricted attribute will cause
> + * sgx_ioc_enclave_init() to fail. Currently, the only restricted attribute
> + * is access to the PROVISION_KEY.
> + *
> + * Note, access to the EINITTOKEN_KEY is disallowed entirely.
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static long sgx_ioc_enclave_set_attribute(struct sgx_encl *encl,
> + void __user *arg)
> +{
> + struct sgx_enclave_set_attribute params;
> + struct file *attribute_file;
> + int ret;
> +
> + if (copy_from_user(¶ms, arg, sizeof(params)))
> + return -EFAULT;
> +
> + attribute_file = fget(params.attribute_fd);
> + if (!attribute_file)
> + return -EINVAL;
> +
> + if (attribute_file->f_op != &sgx_provision_fops) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
> + ret = 0;
> +
> +out:
> + fput(attribute_file);
> + return ret;
> +}
>
> long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> {
> @@ -694,6 +738,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> case SGX_IOC_ENCLAVE_INIT:
> ret = sgx_ioc_enclave_init(encl, (void __user *)arg);
> break;
> + case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
> + ret = sgx_ioc_enclave_set_attribute(encl, (void __user *)arg);
> + break;
> default:
> ret = -ENOIOCTLCMD;
> break;
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH v1 0/4] [RFC] Implement Trampoline File Descriptor
From: Madhavan T. Venkataraman @ 2020-08-06 17:26 UTC (permalink / raw)
To: Mark Rutland
Cc: kernel-hardening, linux-api, linux-arm-kernel, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module, oleg, x86
In-Reply-To: <20200804143018.GB7440@C02TD0UTHF1T.local>
Thanks for the lively discussion. I have tried to answer some of the
comments below.
On 8/4/20 9:30 AM, Mark Rutland wrote:
>
>> So, the context is - if security settings in a system disallow a page to have
>> both write and execute permissions, how do you allow the execution of
>> genuine trampolines that are runtime generated and placed in a data
>> page or a stack page?
> There are options today, e.g.
>
> a) If the restriction is only per-alias, you can have distinct aliases
> where one is writable and another is executable, and you can make it
> hard to find the relationship between the two.
>
> b) If the restriction is only temporal, you can write instructions into
> an RW- buffer, transition the buffer to R--, verify the buffer
> contents, then transition it to --X.
>
> c) You can have two processes A and B where A generates instrucitons into
> a buffer that (only) B can execute (where B may be restricted from
> making syscalls like write, mprotect, etc).
The general principle of the mitigation is W^X. I would argue that
the above options are violations of the W^X principle. If they are
allowed today, they must be fixed. And they will be. So, we cannot
rely on them.
a) This requires a remap operation. Two mappings point to the same
physical page. One mapping has W and the other one has X. This
is a violation of W^X.
b) This is again a violation. The kernel should refuse to give execute
permission to a page that was writeable in the past and refuse to
give write permission to a page that was executable in the past.
c) This is just a variation of (a).
In general, the problem with user-level methods to map and execute
dynamic code is that the kernel cannot tell if a genuine application is
using them or an attacker is using them or piggy-backing on them.
If a security subsystem blocks all user-level methods for this reason,
we need a kernel mechanism to deal with the problem.
The kernel mechanism is not to be a backdoor. It is there to define
ways in which safe dynamic code can be executed.
I admit I have to provide more proof that my API and framework can
cover different cases. So, that is what I am doing now. I am in the process
of identifying other examples (per Andy's comment) and attempting to
show that this API and framework can address them. It will take a little time.
>>
>> IIUC, you are suggesting that the user hands the kernel a code fragment
>> and requests it to be placed in an r-x page, correct? However, the
>> kernel cannot trust any code given to it by the user. Nor can it scan any
>> piece of code and reliably decide if it is safe or not.
> Per that same logic the kernel cannot trust trampfd creation calls to be
> legitimate as the adversary could mess with the arguments. It doesn't
> matter if the kernel's codegen is trustworthy if it's potentially driven
> by an adversary.
That is not true. IMO, this is not a deficiency in trampfd. This is
something that is there even for regular system calls. For instance,
the write() system call will faithfully write out a buffer to a file
even if the buffer contents have been hacked by an attacker.
A system call can perform certain checks on incoming arguments.
But it cannot tell if a hacker has modified them.
So, there are two aspects in dynamic code that I am considering -
data and code. I submit that the data part can be hacked if an
application has a vulnerability such as buffer overflow. I don't see
how we can ever help that.
So, I am focused on the code generation part. Not all dynamic code
is the same. They have different degrees of trust.
Off the top of my head, I have tried to identify some examples
where we can have more trust on dynamic code and have the kernel
permit its execution.
1. If the kernel can do the job, then that is one safe way. Here, the kernel
is the code. There is no code generation involved. This is what I
have presented in the patch series as the first cut.
2. If the kernel can generate the code, then that code has a measure
of trust. For trampolines, I agreed to do this for performance.
3. If the code resides in a signed file, then we know that it comes from
an known source and it was generated at build time. So, it is not
hacker generated. So, there is a measure of trust.
This is not just program text. This could also be a buffer that contains
trampoline code that resides in the read-only data section of a binary.
4. If the code resides in a signed file and is emulated (e.g. by QEMU)
and we generate code for dynamic binary translation, we should
be able to do that provided the code generator itself is not suspect.
See the next point.
5. The above are examples of actual machine code or equivalent.
We could also have source code from which we generate machine
code. E.g., JIT code from Java byte code. In this case, if the source
code is in a signed file, we have a measure of trust on the source.
If the kernel uses its own trusted code generator to generate the
object code from the source code, then that object code has a
measure of trust.
Anyway, these are just examples. The principle is - if we can identify
dynamic code that has a certain measure of trust, can the kernel
permit their execution?
All other code that cannot really be trusted by the kernel cannot be
executed safely (unless we find some safe and efficient way to
sandbox such code and limit the effects of the code to within
the sandbox). This is outside the scope of what I am doing.
>> So, the problem of executing dynamic code when security settings are
>> restrictive cannot be solved in userland. The only option I can think of is
>> to have the kernel provide support for dynamic code. It must have one
>> or more safe, trusted code generation components and an API to use
>> the components.
>>
>> My goal is to introduce an API and start off by supporting simple, regular
>> trampolines that are widely used. Then, evolve the feature over a period
>> of time to include other forms of dynamic code such as JIT code.
> I think that you're making a leap to this approach without sufficient
> justification that it actually solves the problem, and I believe that
> there will be ABI issues with this approach which can be sidestepped by
> other potential approaches.
>
> Taking a step back, I think it's necessary to better describe the
> problem and constraints that you believe apply before attempting to
> justify any potential solution.
I totally agree that more justification is needed and I am working on it.
As I have mentioned above, I intend to have the kernel generate code
only if the code generation is simple enough. For more complicated cases,
I plan to use a user-level code generator that is for exclusive kernel use.
I have yet to work out the details on how this would work. Need time.
>
> [...]
>
>>
>> 1. Create a trampoline by calling trampfd_create()
>> 2. Set the register and/or stack contexts for the trampoline.
>> 3. mmap() the trampoline to get an address
>> 4a. Retrieve the register and stack context for the trampoline from the
>> kernel and check if anything has been altered. If yes, abort.
>> 4b. Invoke the trampoline using the address
> As above, you can also do this when using mprotect today, transitioning
> the buffer RWX -> R-- -> R-X. If you're worried about subsequent
> modification via an alias, a sealed memfd would work assuming that can
> be mapped R-X.
This is a violation of W^X and the security subsystem must be fixed
if it permits it.
> This approach is applicable to trampfd, but it isn't a specific benefit
> of trampfd.
>
> [...]
>
>>>> - In the future, if the kernel can be enhanced to use a safe code
>>>> generation component, that code can be placed in the trampoline mapping
>>>> pages. Then, the trampoline invocation does not have to incur a trip
>>>> into the kernel.
>>>>
>>>> - Also, if the kernel can be enhanced to use a safe code generation
>>>> component, other forms of dynamic code such as JIT code can be
>>>> addressed by the trampfd framework.
>>> I don't see why it's necessary for the kernel to generate code at all.
>>> If the trampfd creation requests can be trusted, what prevents trusting
>>> a sealed set of instructions generated in userspace?
>> Let us consider a system in which:
>> - a process is not permitted to have pages with both write and execute
>> - a process is not permitted to map any file as executable unless it
>> is properly signed. In other words, cryptographically verified.
>>
>> Then, the process cannot execute any code that is runtime generated.
>> That includes trampolines. Only trampoline code that is part of program
>> text at build time would be permitted to execute.
>>
>> In this scenario, trampfd requests are coming from signed code. So, they
>> are trusted by the kernel. But trampoline code could be dynamically generated.
>> The kernel will not trust it.
> I think this a very hand-wavy argument, as it suggests that generated
> code is not trusted, but what is effectively a generated bytecode is.
> If certain codegen can be trusted, then we can add mechanisms to permit
> the results of this to be mapped r-x. If that is not possible, then the
> same argument says that trampfd requests cannot be trusted.
There is certainly an extra measure of trust in code that is in
signature verified files as compared to code that is generated
on the fly. At least, we know that the place from which we get
that code is known and the file was generated at build time
and not hacker generated. Such files could still contain a vulnerability.
But because these files are maintained by a known source, chances
are that there is nothing malicious in them.
Thanks.
Madhavan
^ permalink raw reply
* Re: [PATCH v36 11/24] x86/sgx: Add SGX enclave driver
From: Darren Kenny @ 2020-08-06 13:59 UTC (permalink / raw)
To: Jarkko Sakkinen, x86, linux-sgx
Cc: linux-kernel, Jarkko Sakkinen, linux-security-module, linux-mm,
Andrew Morton, Matthew Wilcox, Jethro Beekman, Haitao Huang,
Chunyang Hui, Jordan Hand, Nathaniel McCallum, Seth Moore,
Sean Christopherson, Suresh Siddha, andriy.shevchenko, asapek, bp,
cedric.xing, chenalexchen, conradparker, cyhanish, dave.hansen,
haitao.huang, josh, kai.huang, kai.svahn, kmoy, ludloff, luto,
nhorman, puiterwijk, rientjes, tglx, yaozhangx
In-Reply-To: <20200716135303.276442-12-jarkko.sakkinen@linux.intel.com>
On Thursday, 2020-07-16 at 16:52:50 +03, Jarkko Sakkinen wrote:
> Intel Software Guard eXtensions (SGX) is a set of CPU instructions that can
> be used by applications to set aside private regions of code and data. The
> code outside the SGX hosted software entity is prevented from accessing the
> memory inside the enclave by the CPU. We call these entities enclaves.
>
> Add a driver that provides an ioctl API to construct and run enclaves.
> Enclaves are constructed from pages residing in reserved physical memory
> areas. The contents of these pages can only be accessed when they are
> mapped as part of an enclave, by a hardware thread running inside the
> enclave.
>
> The starting state of an enclave consists of a fixed measured set of
> pages that are copied to the EPC during the construction process by
> using ENCLS leaf functions and Software Enclave Control Structure (SECS)
> that defines the enclave properties.
>
> Enclaves are constructed by using ENCLS leaf functions ECREATE, EADD and
> EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> the EPC and EINIT checks a given signed measurement and moves the enclave
> into a state ready for execution.
>
> An initialized enclave can only be accessed through special Thread Control
> Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER. This leaf
> function converts a thread into enclave mode and continues the execution in
> the offset defined by the TCS provided to EENTER. An enclave is exited
> through syscall, exception, interrupts or by explicitly calling another
> ENCLU leaf EEXIT.
>
> The mmap() permissions are capped by the contained enclave page
> permissions. The mapped areas must also be opaque, i.e. each page address
> must contain a page. This logic is implemented in sgx_encl_may_map().
>
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Acked-by: Jethro Beekman <jethro@fortanix.com>
> Tested-by: Jethro Beekman <jethro@fortanix.com>
> Tested-by: Haitao Huang <haitao.huang@linux.intel.com>
> Tested-by: Chunyang Hui <sanqian.hcy@antfin.com>
> Tested-by: Jordan Hand <jorhand@linux.microsoft.com>
> Tested-by: Nathaniel McCallum <npmccallum@redhat.com>
> Tested-by: Seth Moore <sethmo@google.com>
Tested-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Co-developed-by: Suresh Siddha <suresh.b.siddha@intel.com>
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> arch/x86/kernel/cpu/sgx/Makefile | 2 +
> arch/x86/kernel/cpu/sgx/driver.c | 177 ++++++++++++++++
> arch/x86/kernel/cpu/sgx/driver.h | 29 +++
> arch/x86/kernel/cpu/sgx/encl.c | 333 +++++++++++++++++++++++++++++++
> arch/x86/kernel/cpu/sgx/encl.h | 87 ++++++++
> arch/x86/kernel/cpu/sgx/main.c | 11 +
> 6 files changed, 639 insertions(+)
> create mode 100644 arch/x86/kernel/cpu/sgx/driver.c
> create mode 100644 arch/x86/kernel/cpu/sgx/driver.h
> create mode 100644 arch/x86/kernel/cpu/sgx/encl.c
> create mode 100644 arch/x86/kernel/cpu/sgx/encl.h
>
> diff --git a/arch/x86/kernel/cpu/sgx/Makefile b/arch/x86/kernel/cpu/sgx/Makefile
> index 79510ce01b3b..3fc451120735 100644
> --- a/arch/x86/kernel/cpu/sgx/Makefile
> +++ b/arch/x86/kernel/cpu/sgx/Makefile
> @@ -1,2 +1,4 @@
> obj-y += \
> + driver.o \
> + encl.o \
> main.o
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> new file mode 100644
> index 000000000000..b52520407f5b
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2016-18 Intel Corporation.
> +
> +#include <linux/acpi.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mman.h>
> +#include <linux/security.h>
> +#include <linux/suspend.h>
> +#include <asm/traps.h>
> +#include "driver.h"
> +#include "encl.h"
> +
> +MODULE_DESCRIPTION("Intel SGX Enclave Driver");
> +MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>");
> +MODULE_LICENSE("Dual BSD/GPL");
> +
> +u64 sgx_encl_size_max_32;
> +u64 sgx_encl_size_max_64;
> +u32 sgx_misc_reserved_mask;
> +u64 sgx_attributes_reserved_mask;
> +u64 sgx_xfrm_reserved_mask = ~0x3;
> +u32 sgx_xsave_size_tbl[64];
> +
> +static int sgx_open(struct inode *inode, struct file *file)
> +{
> + struct sgx_encl *encl;
> + int ret;
> +
> + encl = kzalloc(sizeof(*encl), GFP_KERNEL);
> + if (!encl)
> + return -ENOMEM;
> +
> + atomic_set(&encl->flags, 0);
> + kref_init(&encl->refcount);
> + xa_init(&encl->page_array);
> + mutex_init(&encl->lock);
> + INIT_LIST_HEAD(&encl->mm_list);
> + spin_lock_init(&encl->mm_lock);
> +
> + ret = init_srcu_struct(&encl->srcu);
> + if (ret) {
> + kfree(encl);
> + return ret;
> + }
> +
> + file->private_data = encl;
> +
> + return 0;
> +}
> +
> +static int sgx_release(struct inode *inode, struct file *file)
> +{
> + struct sgx_encl *encl = file->private_data;
> + struct sgx_encl_mm *encl_mm;
> +
> + for ( ; ; ) {
> + spin_lock(&encl->mm_lock);
> +
> + if (list_empty(&encl->mm_list)) {
> + encl_mm = NULL;
> + } else {
> + encl_mm = list_first_entry(&encl->mm_list,
> + struct sgx_encl_mm, list);
> + list_del_rcu(&encl_mm->list);
> + }
> +
> + spin_unlock(&encl->mm_lock);
> +
> + /* The list is empty, ready to go. */
> + if (!encl_mm)
> + break;
> +
> + synchronize_srcu(&encl->srcu);
> + mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);
> + kfree(encl_mm);
> + }
> +
> + mutex_lock(&encl->lock);
> + atomic_or(SGX_ENCL_DEAD, &encl->flags);
> + mutex_unlock(&encl->lock);
> +
> + kref_put(&encl->refcount, sgx_encl_release);
> + return 0;
> +}
> +
> +static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> + struct sgx_encl *encl = file->private_data;
> + int ret;
> +
> + ret = sgx_encl_may_map(encl, vma->vm_start, vma->vm_end, vma->vm_flags);
> + if (ret)
> + return ret;
> +
> + ret = sgx_encl_mm_add(encl, vma->vm_mm);
> + if (ret)
> + return ret;
> +
> + vma->vm_ops = &sgx_vm_ops;
> + vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
> + vma->vm_private_data = encl;
> +
> + return 0;
> +}
> +
> +static unsigned long sgx_get_unmapped_area(struct file *file,
> + unsigned long addr,
> + unsigned long len,
> + unsigned long pgoff,
> + unsigned long flags)
> +{
> + if (flags & MAP_PRIVATE)
> + return -EINVAL;
> +
> + if (flags & MAP_FIXED)
> + return addr;
> +
> + return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
> +}
> +
> +static const struct file_operations sgx_encl_fops = {
> + .owner = THIS_MODULE,
> + .open = sgx_open,
> + .release = sgx_release,
> + .mmap = sgx_mmap,
> + .get_unmapped_area = sgx_get_unmapped_area,
> +};
> +
> +static struct miscdevice sgx_dev_enclave = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "enclave",
> + .nodename = "sgx/enclave",
> + .fops = &sgx_encl_fops,
> +};
> +
> +int __init sgx_drv_init(void)
> +{
> + unsigned int eax, ebx, ecx, edx;
> + u64 attr_mask, xfrm_mask;
> + int ret;
> + int i;
> +
> + if (!boot_cpu_has(X86_FEATURE_SGX_LC)) {
> + pr_info("The public key MSRs are not writable.\n");
> + return -ENODEV;
> + }
> +
> + cpuid_count(SGX_CPUID, 0, &eax, &ebx, &ecx, &edx);
> + sgx_misc_reserved_mask = ~ebx | SGX_MISC_RESERVED_MASK;
> + sgx_encl_size_max_64 = 1ULL << ((edx >> 8) & 0xFF);
> + sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
> +
> + cpuid_count(SGX_CPUID, 1, &eax, &ebx, &ecx, &edx);
> +
> + attr_mask = (((u64)ebx) << 32) + (u64)eax;
> + sgx_attributes_reserved_mask = ~attr_mask | SGX_ATTR_RESERVED_MASK;
> +
> + if (boot_cpu_has(X86_FEATURE_OSXSAVE)) {
> + xfrm_mask = (((u64)edx) << 32) + (u64)ecx;
> +
> + for (i = 2; i < 64; i++) {
> + cpuid_count(0x0D, i, &eax, &ebx, &ecx, &edx);
> + if ((1 << i) & xfrm_mask)
> + sgx_xsave_size_tbl[i] = eax + ebx;
> + }
> +
> + sgx_xfrm_reserved_mask = ~xfrm_mask;
> + }
> +
> + ret = misc_register(&sgx_dev_enclave);
> + if (ret) {
> + pr_err("Creating /dev/sgx/enclave failed with %d.\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> diff --git a/arch/x86/kernel/cpu/sgx/driver.h b/arch/x86/kernel/cpu/sgx/driver.h
> new file mode 100644
> index 000000000000..f7ce40dedc91
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/driver.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
> +#ifndef __ARCH_SGX_DRIVER_H__
> +#define __ARCH_SGX_DRIVER_H__
> +
> +#include <crypto/hash.h>
> +#include <linux/kref.h>
> +#include <linux/mmu_notifier.h>
> +#include <linux/radix-tree.h>
> +#include <linux/rwsem.h>
> +#include <linux/sched.h>
> +#include <linux/workqueue.h>
> +#include "sgx.h"
> +
> +#define SGX_EINIT_SPIN_COUNT 20
> +#define SGX_EINIT_SLEEP_COUNT 50
> +#define SGX_EINIT_SLEEP_TIME 20
> +
> +extern u64 sgx_encl_size_max_32;
> +extern u64 sgx_encl_size_max_64;
> +extern u32 sgx_misc_reserved_mask;
> +extern u64 sgx_attributes_reserved_mask;
> +extern u64 sgx_xfrm_reserved_mask;
> +extern u32 sgx_xsave_size_tbl[64];
> +
> +long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
> +
> +int sgx_drv_init(void);
> +
> +#endif /* __ARCH_X86_SGX_DRIVER_H__ */
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> new file mode 100644
> index 000000000000..af5df6bc58f3
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -0,0 +1,333 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2016-18 Intel Corporation.
> +
> +#include <linux/lockdep.h>
> +#include <linux/mm.h>
> +#include <linux/mman.h>
> +#include <linux/shmem_fs.h>
> +#include <linux/suspend.h>
> +#include <linux/sched/mm.h>
> +#include "arch.h"
> +#include "encl.h"
> +#include "encls.h"
> +#include "sgx.h"
> +
> +static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
> + unsigned long addr)
> +{
> + struct sgx_encl_page *entry;
> + unsigned int flags;
> +
> + /* If process was forked, VMA is still there but vm_private_data is set
> + * to NULL.
> + */
> + if (!encl)
> + return ERR_PTR(-EFAULT);
> +
> + flags = atomic_read(&encl->flags);
> +
> + if ((flags & SGX_ENCL_DEAD) || !(flags & SGX_ENCL_INITIALIZED))
> + return ERR_PTR(-EFAULT);
> +
> + entry = xa_load(&encl->page_array, PFN_DOWN(addr));
> + if (!entry)
> + return ERR_PTR(-EFAULT);
> +
> + /* Page is already resident in the EPC. */
> + if (entry->epc_page)
> + return entry;
> +
> + return ERR_PTR(-EFAULT);
> +}
> +
> +static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
> + struct mm_struct *mm)
> +{
> + struct sgx_encl_mm *encl_mm =
> + container_of(mn, struct sgx_encl_mm, mmu_notifier);
> + struct sgx_encl_mm *tmp = NULL;
> +
> + /*
> + * The enclave itself can remove encl_mm. Note, objects can't be moved
> + * off an RCU protected list, but deletion is ok.
> + */
> + spin_lock(&encl_mm->encl->mm_lock);
> + list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) {
> + if (tmp == encl_mm) {
> + list_del_rcu(&encl_mm->list);
> + break;
> + }
> + }
> + spin_unlock(&encl_mm->encl->mm_lock);
> +
> + if (tmp == encl_mm) {
> + synchronize_srcu(&encl_mm->encl->srcu);
> + mmu_notifier_put(mn);
> + }
> +}
> +
> +static void sgx_mmu_notifier_free(struct mmu_notifier *mn)
> +{
> + struct sgx_encl_mm *encl_mm =
> + container_of(mn, struct sgx_encl_mm, mmu_notifier);
> +
> + kfree(encl_mm);
> +}
> +
> +static const struct mmu_notifier_ops sgx_mmu_notifier_ops = {
> + .release = sgx_mmu_notifier_release,
> + .free_notifier = sgx_mmu_notifier_free,
> +};
> +
> +static struct sgx_encl_mm *sgx_encl_find_mm(struct sgx_encl *encl,
> + struct mm_struct *mm)
> +{
> + struct sgx_encl_mm *encl_mm = NULL;
> + struct sgx_encl_mm *tmp;
> + int idx;
> +
> + idx = srcu_read_lock(&encl->srcu);
> +
> + list_for_each_entry_rcu(tmp, &encl->mm_list, list) {
> + if (tmp->mm == mm) {
> + encl_mm = tmp;
> + break;
> + }
> + }
> +
> + srcu_read_unlock(&encl->srcu, idx);
> +
> + return encl_mm;
> +}
> +
> +int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
> +{
> + struct sgx_encl_mm *encl_mm;
> + int ret;
> +
> + /* mm_list can be accessed only by a single thread at a time. */
> + mmap_assert_write_locked(mm);
> +
> + if (atomic_read(&encl->flags) & SGX_ENCL_DEAD)
> + return -EINVAL;
> +
> + /*
> + * mm_structs are kept on mm_list until the mm or the enclave dies,
> + * i.e. once an mm is off the list, it's gone for good, therefore it's
> + * impossible to get a false positive on @mm due to a stale mm_list.
> + */
> + if (sgx_encl_find_mm(encl, mm))
> + return 0;
> +
> + encl_mm = kzalloc(sizeof(*encl_mm), GFP_KERNEL);
> + if (!encl_mm)
> + return -ENOMEM;
> +
> + encl_mm->encl = encl;
> + encl_mm->mm = mm;
> + encl_mm->mmu_notifier.ops = &sgx_mmu_notifier_ops;
> +
> + ret = __mmu_notifier_register(&encl_mm->mmu_notifier, mm);
> + if (ret) {
> + kfree(encl_mm);
> + return ret;
> + }
> +
> + spin_lock(&encl->mm_lock);
> + list_add_rcu(&encl_mm->list, &encl->mm_list);
> + spin_unlock(&encl->mm_lock);
> +
> + return 0;
> +}
> +
> +static void sgx_vma_open(struct vm_area_struct *vma)
> +{
> + struct sgx_encl *encl = vma->vm_private_data;
> +
> + if (!encl)
> + return;
> +
> + if (sgx_encl_mm_add(encl, vma->vm_mm))
> + vma->vm_private_data = NULL;
> +}
> +
> +static unsigned int sgx_vma_fault(struct vm_fault *vmf)
> +{
> + unsigned long addr = (unsigned long)vmf->address;
> + struct vm_area_struct *vma = vmf->vma;
> + struct sgx_encl *encl = vma->vm_private_data;
> + struct sgx_encl_page *entry;
> + int ret = VM_FAULT_NOPAGE;
> + unsigned long pfn;
> +
> + if (!encl)
> + return VM_FAULT_SIGBUS;
> +
> + mutex_lock(&encl->lock);
> +
> + entry = sgx_encl_load_page(encl, addr);
> + if (IS_ERR(entry)) {
> + if (unlikely(PTR_ERR(entry) != -EBUSY))
> + ret = VM_FAULT_SIGBUS;
> +
> + goto out;
> + }
> +
> + if (!follow_pfn(vma, addr, &pfn))
> + goto out;
> +
> + ret = vmf_insert_pfn(vma, addr, PFN_DOWN(entry->epc_page->desc));
> + if (ret != VM_FAULT_NOPAGE) {
> + ret = VM_FAULT_SIGBUS;
> + goto out;
> + }
> +
> +out:
> + mutex_unlock(&encl->lock);
> + return ret;
> +}
> +
> +/**
> + * sgx_encl_may_map() - Check if a requested VMA mapping is allowed
> + * @encl: an enclave
> + * @start: lower bound of the address range, inclusive
> + * @end: upper bound of the address range, exclusive
> + * @vm_prot_bits: requested protections of the address range
> + *
> + * Iterate through the enclave pages contained within [@start, @end) to verify
> + * the permissions requested by @vm_prot_bits do not exceed that of any enclave
> + * page to be mapped.
> + *
> + * Return:
> + * 0 on success,
> + * -EACCES if VMA permissions exceed enclave page permissions
> + */
> +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> + unsigned long end, unsigned long vm_flags)
> +{
> + unsigned long vm_prot_bits = vm_flags & (VM_READ | VM_WRITE | VM_EXEC);
> + unsigned long idx_start = PFN_DOWN(start);
> + unsigned long idx_end = PFN_DOWN(end - 1);
> + struct sgx_encl_page *page;
> + XA_STATE(xas, &encl->page_array, idx_start);
> +
> + /*
> + * Disallow RIE tasks as their VMA permissions might conflict with the
> + * enclave page permissions.
> + */
> + if (!!(current->personality & READ_IMPLIES_EXEC))
> + return -EACCES;
> +
> + xas_for_each(&xas, page, idx_end)
> + if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> + return -EACCES;
> +
> + return 0;
> +}
> +
> +static int sgx_vma_mprotect(struct vm_area_struct *vma,
> + struct vm_area_struct **pprev, unsigned long start,
> + unsigned long end, unsigned long newflags)
> +{
> + int ret;
> +
> + ret = sgx_encl_may_map(vma->vm_private_data, start, end, newflags);
> + if (ret)
> + return ret;
> +
> + return mprotect_fixup(vma, pprev, start, end, newflags);
> +}
> +
> +const struct vm_operations_struct sgx_vm_ops = {
> + .open = sgx_vma_open,
> + .fault = sgx_vma_fault,
> + .mprotect = sgx_vma_mprotect,
> +};
> +
> +/**
> + * sgx_encl_find - find an enclave
> + * @mm: mm struct of the current process
> + * @addr: address in the ELRANGE
> + * @vma: the resulting VMA
> + *
> + * Find an enclave identified by the given address. Give back a VMA that is
> + * part of the enclave and located in that address. The VMA is given back if it
> + * is a proper enclave VMA even if an &sgx_encl instance does not exist yet
> + * (enclave creation has not been performed).
> + *
> + * Return:
> + * 0 on success,
> + * -EINVAL if an enclave was not found,
> + * -ENOENT if the enclave has not been created yet
> + */
> +int sgx_encl_find(struct mm_struct *mm, unsigned long addr,
> + struct vm_area_struct **vma)
> +{
> + struct vm_area_struct *result;
> + struct sgx_encl *encl;
> +
> + result = find_vma(mm, addr);
> + if (!result || result->vm_ops != &sgx_vm_ops || addr < result->vm_start)
> + return -EINVAL;
> +
> + encl = result->vm_private_data;
> + *vma = result;
> +
> + return encl ? 0 : -ENOENT;
> +}
> +
> +/**
> + * sgx_encl_destroy() - destroy enclave resources
> + * @encl: an &sgx_encl instance
> + */
> +void sgx_encl_destroy(struct sgx_encl *encl)
> +{
> + struct sgx_encl_page *entry;
> + unsigned long index;
> +
> + atomic_or(SGX_ENCL_DEAD, &encl->flags);
> +
> + xa_for_each(&encl->page_array, index, entry) {
> + if (entry->epc_page) {
> + sgx_free_epc_page(entry->epc_page);
> + encl->secs_child_cnt--;
> + entry->epc_page = NULL;
> + }
> +
> + kfree(entry);
> + }
> +
> + xa_destroy(&encl->page_array);
> +
> + if (!encl->secs_child_cnt && encl->secs.epc_page) {
> + sgx_free_epc_page(encl->secs.epc_page);
> + encl->secs.epc_page = NULL;
> + }
> +}
> +
> +/**
> + * sgx_encl_release - Destroy an enclave instance
> + * @kref: address of a kref inside &sgx_encl
> + *
> + * Used together with kref_put(). Frees all the resources associated with the
> + * enclave and the instance itself.
> + */
> +void sgx_encl_release(struct kref *ref)
> +{
> + struct sgx_encl *encl = container_of(ref, struct sgx_encl, refcount);
> +
> + sgx_encl_destroy(encl);
> +
> + if (encl->backing)
> + fput(encl->backing);
> +
> + cleanup_srcu_struct(&encl->srcu);
> +
> + WARN_ON_ONCE(!list_empty(&encl->mm_list));
> +
> + /* Detect EPC page leak's. */
> + WARN_ON_ONCE(encl->secs_child_cnt);
> + WARN_ON_ONCE(encl->secs.epc_page);
> +
> + kfree(encl);
> +}
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> new file mode 100644
> index 000000000000..74ad6c4da783
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
> +/**
> + * Copyright(c) 2016-19 Intel Corporation.
> + */
> +#ifndef _X86_ENCL_H
> +#define _X86_ENCL_H
> +
> +#include <linux/cpumask.h>
> +#include <linux/kref.h>
> +#include <linux/list.h>
> +#include <linux/mm_types.h>
> +#include <linux/mmu_notifier.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +#include <linux/srcu.h>
> +#include <linux/workqueue.h>
> +#include <linux/xarray.h>
> +#include "sgx.h"
> +
> +/**
> + * enum sgx_encl_page_desc - defines bits for an enclave page's descriptor
> + * %SGX_ENCL_PAGE_ADDR_MASK: Holds the virtual address of the page.
> + *
> + * The page address for SECS is zero and is used by the subsystem to recognize
> + * the SECS page.
> + */
> +enum sgx_encl_page_desc {
> + /* Bits 11:3 are available when the page is not swapped. */
> + SGX_ENCL_PAGE_ADDR_MASK = PAGE_MASK,
> +};
> +
> +#define SGX_ENCL_PAGE_ADDR(page) \
> + ((page)->desc & SGX_ENCL_PAGE_ADDR_MASK)
> +
> +struct sgx_encl_page {
> + unsigned long desc;
> + unsigned long vm_max_prot_bits;
> + struct sgx_epc_page *epc_page;
> + struct sgx_encl *encl;
> +};
> +
> +enum sgx_encl_flags {
> + SGX_ENCL_CREATED = BIT(0),
> + SGX_ENCL_INITIALIZED = BIT(1),
> + SGX_ENCL_DEBUG = BIT(2),
> + SGX_ENCL_DEAD = BIT(3),
> + SGX_ENCL_IOCTL = BIT(4),
> +};
> +
> +struct sgx_encl_mm {
> + struct sgx_encl *encl;
> + struct mm_struct *mm;
> + struct list_head list;
> + struct mmu_notifier mmu_notifier;
> +};
> +
> +struct sgx_encl {
> + atomic_t flags;
> + u64 secs_attributes;
> + u64 allowed_attributes;
> + unsigned int page_cnt;
> + unsigned int secs_child_cnt;
> + struct mutex lock;
> + struct list_head mm_list;
> + spinlock_t mm_lock;
> + struct file *backing;
> + struct kref refcount;
> + struct srcu_struct srcu;
> + unsigned long base;
> + unsigned long size;
> + unsigned long ssaframesize;
> + struct xarray page_array;
> + struct sgx_encl_page secs;
> + cpumask_t cpumask;
> +};
> +
> +extern const struct vm_operations_struct sgx_vm_ops;
> +
> +int sgx_encl_find(struct mm_struct *mm, unsigned long addr,
> + struct vm_area_struct **vma);
> +void sgx_encl_destroy(struct sgx_encl *encl);
> +void sgx_encl_release(struct kref *ref);
> +int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
> +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> + unsigned long end, unsigned long vm_flags);
> +
> +#endif /* _X86_ENCL_H */
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 97c6895fb6c9..4137254fb29e 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -9,6 +9,8 @@
> #include <linux/sched/mm.h>
> #include <linux/sched/signal.h>
> #include <linux/slab.h>
> +#include "driver.h"
> +#include "encl.h"
> #include "encls.h"
>
> struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
> @@ -260,6 +262,8 @@ static bool __init sgx_page_cache_init(void)
>
> static void __init sgx_init(void)
> {
> + int ret;
> +
> if (!boot_cpu_has(X86_FEATURE_SGX))
> return;
>
> @@ -269,8 +273,15 @@ static void __init sgx_init(void)
> if (!sgx_page_reclaimer_init())
> goto err_page_cache;
>
> + ret = sgx_drv_init();
> + if (ret)
> + goto err_kthread;
> +
> return;
>
> +err_kthread:
> + kthread_stop(ksgxswapd_tsk);
> +
> err_page_cache:
> sgx_page_cache_teardown();
> }
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH 1/2] ima: Pre-parse the list of keyrings in a KEY_CHECK rule
From: Tyler Hicks @ 2020-08-06 15:46 UTC (permalink / raw)
To: Nayna
Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Tushar Sugandhi, Nayna Jain,
linux-kernel, linux-integrity, linux-security-module
In-Reply-To: <8f749594-1214-9f2d-4614-d360772a2ab6@linux.vnet.ibm.com>
On 2020-08-06 11:34:43, Nayna wrote:
>
> On 7/27/20 10:08 AM, Tyler Hicks wrote:
> > The ima_keyrings buffer was used as a work buffer for strsep()-based
> > parsing of the "keyrings=" option of an IMA policy rule. This parsing
> > was re-performed each time an asymmetric key was added to a kernel
> > keyring for each loaded policy rule that contained a "keyrings=" option.
> >
> > An example rule specifying this option is:
> >
> > measure func=KEY_CHECK keyrings=a|b|c
> >
> > The rule says to measure asymmetric keys added to any of the kernel
> > keyrings named "a", "b", or "c". The size of the buffer size was
> > equal to the size of the largest "keyrings=" value seen in a previously
> > loaded rule (5 + 1 for the NUL-terminator in the previous example) and
> > the buffer was pre-allocated at the time of policy load.
> >
> > The pre-allocated buffer approach suffered from a couple bugs:
> >
> > 1) There was no locking around the use of the buffer so concurrent key
> > add operations, to two different keyrings, would result in the
> > strsep() loop of ima_match_keyring() to modify the buffer at the same
> > time. This resulted in unexpected results from ima_match_keyring()
> > and, therefore, could cause unintended keys to be measured or keys to
> > not be measured when IMA policy intended for them to be measured.
> >
> > 2) If the kstrdup() that initialized entry->keyrings in ima_parse_rule()
> > failed, the ima_keyrings buffer was freed and set to NULL even when a
> > valid KEY_CHECK rule was previously loaded. The next KEY_CHECK event
> > would trigger a call to strcpy() with a NULL destination pointer and
> > crash the kernel.
> >
> > Remove the need for a pre-allocated global buffer by parsing the list of
> > keyrings in a KEY_CHECK rule at the time of policy load. The
> > ima_rule_entry will contain an array of string pointers which point to
> > the name of each keyring specified in the rule. No string processing
> > needs to happen at the time of asymmetric key add so iterating through
> > the list and doing a string comparison is all that's required at the
> > time of policy check.
> >
> > In the process of changing how the "keyrings=" policy option is handled,
> > a couple additional bugs were fixed:
> >
> > 1) The rule parser accepted rules containing invalid "keyrings=" values
> > such as "a|b||c", "a|b|", or simply "|".
> >
> > 2) The /sys/kernel/security/ima/policy file did not display the entire
> > "keyrings=" value if the list of keyrings was longer than what could
> > fit in the fixed size tbuf buffer in ima_policy_show().
> >
> > Fixes: 5c7bac9fb2c5 ("IMA: pre-allocate buffer to hold keyrings string")
> > Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > ---
> > security/integrity/ima/ima_policy.c | 138 +++++++++++++++++++---------
> > 1 file changed, 93 insertions(+), 45 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 07f033634b27..c328cfa0fc49 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -59,6 +59,11 @@ enum policy_types { ORIGINAL_TCB = 1, DEFAULT_TCB };
> > enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY };
> > +struct ima_rule_opt_list {
> > + size_t count;
> > + char *items[];
> > +};
> > +
> > struct ima_rule_entry {
> > struct list_head list;
> > int action;
> > @@ -78,7 +83,7 @@ struct ima_rule_entry {
> > int type; /* audit type */
> > } lsm[MAX_LSM_RULES];
> > char *fsname;
> > - char *keyrings; /* Measure keys added to these keyrings */
> > + struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
> > struct ima_template_desc *template;
> > };
> > @@ -206,10 +211,6 @@ static LIST_HEAD(ima_policy_rules);
> > static LIST_HEAD(ima_temp_rules);
> > static struct list_head *ima_rules = &ima_default_rules;
> > -/* Pre-allocated buffer used for matching keyrings. */
> > -static char *ima_keyrings;
> > -static size_t ima_keyrings_len;
> > -
> > static int ima_policy __initdata;
> > static int __init default_measure_policy_setup(char *str)
> > @@ -253,6 +254,72 @@ static int __init default_appraise_policy_setup(char *str)
> > }
> > __setup("ima_appraise_tcb", default_appraise_policy_setup);
> > +static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src)
> > +{
> > + struct ima_rule_opt_list *opt_list;
> > + size_t count = 0;
> > + char *src_copy;
> > + char *cur, *next;
> > + size_t i;
> > +
> > + src_copy = match_strdup(src);
> > + if (!src_copy)
> > + return NULL;
>
> The caller of this function checks for IS_ERR(..) and not
> IS_ERR_OR_NULL(..). Shouldn't it return ERR_PTR(-EINVAL) instead of NULL ?
Yes! Thank you for catching this.
I switched this function to returning an ERR_PTR() towards the end of my
development for this series and missed this particular return.
I'll send out a v2 ASAP.
Tyler
>
> Thanks & Regards,
>
> - Nayna
^ permalink raw reply
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