[PATCH v12 0/4] SELinux support for anonymous inodes and UFFD

[PATCH v12 0/4] SELinux support for anonymous inodes and UFFD

[Lokesh Gidra]

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.

With SELinux managed userfaultfd, an admin can control creation and
movement of the file descriptors. In particular, handling of
a userfaultfd descriptor by a different process is essentially a
ptrace access into the process, without any of the corresponding
security_ptrace_access_check() checks. For privacy, the admin may
want to deny such accesses, which is possible with SELinux support.

Inside the kernel, a new anon_inode interface, anon_inode_getfd_secure,
allows callers to opt into this SELinux management. In this new "secure"
mode, anon_inodes create 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.

Changes from the sixth version of the patch:

  - Removed definition of anon_inode_getfile_secure() as there are no
    callers.
  - Simplified function description of anon_inode_getfd_secure().
  - Elaborated more on the purpose of 'context_inode' in commit message.

Changes from the seventh version of the patch:

  - Fixed error handling in _anon_inode_getfile().
  - Fixed minor comment and indentation related issues.

Changes from the eighth version of the patch:

  - Replaced selinux_state.initialized with selinux_state.initialized

Changes from the ninth version of the patch:

  - Fixed function names in fs/anon_inodes.c
  - Fixed comment of anon_inode_getfd_secure()
  - Fixed name of the patch wherein userfaultfd code uses
    anon_inode_getfd_secure()

Changes from the tenth version of the patch:

  - Split first patch into VFS and LSM specific patches
  - Fixed comments in fs/anon_inodes.c
  - Fixed comment of alloc_anon_inode()

Changes from the eleventh version of the patch:

  - Removed comment of alloc_anon_inode() for consistency with the code
  - Fixed explanation of LSM hook in the commit message

[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):
  fs: add LSM-supporting anon-inode interface
  selinux: teach SELinux about anonymous inodes
  userfaultfd: use secure anon inodes for userfaultfd

Lokesh Gidra (1):
  security: add inode_init_security_anon() LSM hook

 fs/anon_inodes.c                    | 150 ++++++++++++++++++++--------
 fs/libfs.c                          |   5 -
 fs/userfaultfd.c                    |  19 ++--
 include/linux/anon_inodes.h         |   5 +
 include/linux/lsm_hook_defs.h       |   2 +
 include/linux/lsm_hooks.h           |   9 ++
 include/linux/security.h            |  10 ++
 security/security.c                 |   8 ++
 security/selinux/hooks.c            |  53 ++++++++++
 security/selinux/include/classmap.h |   2 +
 10 files changed, 209 insertions(+), 54 deletions(-)

-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH v12 1/4] security: add inode_init_security_anon() LSM hook

[Lokesh Gidra]

This change adds a new LSM hook, inode_init_security_anon(), that will
be used while creating secure anonymous inodes. The hook allows/denies
its creation and assigns a security context to the inode.

The new hook accepts an optional context_inode parameter that callers
can use to provide additional contextual information to security modules
for granting/denying permission to create an anon-inode of the same type.
This context_inode's security_context can also be used to initialize the
newly created anon-inode's security_context.

Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
 include/linux/lsm_hook_defs.h |  2 ++
 include/linux/lsm_hooks.h     |  9 +++++++++
 include/linux/security.h      | 10 ++++++++++
 security/security.c           |  8 ++++++++
 4 files changed, 29 insertions(+)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 32a940117e7a..435a2e22ff95 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 c503f7ab8afb..3af055b7ee1f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -233,6 +233,15 @@
  *	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 the incore security field for the new anonymous inode
+ *      and return whether the inode creation is permitted by the security
+ *      module or not.
+ *      @inode contains the inode structure
+ *      @name name of the anonymous inode class
+ *      @context_inode optional related inode
+ *	Returns 0 on success, -EACCES 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 bc2725491560..7494a93b9ed9 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -323,6 +323,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);
@@ -737,6 +740,13 @@ static inline int security_inode_init_security(struct inode *inode,
 	return 0;
 }
 
+static inline int security_inode_init_security_anon(struct inode *inode,
+						    const struct qstr *name,
+						    const struct inode *context_inode)
+{
+	return 0;
+}
+
 static inline int security_old_inode_init_security(struct inode *inode,
 						   struct inode *dir,
 						   const struct qstr *qstr,
diff --git a/security/security.c b/security/security.c
index a28045dc9e7f..8989ba6af4f6 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1058,6 +1058,14 @@ 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.29.1.341.ge80a0c044ae-goog

[PATCH v12 2/4] fs: add LSM-supporting anon-inode interface

[Lokesh Gidra]

From: Daniel Colascione <dancol@google.com>

This change adds a new function, anon_inode_getfd_secure, that creates
anonymous-node file with individual non-S_PRIVATE inode 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 function accepts an optional context_inode parameter that callers
can use to provide additional contextual information to security modules.
For example, in case of userfaultfd, the created inode is a 'logical child'
of the context_inode (userfaultfd inode of the parent process) in the sense
that it provides the security context required during creation of the child
process' userfaultfd inode.

Signed-off-by: Daniel Colascione <dancol@google.com>

[Delete obsolete comments to alloc_anon_inode()]
[Add context_inode description in comments to anon_inode_getfd_secure()]
[Remove definition of anon_inode_getfile_secure() as there are no callers]
[Make __anon_inode_getfile() static]
[Use correct error cast in __anon_inode_getfile()]
[Fix error handling in __anon_inode_getfile()]

Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
 fs/anon_inodes.c            | 150 ++++++++++++++++++++++++++----------
 fs/libfs.c                  |   5 --
 include/linux/anon_inodes.h |   5 ++
 3 files changed, 115 insertions(+), 45 deletions(-)

diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b..f4b35aaed7b2 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -55,61 +55,79 @@ 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 file *file;
+	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;
+}
 
-	if (IS_ERR(anon_inode_inode))
-		return ERR_PTR(-ENODEV);
+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 (fops->owner && !try_module_get(fops->owner))
 		return ERR_PTR(-ENOENT);
 
-	/*
-	 * 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,
+	if (secure) {
+		inode =	anon_inode_make_secure_inode(name, context_inode);
+		if (IS_ERR(inode)) {
+			file = ERR_CAST(inode);
+			goto err;
+		}
+	} else {
+		inode =	anon_inode_inode;
+		if (IS_ERR(inode)) {
+			file = ERR_PTR(-ENODEV);
+			goto err;
+		}
+		/*
+		 * We know the anon_inode inode count is always
+		 * greater than zero, so ihold() is safe.
+		 */
+		ihold(inode);
+	}
+
+	file = alloc_file_pseudo(inode, anon_inode_mnt, name,
 				 flags & (O_ACCMODE | O_NONBLOCK), fops);
 	if (IS_ERR(file))
-		goto err;
+		goto err_iput;
 
-	file->f_mapping = anon_inode_inode->i_mapping;
+	file->f_mapping = inode->i_mapping;
 
 	file->private_data = priv;
 
 	return file;
 
+err_iput:
+	iput(inode);
 err:
-	iput(anon_inode_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 - 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 +136,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 +162,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,8 +176,48 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
 	put_unused_fd(fd);
 	return error;
 }
+
+/**
+ * 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_getfd() 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);
 
+/**
+ * Like anon_inode_getfd(), but creates a new !S_PRIVATE anon inode rather than
+ * reuse the singleton anon inode, and call the anon_init_security_anon() LSM
+ * hook. This allows the inode to have its own security context and for a LSM
+ * to reject creation of the inode.  An optional @context_inode argument is
+ * also added to provide the logical relationship with the new inode.  The LSM
+ * may use @context_inode in inode_init_security_anon(), but a reference to it
+ * is not held.
+ */
+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);
+
 static int __init anon_inode_init(void)
 {
 	anon_inode_mnt = kern_mount(&anon_inode_fs_type);
diff --git a/fs/libfs.c b/fs/libfs.c
index fc34361c1489..51c19c72e563 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1212,11 +1212,6 @@ static int anon_set_page_dirty(struct page *page)
 	return 0;
 };
 
-/*
- * A single inode exists for all anon_inode files. Contrary to pipes,
- * anon_inode inodes have no associated per-instance data, so we need
- * only allocate one of them.
- */
 struct inode *alloc_anon_inode(struct super_block *s)
 {
 	static const struct address_space_operations anon_aops = {
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261ad..71881a2b6f78 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -10,12 +10,17 @@
 #define _LINUX_ANON_INODES_H
 
 struct file_operations;
+struct inode;
 
 struct file *anon_inode_getfile(const char *name,
 				const struct file_operations *fops,
 				void *priv, int flags);
 int anon_inode_getfd(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);
 
 #endif /* _LINUX_ANON_INODES_H */
 
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Lokesh Gidra]

From: Daniel Colascione <dancol@google.com>

This change uses the anon_inodes and LSM infrastructure introduced in
the previous patches to give SELinux the ability to control
anonymous-inode files that are created using the new
anon_inode_getfd_secure() function.

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>
Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
 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 6b1826fc3658..1c0adcdce7a8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2927,6 +2927,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_initialized(&selinux_state)))
+		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);
@@ -6992,6 +7044,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.29.1.341.ge80a0c044ae-goog

[PATCH v12 4/4] userfaultfd: use secure anon inodes for userfaultfd

[Lokesh Gidra]

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>
Reviewed-by: Eric Biggers <ebiggers@google.com>
---
 fs/userfaultfd.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 000b457ad087..dd78daf06de6 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -972,14 +972,14 @@ 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;
 
@@ -989,7 +989,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);
@@ -1100,7 +1100,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)) {
 			/*
@@ -1160,6 +1160,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;
@@ -1167,7 +1168,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)))
@@ -1985,8 +1986,8 @@ 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.29.1.341.ge80a0c044ae-goog

Re: [PATCH v12 1/4] security: add inode_init_security_anon() LSM hook

[Eric Biggers]

On Fri, Nov 06, 2020 at 07:56:23AM -0800, Lokesh Gidra wrote:
> This change adds a new LSM hook, inode_init_security_anon(), that will
> be used while creating secure anonymous inodes. The hook allows/denies
> its creation and assigns a security context to the inode.
> 
> The new hook accepts an optional context_inode parameter that callers
> can use to provide additional contextual information to security modules
> for granting/denying permission to create an anon-inode of the same type.
> This context_inode's security_context can also be used to initialize the
> newly created anon-inode's security_context.
> 
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>

Reviewed-by: Eric Biggers <ebiggers@google.com>

Re: [PATCH v12 2/4] fs: add LSM-supporting anon-inode interface

[Eric Biggers]

On Fri, Nov 06, 2020 at 07:56:24AM -0800, Lokesh Gidra wrote:
> +/**
> + * Like anon_inode_getfd(), but creates a new !S_PRIVATE anon inode rather than
> + * reuse the singleton anon inode, and call the anon_init_security_anon() LSM
> + * hook. This allows the inode to have its own security context and for a LSM

"call the anon_init_security_anon() LSM hook" =>
"calls the inode_init_security_anon() LSM hook".

Otherwise this patch looks okay.  Feel free to add:

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Paul Moore]

On Fri, Nov 6, 2020 at 10:56 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
>
> From: Daniel Colascione <dancol@google.com>
>
> This change uses the anon_inodes and LSM infrastructure introduced in
> the previous patches to give SELinux the ability to control
> anonymous-inode files that are created using the new
> anon_inode_getfd_secure() function.
>
> 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>
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> ---
>  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 6b1826fc3658..1c0adcdce7a8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2927,6 +2927,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_initialized(&selinux_state)))
> +               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;

I suppose this isn't a major concern given the limited usage at the
moment, but I wonder if it would be a good idea to make sure the
context_inode's SELinux label is valid before we assign it to the
anonymous inode?  If it is invalid, what should we do?  Do we attempt
to (re)validate it?  Do we simply fallback to the transition approach?

> +       } 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,

I believe you want to use ANON_INODE__CREATE here instead of FILE__CREATE, yes?

This brings up another question, and requirement - what testing are
you doing for this patchset?  We require that new SELinux kernel
functionality includes additions to the SELinux test suite to help
verify the functionality.  I'm also *strongly* encouraging that new
contributions come with updates to The SELinux Notebook.  If you are
unsure about what to do for either, let us know and we can help get
you started.

* https://github.com/SELinuxProject/selinux-testsuite
* https://github.com/SELinuxProject/selinux-notebook

> +                           &ad);
> +}
> +
>  static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
>  {
>         return may_create(dir, dentry, SECCLASS_FILE);
> @@ -6992,6 +7044,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 }
>    };
>

-- 
paul moore
www.paul-moore.com

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Lokesh Gidra]

Thanks a lot Paul for the reviewing this patch.

On Mon, Nov 9, 2020 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Nov 6, 2020 at 10:56 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> >
> > From: Daniel Colascione <dancol@google.com>
> >
> > This change uses the anon_inodes and LSM infrastructure introduced in
> > the previous patches to give SELinux the ability to control
> > anonymous-inode files that are created using the new
> > anon_inode_getfd_secure() function.
> >
> > 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>
> > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > ---
> >  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 6b1826fc3658..1c0adcdce7a8 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2927,6 +2927,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_initialized(&selinux_state)))
> > +               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;
>
> I suppose this isn't a major concern given the limited usage at the
> moment, but I wonder if it would be a good idea to make sure the
> context_inode's SELinux label is valid before we assign it to the
> anonymous inode?  If it is invalid, what should we do?  Do we attempt
> to (re)validate it?  Do we simply fallback to the transition approach?
>
Frankly, I'm not too familiar with SELinux. Originally this patch
series was developed by Daniel, in consultation with Stephen Smalley.
In my (probably naive) opinion we should fallback to transition
approach. But I'd request you to tell me if this needs to be addressed
now, and if so then what's the right approach.

If the decision is to address this now, then what's the best way to
check the SELinux label validity?

> > +       } 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,
>
> I believe you want to use ANON_INODE__CREATE here instead of FILE__CREATE, yes?

ANON_INODE__CREATE definitely seems more appropriate. I'll change it
in the next revision.
>
> This brings up another question, and requirement - what testing are
> you doing for this patchset?  We require that new SELinux kernel
> functionality includes additions to the SELinux test suite to help
> verify the functionality.  I'm also *strongly* encouraging that new
> contributions come with updates to The SELinux Notebook.  If you are
> unsure about what to do for either, let us know and we can help get
> you started.
>
> * https://github.com/SELinuxProject/selinux-testsuite
> * https://github.com/SELinuxProject/selinux-notebook
>
I'd definitely need help with both of these. Kindly guide how to proceed.

> > +                           &ad);
> > +}
> > +
> >  static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
> >  {
> >         return may_create(dir, dentry, SECCLASS_FILE);
> > @@ -6992,6 +7044,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 }
> >    };
> >
>
> --
> paul moore
> www.paul-moore.com

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Paul Moore]

On Tue, Nov 10, 2020 at 1:24 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> On Mon, Nov 9, 2020 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Nov 6, 2020 at 10:56 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > >
> > > From: Daniel Colascione <dancol@google.com>
> > >
> > > This change uses the anon_inodes and LSM infrastructure introduced in
> > > the previous patches to give SELinux the ability to control
> > > anonymous-inode files that are created using the new
> > > anon_inode_getfd_secure() function.
> > >
> > > 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>
> > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > ---
> > >  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 6b1826fc3658..1c0adcdce7a8 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2927,6 +2927,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_initialized(&selinux_state)))
> > > +               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;
> >
> > I suppose this isn't a major concern given the limited usage at the
> > moment, but I wonder if it would be a good idea to make sure the
> > context_inode's SELinux label is valid before we assign it to the
> > anonymous inode?  If it is invalid, what should we do?  Do we attempt
> > to (re)validate it?  Do we simply fallback to the transition approach?
>
> Frankly, I'm not too familiar with SELinux. Originally this patch
> series was developed by Daniel, in consultation with Stephen Smalley.
> In my (probably naive) opinion we should fallback to transition
> approach. But I'd request you to tell me if this needs to be addressed
> now, and if so then what's the right approach.
>
> If the decision is to address this now, then what's the best way to
> check the SELinux label validity?

You can check to see if an inode's label is valid by looking at the
isec->initialized field; if it is LABEL_INITIALIZED then it is all
set, if it is any other value then the label isn't entirely correct.
It may have not have ever been fully initialized (and has a default
value) or it may have live on a remote filesystem where the host has
signaled that the label has changed (and the label is now outdated).

This patchset includes support for userfaultfd, which means we don't
really have to worry about the remote fs problem, but the
never-fully-initialized problem could be real in this case.  Normally
we would revalidate an inode in SELinux by calling
__inode_security_revalidate() which requires either a valid dentry or
one that can be found via the inode; does d_find_alias() work on
userfaultfd inodes?

If all else fails, it seems like the safest approach would be to
simply fail the selinux_inode_init_security_anon() call if a
context_inode was supplied and the label wasn't valid.  If we later
decide to change it to falling back to the transition approach we can
do that, we can't go the other way (from transition to error).

> > > +       } 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,
> >
> > I believe you want to use ANON_INODE__CREATE here instead of FILE__CREATE, yes?
>
> ANON_INODE__CREATE definitely seems more appropriate. I'll change it
> in the next revision.
>
> > This brings up another question, and requirement - what testing are
> > you doing for this patchset?  We require that new SELinux kernel
> > functionality includes additions to the SELinux test suite to help
> > verify the functionality.  I'm also *strongly* encouraging that new
> > contributions come with updates to The SELinux Notebook.  If you are
> > unsure about what to do for either, let us know and we can help get
> > you started.
> >
> > * https://github.com/SELinuxProject/selinux-testsuite
> > * https://github.com/SELinuxProject/selinux-notebook
> >
> I'd definitely need help with both of these. Kindly guide how to proceed.

Well, perhaps the best way to start is to explain how you have been
testing this so far and then using that information to draft a test
for the testsuite.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Lokesh Gidra]

On Tue, Nov 10, 2020 at 6:13 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Tue, Nov 10, 2020 at 1:24 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > On Mon, Nov 9, 2020 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Nov 6, 2020 at 10:56 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > >
> > > > From: Daniel Colascione <dancol@google.com>
> > > >
> > > > This change uses the anon_inodes and LSM infrastructure introduced in
> > > > the previous patches to give SELinux the ability to control
> > > > anonymous-inode files that are created using the new
> > > > anon_inode_getfd_secure() function.
> > > >
> > > > 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>
> > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > > ---
> > > >  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 6b1826fc3658..1c0adcdce7a8 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -2927,6 +2927,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_initialized(&selinux_state)))
> > > > +               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;
> > >
> > > I suppose this isn't a major concern given the limited usage at the
> > > moment, but I wonder if it would be a good idea to make sure the
> > > context_inode's SELinux label is valid before we assign it to the
> > > anonymous inode?  If it is invalid, what should we do?  Do we attempt
> > > to (re)validate it?  Do we simply fallback to the transition approach?
> >
> > Frankly, I'm not too familiar with SELinux. Originally this patch
> > series was developed by Daniel, in consultation with Stephen Smalley.
> > In my (probably naive) opinion we should fallback to transition
> > approach. But I'd request you to tell me if this needs to be addressed
> > now, and if so then what's the right approach.
> >
> > If the decision is to address this now, then what's the best way to
> > check the SELinux label validity?
>
> You can check to see if an inode's label is valid by looking at the
> isec->initialized field; if it is LABEL_INITIALIZED then it is all
> set, if it is any other value then the label isn't entirely correct.
> It may have not have ever been fully initialized (and has a default
> value) or it may have live on a remote filesystem where the host has
> signaled that the label has changed (and the label is now outdated).
>
> This patchset includes support for userfaultfd, which means we don't
> really have to worry about the remote fs problem, but the
> never-fully-initialized problem could be real in this case.  Normally
> we would revalidate an inode in SELinux by calling
> __inode_security_revalidate() which requires either a valid dentry or
> one that can be found via the inode; does d_find_alias() work on
> userfaultfd inodes?
>
> If all else fails, it seems like the safest approach would be to
> simply fail the selinux_inode_init_security_anon() call if a
> context_inode was supplied and the label wasn't valid.  If we later
> decide to change it to falling back to the transition approach we can
> do that, we can't go the other way (from transition to error).
>
I'm not sure about d_find_alias() on userfaultfd inodes. But it seems
ok to fail selinux_inode_init_security_anon() to begin with.

> > > > +       } 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,
> > >
> > > I believe you want to use ANON_INODE__CREATE here instead of FILE__CREATE, yes?
> >
> > ANON_INODE__CREATE definitely seems more appropriate. I'll change it
> > in the next revision.
> >
> > > This brings up another question, and requirement - what testing are
> > > you doing for this patchset?  We require that new SELinux kernel
> > > functionality includes additions to the SELinux test suite to help
> > > verify the functionality.  I'm also *strongly* encouraging that new
> > > contributions come with updates to The SELinux Notebook.  If you are
> > > unsure about what to do for either, let us know and we can help get
> > > you started.
> > >
> > > * https://github.com/SELinuxProject/selinux-testsuite
> > > * https://github.com/SELinuxProject/selinux-notebook
> > >
> > I'd definitely need help with both of these. Kindly guide how to proceed.
>
> Well, perhaps the best way to start is to explain how you have been
> testing this so far and then using that information to draft a test
> for the testsuite.
>
As I said in my previous reply, Daniel worked on this patch along with
Stephan Smalley. Here's the conversation regarding testing from back
then:
https://lore.kernel.org/lkml/CAEjxPJ4iquFSBfEj+UEFLUFHPsezuQ-Bzv09n+WgOWk38Nyw3w@mail.gmail.com/

There have been only minor changes (fixing comments/coding-style),
except for addressing a double free issue with userfaultfd_ctx since
last time it was tested as per the link above.

> --
> paul moore
> www.paul-moore.com

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Paul Moore]

On Tue, Nov 10, 2020 at 10:30 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> On Tue, Nov 10, 2020 at 6:13 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Nov 10, 2020 at 1:24 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > On Mon, Nov 9, 2020 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Fri, Nov 6, 2020 at 10:56 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > >
> > > > > From: Daniel Colascione <dancol@google.com>
> > > > >
> > > > > This change uses the anon_inodes and LSM infrastructure introduced in
> > > > > the previous patches to give SELinux the ability to control
> > > > > anonymous-inode files that are created using the new
> > > > > anon_inode_getfd_secure() function.
> > > > >
> > > > > 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>
> > > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > > > ---
> > > > >  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 6b1826fc3658..1c0adcdce7a8 100644
> > > > > --- a/security/selinux/hooks.c
> > > > > +++ b/security/selinux/hooks.c
> > > > > @@ -2927,6 +2927,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_initialized(&selinux_state)))
> > > > > +               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;
> > > >
> > > > I suppose this isn't a major concern given the limited usage at the
> > > > moment, but I wonder if it would be a good idea to make sure the
> > > > context_inode's SELinux label is valid before we assign it to the
> > > > anonymous inode?  If it is invalid, what should we do?  Do we attempt
> > > > to (re)validate it?  Do we simply fallback to the transition approach?
> > >
> > > Frankly, I'm not too familiar with SELinux. Originally this patch
> > > series was developed by Daniel, in consultation with Stephen Smalley.
> > > In my (probably naive) opinion we should fallback to transition
> > > approach. But I'd request you to tell me if this needs to be addressed
> > > now, and if so then what's the right approach.
> > >
> > > If the decision is to address this now, then what's the best way to
> > > check the SELinux label validity?
> >
> > You can check to see if an inode's label is valid by looking at the
> > isec->initialized field; if it is LABEL_INITIALIZED then it is all
> > set, if it is any other value then the label isn't entirely correct.
> > It may have not have ever been fully initialized (and has a default
> > value) or it may have live on a remote filesystem where the host has
> > signaled that the label has changed (and the label is now outdated).
> >
> > This patchset includes support for userfaultfd, which means we don't
> > really have to worry about the remote fs problem, but the
> > never-fully-initialized problem could be real in this case.  Normally
> > we would revalidate an inode in SELinux by calling
> > __inode_security_revalidate() which requires either a valid dentry or
> > one that can be found via the inode; does d_find_alias() work on
> > userfaultfd inodes?
> >
> > If all else fails, it seems like the safest approach would be to
> > simply fail the selinux_inode_init_security_anon() call if a
> > context_inode was supplied and the label wasn't valid.  If we later
> > decide to change it to falling back to the transition approach we can
> > do that, we can't go the other way (from transition to error).
>
> I'm not sure about d_find_alias() on userfaultfd inodes. But it seems
> ok to fail selinux_inode_init_security_anon() to begin with.

I'm okay with simply failing here, but I'm growing a bit concerned
that this patchset hasn't been well tested.  That is a problem.

> > > > This brings up another question, and requirement - what testing are
> > > > you doing for this patchset?  We require that new SELinux kernel
> > > > functionality includes additions to the SELinux test suite to help
> > > > verify the functionality.  I'm also *strongly* encouraging that new
> > > > contributions come with updates to The SELinux Notebook.  If you are
> > > > unsure about what to do for either, let us know and we can help get
> > > > you started.
> > > >
> > > > * https://github.com/SELinuxProject/selinux-testsuite
> > > > * https://github.com/SELinuxProject/selinux-notebook
> > > >
> > > I'd definitely need help with both of these. Kindly guide how to proceed.
> >
> > Well, perhaps the best way to start is to explain how you have been
> > testing this so far and then using that information to draft a test
> > for the testsuite.
>
> As I said in my previous reply, Daniel worked on this patch along with
> Stephan Smalley. Here's the conversation regarding testing from back
> then:
> https://lore.kernel.org/lkml/CAEjxPJ4iquFSBfEj+UEFLUFHPsezuQ-Bzv09n+WgOWk38Nyw3w@mail.gmail.com/
>
> There have been only minor changes (fixing comments/coding-style),
> except for addressing a double free issue with userfaultfd_ctx since
> last time it was tested as per the link above.

I should probably be more clear.  I honestly don't care who originally
wrote the patch, the simple fact is that you are the one who is
posting it *now* for inclusion in the kernel; at the very least I
expect you to be able to demonstrate that you are able to reliably
test this functionality and prove it is working.  While being able to
test this submission initially is important, it is far more important
to have the tests and docs necessary to maintain this functionality
long term.  Perhaps you and/or Google will continue to contribute and
support this functionality long term, but it would be irresponsible of
me to assume that to be true; both people and companies come and go
but code has a tendency to live forever.

Let's start again; how have you been testing this code?

-- 
paul moore
www.paul-moore.com

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Lokesh Gidra]

[-- Attachment #1: Type: text/plain, Size: 9534 bytes --]

On Thu, Nov 12, 2020 at 4:13 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Tue, Nov 10, 2020 at 10:30 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > On Tue, Nov 10, 2020 at 6:13 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Tue, Nov 10, 2020 at 1:24 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > On Mon, Nov 9, 2020 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Fri, Nov 6, 2020 at 10:56 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > > >
> > > > > > From: Daniel Colascione <dancol@google.com>
> > > > > >
> > > > > > This change uses the anon_inodes and LSM infrastructure introduced in
> > > > > > the previous patches to give SELinux the ability to control
> > > > > > anonymous-inode files that are created using the new
> > > > > > anon_inode_getfd_secure() function.
> > > > > >
> > > > > > 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>
> > > > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > > > > ---
> > > > > >  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 6b1826fc3658..1c0adcdce7a8 100644
> > > > > > --- a/security/selinux/hooks.c
> > > > > > +++ b/security/selinux/hooks.c
> > > > > > @@ -2927,6 +2927,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_initialized(&selinux_state)))
> > > > > > +               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;
> > > > >
> > > > > I suppose this isn't a major concern given the limited usage at the
> > > > > moment, but I wonder if it would be a good idea to make sure the
> > > > > context_inode's SELinux label is valid before we assign it to the
> > > > > anonymous inode?  If it is invalid, what should we do?  Do we attempt
> > > > > to (re)validate it?  Do we simply fallback to the transition approach?
> > > >
> > > > Frankly, I'm not too familiar with SELinux. Originally this patch
> > > > series was developed by Daniel, in consultation with Stephen Smalley.
> > > > In my (probably naive) opinion we should fallback to transition
> > > > approach. But I'd request you to tell me if this needs to be addressed
> > > > now, and if so then what's the right approach.
> > > >
> > > > If the decision is to address this now, then what's the best way to
> > > > check the SELinux label validity?
> > >
> > > You can check to see if an inode's label is valid by looking at the
> > > isec->initialized field; if it is LABEL_INITIALIZED then it is all
> > > set, if it is any other value then the label isn't entirely correct.
> > > It may have not have ever been fully initialized (and has a default
> > > value) or it may have live on a remote filesystem where the host has
> > > signaled that the label has changed (and the label is now outdated).
> > >
> > > This patchset includes support for userfaultfd, which means we don't
> > > really have to worry about the remote fs problem, but the
> > > never-fully-initialized problem could be real in this case.  Normally
> > > we would revalidate an inode in SELinux by calling
> > > __inode_security_revalidate() which requires either a valid dentry or
> > > one that can be found via the inode; does d_find_alias() work on
> > > userfaultfd inodes?
> > >
> > > If all else fails, it seems like the safest approach would be to
> > > simply fail the selinux_inode_init_security_anon() call if a
> > > context_inode was supplied and the label wasn't valid.  If we later
> > > decide to change it to falling back to the transition approach we can
> > > do that, we can't go the other way (from transition to error).
> >
> > I'm not sure about d_find_alias() on userfaultfd inodes. But it seems
> > ok to fail selinux_inode_init_security_anon() to begin with.
>
> I'm okay with simply failing here, but I'm growing a bit concerned
> that this patchset hasn't been well tested.  That is a problem.
>
> > > > > This brings up another question, and requirement - what testing are
> > > > > you doing for this patchset?  We require that new SELinux kernel
> > > > > functionality includes additions to the SELinux test suite to help
> > > > > verify the functionality.  I'm also *strongly* encouraging that new
> > > > > contributions come with updates to The SELinux Notebook.  If you are
> > > > > unsure about what to do for either, let us know and we can help get
> > > > > you started.
> > > > >
> > > > > * https://github.com/SELinuxProject/selinux-testsuite
> > > > > * https://github.com/SELinuxProject/selinux-notebook
> > > > >
> > > > I'd definitely need help with both of these. Kindly guide how to proceed.
> > >
> > > Well, perhaps the best way to start is to explain how you have been
> > > testing this so far and then using that information to draft a test
> > > for the testsuite.
> >
> > As I said in my previous reply, Daniel worked on this patch along with
> > Stephan Smalley. Here's the conversation regarding testing from back
> > then:
> > https://lore.kernel.org/lkml/CAEjxPJ4iquFSBfEj+UEFLUFHPsezuQ-Bzv09n+WgOWk38Nyw3w@mail.gmail.com/
> >
> > There have been only minor changes (fixing comments/coding-style),
> > except for addressing a double free issue with userfaultfd_ctx since
> > last time it was tested as per the link above.
>
> I should probably be more clear.  I honestly don't care who originally
> wrote the patch, the simple fact is that you are the one who is
> posting it *now* for inclusion in the kernel; at the very least I
> expect you to be able to demonstrate that you are able to reliably
> test this functionality and prove it is working.  While being able to
> test this submission initially is important, it is far more important
> to have the tests and docs necessary to maintain this functionality
> long term.  Perhaps you and/or Google will continue to contribute and
> support this functionality long term, but it would be irresponsible of
> me to assume that to be true; both people and companies come and go
> but code has a tendency to live forever.
>
> Let's start again; how have you been testing this code?
>
I have created a cuttlefish build and have tested with the attached
userfaultfd program:

1) Without these kernel patches the program executes without any restrictions

vsoc_x86_64:/ $ ./system/bin/userfaultfdSimple
api: 170
features: 511
ioctls: 9223372036854775811

read: Try again


2) With these patches applied but without any policy the 'permission
denied' is thrown

vsoc_x86_64:/ $ ./system/bin/userfaultfdSimple
syscall(userfaultfd): Permission denied

with the following logcat message:
11-18 14:21:44.041  3130  3130 W userfaultfdSimp: type=1400
audit(0.0:107): avc: denied { create } for dev="anon_inodefs"
ino=45031 scontext=u:r:shell:s0 tcontext=u:object_r:shell:s0
tclass=anon_inode permissive=0


3) With the attached .te policy file in place the following output is
observed, confirming that the patch is working as intended.
vsoc_x86_64:/ $ ./vendor/bin/userfaultfdSimple
UFFDIO_API: Permission denied

with the following logcat message:
11-18 14:33:29.142  2028  2028 W userfaultfdSimp: type=1400
audit(0.0:104): avc: denied { ioctl } for
path="anon_inode:[userfaultfd]" dev="anon_inodefs" ino=41169
ioctlcmd=0xaa3f scontext=u:r:userfaultfdSimple:s0
tcontext=u:object_r:uffd_t:s0 tclass=anon_inode permissive=0


> --
> paul moore
> www.paul-moore.com

[-- Attachment #2: userfaultfd_simple.cc --]
[-- Type: text/x-c++src, Size: 951 bytes --]

#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <cstring>

#include <sys/types.h>
#include <sys/ioctl.h>
#include <sys/syscall.h>

#include <linux/userfaultfd.h>

void print_api(const struct uffdio_api *api)
{
	printf("api: %llu\n", api->api);
	printf("features: %llu\n", api->features);
	printf("ioctls: %llu\n", api->ioctls);

	printf("\n");
}

int main(void)
{
	long uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
	if (uffd < 0) {
		perror("syscall(userfaultfd)");
		return -1;
	}

	struct uffdio_api api;
	std::memset(&api, 0x0, sizeof api);
	api.api = UFFD_API;
	if (ioctl(uffd, UFFDIO_API, &api) < 0) {
		perror("UFFDIO_API");
		return -1;
	}

	print_api(&api);

	struct uffd_msg msg;
	std::memset(&msg, 0x0, sizeof msg);
	ssize_t count = read(uffd, &msg, sizeof(msg));
	if (count < 0) {
		perror("read");
		return -1;
	} else if (count == 0) {
		printf("read EOF\n\n");
	}

	printf("read uffd\n\n");

	return 0;
}

[-- Attachment #3: userfaultfdSimple.te --]
[-- Type: application/octet-stream, Size: 778 bytes --]


type userfaultfdSimple, domain;

type userfaultfdSimple_exec, vendor_file_type, exec_type, file_type;

type uffd_t;
type_transition userfaultfdSimple userfaultfdSimple : anon_inode uffd_t "[userfaultfd]";
allow userfaultfdSimple uffd_t:anon_inode { create ioctl read };

# Uncomment one of the allowx lines below to test ioctl whitelisting.
# None
allowxperm userfaultfdSimple uffd_t:anon_inode ioctl 0x0;
# UFFDIO_API
#allowxperm userfaultfdSimple uffd_t:anon_inode ioctl 0xaa3f;

dontaudit userfaultfdSimple adbd:fd use;
dontaudit userfaultfdSimple adbd:unix_stream_socket { read write };
dontaudit userfaultfdSimple devpts:chr_file { getattr ioctl read write };
dontaudit userfaultfdSimple shell:fd use;

domain_auto_trans(shell, userfaultfdSimple_exec, userfaultfdSimple);

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Paul Moore]

On Wed, Nov 18, 2020 at 5:39 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> I have created a cuttlefish build and have tested with the attached
> userfaultfd program:

Thanks, that's a good place to start, a few comments:

- While we support Android as a distribution, it isn't a platform that
we common use for development and testing.  At the moment, Fedora is
probably your best choice for that.

- Your test program should be written in vanilla C for the
selinux-testsuite.  Looking at the userfaultfdSimple.cc code that
should be a trivial conversion.

- I think you have a good start on a test for the selinux-testsuite,
please take a look at the test suite and submit a patch against that
repo.  Ondrej (CC'd) currently maintains the test suite and he may
have some additional thoughts.

* https://github.com/SELinuxProject/selinux-testsuite

> 1) Without these kernel patches the program executes without any restrictions
>
> vsoc_x86_64:/ $ ./system/bin/userfaultfdSimple
> api: 170
> features: 511
> ioctls: 9223372036854775811
>
> read: Try again
>
>
> 2) With these patches applied but without any policy the 'permission
> denied' is thrown
>
> vsoc_x86_64:/ $ ./system/bin/userfaultfdSimple
> syscall(userfaultfd): Permission denied
>
> with the following logcat message:
> 11-18 14:21:44.041  3130  3130 W userfaultfdSimp: type=1400
> audit(0.0:107): avc: denied { create } for dev="anon_inodefs"
> ino=45031 scontext=u:r:shell:s0 tcontext=u:object_r:shell:s0
> tclass=anon_inode permissive=0
>
>
> 3) With the attached .te policy file in place the following output is
> observed, confirming that the patch is working as intended.
> vsoc_x86_64:/ $ ./vendor/bin/userfaultfdSimple
> UFFDIO_API: Permission denied
>
> with the following logcat message:
> 11-18 14:33:29.142  2028  2028 W userfaultfdSimp: type=1400
> audit(0.0:104): avc: denied { ioctl } for
> path="anon_inode:[userfaultfd]" dev="anon_inodefs" ino=41169
> ioctlcmd=0xaa3f scontext=u:r:userfaultfdSimple:s0
> tcontext=u:object_r:uffd_t:s0 tclass=anon_inode permissive=0

-- 
paul moore
www.paul-moore.com

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Lokesh Gidra]

On Sun, Nov 22, 2020 at 3:14 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Nov 18, 2020 at 5:39 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > I have created a cuttlefish build and have tested with the attached
> > userfaultfd program:
>
> Thanks, that's a good place to start, a few comments:
>
> - While we support Android as a distribution, it isn't a platform that
> we common use for development and testing.  At the moment, Fedora is
> probably your best choice for that.
>
I tried setting up a debian/ubuntu system for testing using the
instructions on the selinux-testsuite page, but the system kept
freezing after 'setenforce 1'. I'll try with fedora now.

> - Your test program should be written in vanilla C for the
> selinux-testsuite.  Looking at the userfaultfdSimple.cc code that
> should be a trivial conversion.
>
> - I think you have a good start on a test for the selinux-testsuite,
> please take a look at the test suite and submit a patch against that
> repo.  Ondrej (CC'd) currently maintains the test suite and he may
> have some additional thoughts.
>
> * https://github.com/SELinuxProject/selinux-testsuite

Thanks a lot for the inputs. I'll start working on this.
>
> > 1) Without these kernel patches the program executes without any restrictions
> >
> > vsoc_x86_64:/ $ ./system/bin/userfaultfdSimple
> > api: 170
> > features: 511
> > ioctls: 9223372036854775811
> >
> > read: Try again
> >
> >
> > 2) With these patches applied but without any policy the 'permission
> > denied' is thrown
> >
> > vsoc_x86_64:/ $ ./system/bin/userfaultfdSimple
> > syscall(userfaultfd): Permission denied
> >
> > with the following logcat message:
> > 11-18 14:21:44.041  3130  3130 W userfaultfdSimp: type=1400
> > audit(0.0:107): avc: denied { create } for dev="anon_inodefs"
> > ino=45031 scontext=u:r:shell:s0 tcontext=u:object_r:shell:s0
> > tclass=anon_inode permissive=0
> >
> >
> > 3) With the attached .te policy file in place the following output is
> > observed, confirming that the patch is working as intended.
> > vsoc_x86_64:/ $ ./vendor/bin/userfaultfdSimple
> > UFFDIO_API: Permission denied
> >
> > with the following logcat message:
> > 11-18 14:33:29.142  2028  2028 W userfaultfdSimp: type=1400
> > audit(0.0:104): avc: denied { ioctl } for
> > path="anon_inode:[userfaultfd]" dev="anon_inodefs" ino=41169
> > ioctlcmd=0xaa3f scontext=u:r:userfaultfdSimple:s0
> > tcontext=u:object_r:uffd_t:s0 tclass=anon_inode permissive=0
>
> --
> paul moore
> www.paul-moore.com

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Paul Moore]

On Mon, Nov 23, 2020 at 2:21 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> On Sun, Nov 22, 2020 at 3:14 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Nov 18, 2020 at 5:39 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > I have created a cuttlefish build and have tested with the attached
> > > userfaultfd program:
> >
> > Thanks, that's a good place to start, a few comments:
> >
> > - While we support Android as a distribution, it isn't a platform that
> > we common use for development and testing.  At the moment, Fedora is
> > probably your best choice for that.
> >
> I tried setting up a debian/ubuntu system for testing using the
> instructions on the selinux-testsuite page, but the system kept
> freezing after 'setenforce 1'. I'll try with fedora now.

I would expect you to have much better luck with Fedora.

> > - Your test program should be written in vanilla C for the
> > selinux-testsuite.  Looking at the userfaultfdSimple.cc code that
> > should be a trivial conversion.
> >
> > - I think you have a good start on a test for the selinux-testsuite,
> > please take a look at the test suite and submit a patch against that
> > repo.  Ondrej (CC'd) currently maintains the test suite and he may
> > have some additional thoughts.
> >
> > * https://github.com/SELinuxProject/selinux-testsuite
>
> Thanks a lot for the inputs. I'll start working on this.

Great, let us know if you hit any problems.  I think we would all like
to see this upstream :)

-- 
paul moore
www.paul-moore.com

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Lokesh Gidra]

On Mon, Nov 23, 2020 at 2:43 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Nov 23, 2020 at 2:21 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > On Sun, Nov 22, 2020 at 3:14 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Nov 18, 2020 at 5:39 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > I have created a cuttlefish build and have tested with the attached
> > > > userfaultfd program:
> > >
> > > Thanks, that's a good place to start, a few comments:
> > >
> > > - While we support Android as a distribution, it isn't a platform that
> > > we common use for development and testing.  At the moment, Fedora is
> > > probably your best choice for that.
> > >
> > I tried setting up a debian/ubuntu system for testing using the
> > instructions on the selinux-testsuite page, but the system kept
> > freezing after 'setenforce 1'. I'll try with fedora now.
>
> I would expect you to have much better luck with Fedora.

Yes. It worked!
>
> > > - Your test program should be written in vanilla C for the
> > > selinux-testsuite.  Looking at the userfaultfdSimple.cc code that
> > > should be a trivial conversion.
> > >
> > > - I think you have a good start on a test for the selinux-testsuite,
> > > please take a look at the test suite and submit a patch against that
> > > repo.  Ondrej (CC'd) currently maintains the test suite and he may
> > > have some additional thoughts.
> > >
> > > * https://github.com/SELinuxProject/selinux-testsuite
> >
> > Thanks a lot for the inputs. I'll start working on this.
>
> Great, let us know if you hit any problems.  I think we would all like
> to see this upstream :)
>
I have the patch ready. I couldn't find any instructions on the
testsuite site about patch submission. Can you please tell me how to
proceed.

> --
> paul moore
> www.paul-moore.com

Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes

[Paul Moore]

On Tue, Nov 24, 2020 at 3:44 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> On Mon, Nov 23, 2020 at 2:43 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Nov 23, 2020 at 2:21 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > On Sun, Nov 22, 2020 at 3:14 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Wed, Nov 18, 2020 at 5:39 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > > I have created a cuttlefish build and have tested with the attached
> > > > > userfaultfd program:
> > > >
> > > > Thanks, that's a good place to start, a few comments:
> > > >
> > > > - While we support Android as a distribution, it isn't a platform that
> > > > we common use for development and testing.  At the moment, Fedora is
> > > > probably your best choice for that.
> > > >
> > > I tried setting up a debian/ubuntu system for testing using the
> > > instructions on the selinux-testsuite page, but the system kept
> > > freezing after 'setenforce 1'. I'll try with fedora now.
> >
> > I would expect you to have much better luck with Fedora.
>
> Yes. It worked!

Excellent :)

> > > > - Your test program should be written in vanilla C for the
> > > > selinux-testsuite.  Looking at the userfaultfdSimple.cc code that
> > > > should be a trivial conversion.
> > > >
> > > > - I think you have a good start on a test for the selinux-testsuite,
> > > > please take a look at the test suite and submit a patch against that
> > > > repo.  Ondrej (CC'd) currently maintains the test suite and he may
> > > > have some additional thoughts.
> > > >
> > > > * https://github.com/SELinuxProject/selinux-testsuite
> > >
> > > Thanks a lot for the inputs. I'll start working on this.
> >
> > Great, let us know if you hit any problems.  I think we would all like
> > to see this upstream :)
>
> I have the patch ready. I couldn't find any instructions on the
> testsuite site about patch submission. Can you please tell me how to
> proceed.

You can post it to the SELinux mailing list, much like you would do a
SELinux kernel patch.  I'll take a look and I'll make sure Ondrej
looks at it too.

Thanks!

-- 
paul moore
www.paul-moore.com