[RFC PATCH 0/2] selinux,anon_inodes: Use a separate SELinux class for each type of anon inode

[RFC PATCH 0/2] selinux,anon_inodes: Use a separate SELinux class for each type of anon inode

[Ondrej Mosnacek]

This series aims to correct a design flaw in the original anon_inode
SELinux support that would make it hard to write policies for anonymous
inodes once more types of them are supported (currently only userfaultfd
inodes are). A more detailed rationale is provided in the second patch.

The first patch extends the anon_inode_getfd_secure() function to accept
an additional numeric identifier that represents the type of the
anonymous inode being created, which is passed to the LSMs via
security_inode_init_security_anon().

The second patch then introduces a new SELinux policy capability that
allow policies to opt-in to have a separate class used for each type of
anon inode. That means that the "old way" will still 

I wish I had realized the practical consequences earlier, while the
patches were still under review, but it only started to sink in after
the authors themselves later raised the issue in an off-list
conversation. Even then, I still hoped it wouldn't be that bad, but the
more I thought about how to apply this in an actual policy, the more I
realized how much pain it would be to work with the current design, so
I decided to propose these changes.

I hope this will be an acceptable solution.

A selinux-testsuite patch that adapts the userfaultfd test to work also
with the new policy capability enabled will follow.

Ondrej Mosnacek (2):
  LSM,anon_inodes: explicitly distinguish anon inode types
  selinux: add capability to map anon inode types to separate classes

 fs/anon_inodes.c                           | 42 +++++++++++++---------
 fs/userfaultfd.c                           |  6 ++--
 include/linux/anon_inodes.h                |  4 ++-
 include/linux/lsm_hook_defs.h              |  3 +-
 include/linux/security.h                   | 19 ++++++++++
 security/security.c                        |  3 +-
 security/selinux/hooks.c                   | 28 ++++++++++++++-
 security/selinux/include/classmap.h        |  2 ++
 security/selinux/include/policycap.h       |  1 +
 security/selinux/include/policycap_names.h |  3 +-
 security/selinux/include/security.h        |  7 ++++
 11 files changed, 95 insertions(+), 23 deletions(-)

-- 
2.30.2

[RFC PATCH 1/2] LSM,anon_inodes: explicitly distinguish anon inode types

[Ondrej Mosnacek]

Add an enum to <linux/security.h> that allows LSMs to reliably
distinguish types of anon inodes created via anon_inode_getfd_secure()
and require callers of this function to pass the type as an argument.
For the single current user of this function (userfaultfd), add the
corresponding type and pass it in its anon_inode_getfd_secure() call.

While the "name" argument can be used to distinguish different types as
well, some users of anon_inode_getfd() put some additional information
here (e.g. KVM anon inodes), so using an explicit numeric identifier is
preferred to parsing this information from strings.

The new type information will be used by SELinux in a subsequent patch.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 fs/anon_inodes.c              | 42 ++++++++++++++++++++++-------------
 fs/userfaultfd.c              |  6 +++--
 include/linux/anon_inodes.h   |  4 +++-
 include/linux/lsm_hook_defs.h |  3 ++-
 include/linux/security.h      | 19 ++++++++++++++++
 security/security.c           |  3 ++-
 security/selinux/hooks.c      |  1 +
 7 files changed, 57 insertions(+), 21 deletions(-)

diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index a280156138ed..0c8e77b69893 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -56,6 +56,7 @@ static struct file_system_type anon_inode_fs_type = {
 };
 
 static struct inode *anon_inode_make_secure_inode(
+	enum lsm_anon_inode_type type,
 	const char *name,
 	const struct inode *context_inode)
 {
@@ -67,7 +68,8 @@ static struct inode *anon_inode_make_secure_inode(
 	if (IS_ERR(inode))
 		return inode;
 	inode->i_flags &= ~S_PRIVATE;
-	error =	security_inode_init_security_anon(inode, &qname, context_inode);
+	error =	security_inode_init_security_anon(inode, type, &qname,
+						  context_inode);
 	if (error) {
 		iput(inode);
 		return ERR_PTR(error);
@@ -75,11 +77,11 @@ static struct inode *anon_inode_make_secure_inode(
 	return inode;
 }
 
-static struct file *__anon_inode_getfile(const char *name,
+static struct file *__anon_inode_getfile(enum lsm_anon_inode_type type,
+					 const char *name,
 					 const struct file_operations *fops,
 					 void *priv, int flags,
-					 const struct inode *context_inode,
-					 bool secure)
+					 const struct inode *context_inode)
 {
 	struct inode *inode;
 	struct file *file;
@@ -87,8 +89,8 @@ static struct file *__anon_inode_getfile(const char *name,
 	if (fops->owner && !try_module_get(fops->owner))
 		return ERR_PTR(-ENOENT);
 
-	if (secure) {
-		inode =	anon_inode_make_secure_inode(name, context_inode);
+	if (type != LSM_ANON_INODE_NONE) {
+		inode =	anon_inode_make_secure_inode(type, name, context_inode);
 		if (IS_ERR(inode)) {
 			file = ERR_CAST(inode);
 			goto err;
@@ -144,15 +146,16 @@ 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);
+	return __anon_inode_getfile(LSM_ANON_INODE_NONE, name, fops, priv,
+				    flags, NULL);
 }
 EXPORT_SYMBOL_GPL(anon_inode_getfile);
 
-static int __anon_inode_getfd(const char *name,
+static int __anon_inode_getfd(enum lsm_anon_inode_type type,
+			      const char *name,
 			      const struct file_operations *fops,
 			      void *priv, int flags,
-			      const struct inode *context_inode,
-			      bool secure)
+			      const struct inode *context_inode)
 {
 	int error, fd;
 	struct file *file;
@@ -162,8 +165,8 @@ static int __anon_inode_getfd(const char *name,
 		return error;
 	fd = error;
 
-	file = __anon_inode_getfile(name, fops, priv, flags, context_inode,
-				    secure);
+	file = __anon_inode_getfile(type, name, fops, priv, flags,
+				    context_inode);
 	if (IS_ERR(file)) {
 		error = PTR_ERR(file);
 		goto err_put_unused_fd;
@@ -197,7 +200,8 @@ err_put_unused_fd:
 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);
+	return __anon_inode_getfd(LSM_ANON_INODE_NONE, name, fops, priv,
+				  flags, NULL);
 }
 EXPORT_SYMBOL_GPL(anon_inode_getfd);
 
@@ -207,7 +211,9 @@ EXPORT_SYMBOL_GPL(anon_inode_getfd);
  * the inode_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.
  *
- * @name:    [in]    name of the "class" of the new file
+ * @type:    [in]    type of the file recognizable by LSMs
+ * @name:    [in]    name of the "class" of the new file (may be more specific
+ *                   than @type)
  * @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
@@ -217,11 +223,15 @@ EXPORT_SYMBOL_GPL(anon_inode_getfd);
  * 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,
+int anon_inode_getfd_secure(enum lsm_anon_inode_type type, 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);
+	/* The caller must pass a valid type! */
+	if (WARN_ON(type <= LSM_ANON_INODE_NONE || type > LSM_ANON_INODE_MAX))
+		return -EINVAL;
+	return __anon_inode_getfd(type, name, fops, priv, flags, context_inode);
 }
 EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);
 
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 0be8cdd4425a..003f65d752c4 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -985,7 +985,8 @@ static int resolve_userfault_fork(struct userfaultfd_ctx *new,
 {
 	int fd;
 
-	fd = anon_inode_getfd_secure("[userfaultfd]", &userfaultfd_fops, new,
+	fd = anon_inode_getfd_secure(LSM_ANON_INODE_USERFAULTFD,
+			"[userfaultfd]", &userfaultfd_fops, new,
 			O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS), inode);
 	if (fd < 0)
 		return fd;
@@ -2000,7 +2001,8 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
 	/* prevent the mm struct to be freed */
 	mmgrab(ctx->mm);
 
-	fd = anon_inode_getfd_secure("[userfaultfd]", &userfaultfd_fops, ctx,
+	fd = anon_inode_getfd_secure(LSM_ANON_INODE_USERFAULTFD,
+			"[userfaultfd]", &userfaultfd_fops, ctx,
 			O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS), NULL);
 	if (fd < 0) {
 		mmdrop(ctx->mm);
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index 71881a2b6f78..37137e994ceb 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -9,6 +9,8 @@
 #ifndef _LINUX_ANON_INODES_H
 #define _LINUX_ANON_INODES_H
 
+#include <linux/security.h>
+
 struct file_operations;
 struct inode;
 
@@ -17,7 +19,7 @@ struct file *anon_inode_getfile(const char *name,
 				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,
+int anon_inode_getfd_secure(enum lsm_anon_inode_type type, const char *name,
 			    const struct file_operations *fops,
 			    void *priv, int flags,
 			    const struct inode *context_inode);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 61f04f7dc1a4..ba03a7d0bf1a 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -115,7 +115,8 @@ 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)
+	 enum lsm_anon_inode_type type, 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/security.h b/include/linux/security.h
index 9aeda3f9e838..7c5117676f29 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -79,6 +79,23 @@ enum lsm_event {
 	LSM_POLICY_CHANGE,
 };
 
+/*
+ * Types of anonymous inodes that may be interesting to LSMs.
+ * Passed to anon_inode_getfd_secure() and
+ * security_inode_init_security_anon().
+ */
+enum lsm_anon_inode_type {
+	/* anon inodes invisible to the LSMs */
+	LSM_ANON_INODE_NONE = 0,
+	/* userfaultfd anon inodes */
+	LSM_ANON_INODE_USERFAULTFD,
+	/* (add new types above this line) */
+
+	__LSM_ANON_INODE_MAX,
+	/* max value used for asserts */
+	LSM_ANON_INODE_MAX = __LSM_ANON_INODE_MAX - 1,
+};
+
 /*
  * These are reasons that can be passed to the security_locked_down()
  * LSM hook. Lockdown reasons that protect kernel integrity (ie, the
@@ -329,6 +346,7 @@ 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,
+				      enum lsm_anon_inode_type type,
 				      const struct qstr *name,
 				      const struct inode *context_inode);
 int security_old_inode_init_security(struct inode *inode, struct inode *dir,
@@ -759,6 +777,7 @@ static inline int security_inode_init_security(struct inode *inode,
 }
 
 static inline int security_inode_init_security_anon(struct inode *inode,
+						    enum lsm_anon_inode_type type,
 						    const struct qstr *name,
 						    const struct inode *context_inode)
 {
diff --git a/security/security.c b/security/security.c
index 94383f83ba42..3786932c576c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1067,10 +1067,11 @@ out:
 EXPORT_SYMBOL(security_inode_init_security);
 
 int security_inode_init_security_anon(struct inode *inode,
+				      enum lsm_anon_inode_type type,
 				      const struct qstr *name,
 				      const struct inode *context_inode)
 {
-	return call_int_hook(inode_init_security_anon, 0, inode, name,
+	return call_int_hook(inode_init_security_anon, 0, inode, type, name,
 			     context_inode);
 }
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 92f909a2e8f7..dc57ba21d8ff 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3048,6 +3048,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 }
 
 static int selinux_inode_init_security_anon(struct inode *inode,
+					    enum lsm_anon_inode_type type,
 					    const struct qstr *name,
 					    const struct inode *context_inode)
 {
-- 
2.30.2

[RFC PATCH 2/2] selinux: add capability to map anon inode types to separate classes

[Ondrej Mosnacek]

Unfortunately, the approach chosen in commit 29cd6591ab6f ("selinux:
teach SELinux about anonymous inodes") to use a single class for all
anon inodes and let the policy distinguish between them using named
transitions turned out to have a rather unfortunate drawback.

For example, suppose we have two types of anon inodes, "A" and "B", and
we want to allow a set of domains (represented by an attribute "attr_x")
certain set of permissions on anon inodes of type "A" that were created
by the same domain, but at the same time disallow this set to access
anon inodes of type "B" entirely. Since all inodes share the same class
and we want to distinguish both the inode types and the domains that
created them, we have no choice than to create separate types for the
cartesian product of (domains that belong to attr_x) x ("A", "B") and
add all the necessary allow and transition rules for each domain
individually.

This makes it very impractical to write sane policies for anon inodes in
the future, as more anon inode types are added. Therefore, this patch
implements an alternative approach that assigns a separate class to each
type of anon inode. This allows the example above to be implemented
without any transition rules and with just a single allow rule:

allow attr_x self:A { ... };

In order to not break possible existing users of the already merged
original approach, this patch also adds a new policy capability
"extended_anon_inode_class" that needs to be set by the policy to enable
the new behavior.

I decided to keep the named transition mechanism in the new variant,
since there might eventually be some extra information in the anon inode
name that could be used in transitions.

One minor annoyance is that the kernel still expects the policy to
provide both classes (anon_inode and userfaultfd) regardless of the
capability setting and if one of them is not defined in the policy, the
kernel will print a warning when loading the policy. However, it doesn't
seem worth to work around that in the kernel, as the policy can provide
just the definition of the unused class(es) (and permissions) to avoid
this warning. Keeping the legacy anon_inode class with some fallback
rules may also be desirable to keep the policy compatible with kernels
that only support anon_inode.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/hooks.c                   | 27 +++++++++++++++++++++-
 security/selinux/include/classmap.h        |  2 ++
 security/selinux/include/policycap.h       |  1 +
 security/selinux/include/policycap_names.h |  3 ++-
 security/selinux/include/security.h        |  7 ++++++
 5 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index dc57ba21d8ff..20a8d7d17936 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3079,7 +3079,32 @@ static int selinux_inode_init_security_anon(struct inode *inode,
 		isec->sclass = context_isec->sclass;
 		isec->sid = context_isec->sid;
 	} else {
-		isec->sclass = SECCLASS_ANON_INODE;
+		/*
+		 * If the check below fails:
+		 *  1. Add the corresponding security class to
+		 *     security/selinux/include/classmap.h
+		 *  2. Map the new LSM_ANON_INODE_* value to the class in
+		 *     the switch statement below.
+		 *  3. Update the RHS of the comparison in the BUILD_BUG_ON().
+		 *  4. CC selinux@vger.kernel.org and
+		 *     linux-security-module@vger.kernel.org when submitting
+		 *     the patch or in case of any questions.
+		 */
+		BUILD_BUG_ON(LSM_ANON_INODE_MAX > LSM_ANON_INODE_USERFAULTFD);
+
+		if (selinux_policycap_extended_anon_inode()) {
+			switch (type) {
+			case LSM_ANON_INODE_USERFAULTFD:
+				isec->sclass = SECCLASS_USERFAULTFD;
+				break;
+			default:
+				pr_err("SELinux:  got invalid anon inode type: %d",
+				       (int)type);
+				return -EINVAL;
+			}
+		} else {
+			isec->sclass = SECCLASS_ANON_INODE;
+		}
 		rc = security_transition_sid(
 			&selinux_state, tsec->sid, tsec->sid,
 			isec->sclass, name, &isec->sid);
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index ba2e01a6955c..e4308cad6407 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -251,6 +251,8 @@ struct security_class_mapping secclass_map[] = {
 	  { "integrity", "confidentiality", NULL } },
 	{ "anon_inode",
 	  { COMMON_FILE_PERMS, NULL } },
+	{ "userfaultfd",
+	  { COMMON_FILE_PERMS, NULL } },
 	{ NULL }
   };
 
diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h
index 2ec038efbb03..969804bd6dab 100644
--- a/security/selinux/include/policycap.h
+++ b/security/selinux/include/policycap.h
@@ -11,6 +11,7 @@ enum {
 	POLICYDB_CAPABILITY_CGROUPSECLABEL,
 	POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
 	POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS,
+	POLICYDB_CAPABILITY_EXTENDED_ANON_INODE_CLASS,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h
index b89289f092c9..78651990425e 100644
--- a/security/selinux/include/policycap_names.h
+++ b/security/selinux/include/policycap_names.h
@@ -12,7 +12,8 @@ const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
 	"always_check_network",
 	"cgroup_seclabel",
 	"nnp_nosuid_transition",
-	"genfs_seclabel_symlinks"
+	"genfs_seclabel_symlinks",
+	"extended_anon_inode_class",
 };
 
 #endif /* _SELINUX_POLICYCAP_NAMES_H_ */
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 7130c9648ad1..4fb75101aca4 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -219,6 +219,13 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
 	return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]);
 }
 
+static inline bool selinux_policycap_extended_anon_inode(void)
+{
+	struct selinux_state *state = &selinux_state;
+
+	return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_EXTENDED_ANON_INODE_CLASS]);
+}
+
 int security_mls_enabled(struct selinux_state *state);
 int security_load_policy(struct selinux_state *state,
 			void *data, size_t len,
-- 
2.30.2

Re: [RFC PATCH 0/2] selinux,anon_inodes: Use a separate SELinux class for each type of anon inode

[Paul Moore]

On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> This series aims to correct a design flaw in the original anon_inode
> SELinux support that would make it hard to write policies for anonymous
> inodes once more types of them are supported (currently only userfaultfd
> inodes are). A more detailed rationale is provided in the second patch.
>
> The first patch extends the anon_inode_getfd_secure() function to accept
> an additional numeric identifier that represents the type of the
> anonymous inode being created, which is passed to the LSMs via
> security_inode_init_security_anon().
>
> The second patch then introduces a new SELinux policy capability that
> allow policies to opt-in to have a separate class used for each type of
> anon inode. That means that the "old way" will still

... will what? :)

I think it would be a very good idea if you could provide some
concrete examples of actual policy problems encountered using the
current approach.  I haven't looked at these patches very seriously
yet, but my initial reaction is not "oh yes, we definitely need this".

> I wish I had realized the practical consequences earlier, while the
> patches were still under review, but it only started to sink in after
> the authors themselves later raised the issue in an off-list
> conversation. Even then, I still hoped it wouldn't be that bad, but the
> more I thought about how to apply this in an actual policy, the more I
> realized how much pain it would be to work with the current design, so
> I decided to propose these changes.
>
> I hope this will be an acceptable solution.
>
> A selinux-testsuite patch that adapts the userfaultfd test to work also
> with the new policy capability enabled will follow.
>
> Ondrej Mosnacek (2):
>   LSM,anon_inodes: explicitly distinguish anon inode types
>   selinux: add capability to map anon inode types to separate classes
>
>  fs/anon_inodes.c                           | 42 +++++++++++++---------
>  fs/userfaultfd.c                           |  6 ++--
>  include/linux/anon_inodes.h                |  4 ++-
>  include/linux/lsm_hook_defs.h              |  3 +-
>  include/linux/security.h                   | 19 ++++++++++
>  security/security.c                        |  3 +-
>  security/selinux/hooks.c                   | 28 ++++++++++++++-
>  security/selinux/include/classmap.h        |  2 ++
>  security/selinux/include/policycap.h       |  1 +
>  security/selinux/include/policycap_names.h |  3 +-
>  security/selinux/include/security.h        |  7 ++++
>  11 files changed, 95 insertions(+), 23 deletions(-)
>
> --
> 2.30.2

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 0/2] selinux,anon_inodes: Use a separate SELinux class for each type of anon inode

[Ondrej Mosnacek]

On Wed, Apr 21, 2021 at 10:38 PM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > This series aims to correct a design flaw in the original anon_inode
> > SELinux support that would make it hard to write policies for anonymous
> > inodes once more types of them are supported (currently only userfaultfd
> > inodes are). A more detailed rationale is provided in the second patch.
> >
> > The first patch extends the anon_inode_getfd_secure() function to accept
> > an additional numeric identifier that represents the type of the
> > anonymous inode being created, which is passed to the LSMs via
> > security_inode_init_security_anon().
> >
> > The second patch then introduces a new SELinux policy capability that
> > allow policies to opt-in to have a separate class used for each type of
> > anon inode. That means that the "old way" will still
>
> ... will what? :)

Whoops, I thought I had gone over all the text enough times, but
apparently not :) It should have said something along the lines of:

...will still work and will be used by default.

>
> I think it would be a very good idea if you could provide some
> concrete examples of actual policy problems encountered using the
> current approach.  I haven't looked at these patches very seriously
> yet, but my initial reaction is not "oh yes, we definitely need this".

An example is provided in patch 2. It is a generalized problem that we
would eventually run into in Fedora policy (at least) with the
unconfined_domain_type attribute and so far only hypothetical future
types of anon inodes.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [RFC PATCH 2/2] selinux: add capability to map anon inode types to separate classes

[Stephen Smalley]

On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Unfortunately, the approach chosen in commit 29cd6591ab6f ("selinux:
> teach SELinux about anonymous inodes") to use a single class for all
> anon inodes and let the policy distinguish between them using named
> transitions turned out to have a rather unfortunate drawback.
>
> For example, suppose we have two types of anon inodes, "A" and "B", and
> we want to allow a set of domains (represented by an attribute "attr_x")
> certain set of permissions on anon inodes of type "A" that were created
> by the same domain, but at the same time disallow this set to access
> anon inodes of type "B" entirely. Since all inodes share the same class
> and we want to distinguish both the inode types and the domains that
> created them, we have no choice than to create separate types for the
> cartesian product of (domains that belong to attr_x) x ("A", "B") and
> add all the necessary allow and transition rules for each domain
> individually.
>
> This makes it very impractical to write sane policies for anon inodes in
> the future, as more anon inode types are added. Therefore, this patch
> implements an alternative approach that assigns a separate class to each
> type of anon inode. This allows the example above to be implemented
> without any transition rules and with just a single allow rule:
>
> allow attr_x self:A { ... };
>
> In order to not break possible existing users of the already merged
> original approach, this patch also adds a new policy capability
> "extended_anon_inode_class" that needs to be set by the policy to enable
> the new behavior.
>
> I decided to keep the named transition mechanism in the new variant,
> since there might eventually be some extra information in the anon inode
> name that could be used in transitions.
>
> One minor annoyance is that the kernel still expects the policy to
> provide both classes (anon_inode and userfaultfd) regardless of the
> capability setting and if one of them is not defined in the policy, the
> kernel will print a warning when loading the policy. However, it doesn't
> seem worth to work around that in the kernel, as the policy can provide
> just the definition of the unused class(es) (and permissions) to avoid
> this warning. Keeping the legacy anon_inode class with some fallback
> rules may also be desirable to keep the policy compatible with kernels
> that only support anon_inode.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

NAK.  We do not want to introduce a new security class for every user
of anon inodes - that isn't what security classes are for.
For things like kvm device inodes, those should ultimately use the
inherited context from the related inode (the /dev/kvm inode itself).
That was the original intent of supporting the related inode.

> ---
>  security/selinux/hooks.c                   | 27 +++++++++++++++++++++-
>  security/selinux/include/classmap.h        |  2 ++
>  security/selinux/include/policycap.h       |  1 +
>  security/selinux/include/policycap_names.h |  3 ++-
>  security/selinux/include/security.h        |  7 ++++++
>  5 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index dc57ba21d8ff..20a8d7d17936 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3079,7 +3079,32 @@ static int selinux_inode_init_security_anon(struct inode *inode,
>                 isec->sclass = context_isec->sclass;
>                 isec->sid = context_isec->sid;
>         } else {
> -               isec->sclass = SECCLASS_ANON_INODE;
> +               /*
> +                * If the check below fails:
> +                *  1. Add the corresponding security class to
> +                *     security/selinux/include/classmap.h
> +                *  2. Map the new LSM_ANON_INODE_* value to the class in
> +                *     the switch statement below.
> +                *  3. Update the RHS of the comparison in the BUILD_BUG_ON().
> +                *  4. CC selinux@vger.kernel.org and
> +                *     linux-security-module@vger.kernel.org when submitting
> +                *     the patch or in case of any questions.
> +                */
> +               BUILD_BUG_ON(LSM_ANON_INODE_MAX > LSM_ANON_INODE_USERFAULTFD);
> +
> +               if (selinux_policycap_extended_anon_inode()) {
> +                       switch (type) {
> +                       case LSM_ANON_INODE_USERFAULTFD:
> +                               isec->sclass = SECCLASS_USERFAULTFD;
> +                               break;
> +                       default:
> +                               pr_err("SELinux:  got invalid anon inode type: %d",
> +                                      (int)type);
> +                               return -EINVAL;
> +                       }
> +               } else {
> +                       isec->sclass = SECCLASS_ANON_INODE;
> +               }
>                 rc = security_transition_sid(
>                         &selinux_state, tsec->sid, tsec->sid,
>                         isec->sclass, name, &isec->sid);
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index ba2e01a6955c..e4308cad6407 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -251,6 +251,8 @@ struct security_class_mapping secclass_map[] = {
>           { "integrity", "confidentiality", NULL } },
>         { "anon_inode",
>           { COMMON_FILE_PERMS, NULL } },
> +       { "userfaultfd",
> +         { COMMON_FILE_PERMS, NULL } },
>         { NULL }
>    };
>
> diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h
> index 2ec038efbb03..969804bd6dab 100644
> --- a/security/selinux/include/policycap.h
> +++ b/security/selinux/include/policycap.h
> @@ -11,6 +11,7 @@ enum {
>         POLICYDB_CAPABILITY_CGROUPSECLABEL,
>         POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
>         POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS,
> +       POLICYDB_CAPABILITY_EXTENDED_ANON_INODE_CLASS,
>         __POLICYDB_CAPABILITY_MAX
>  };
>  #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
> diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h
> index b89289f092c9..78651990425e 100644
> --- a/security/selinux/include/policycap_names.h
> +++ b/security/selinux/include/policycap_names.h
> @@ -12,7 +12,8 @@ const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
>         "always_check_network",
>         "cgroup_seclabel",
>         "nnp_nosuid_transition",
> -       "genfs_seclabel_symlinks"
> +       "genfs_seclabel_symlinks",
> +       "extended_anon_inode_class",
>  };
>
>  #endif /* _SELINUX_POLICYCAP_NAMES_H_ */
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 7130c9648ad1..4fb75101aca4 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -219,6 +219,13 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
>         return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]);
>  }
>
> +static inline bool selinux_policycap_extended_anon_inode(void)
> +{
> +       struct selinux_state *state = &selinux_state;
> +
> +       return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_EXTENDED_ANON_INODE_CLASS]);
> +}
> +
>  int security_mls_enabled(struct selinux_state *state);
>  int security_load_policy(struct selinux_state *state,
>                         void *data, size_t len,
> --
> 2.30.2
>

Re: [RFC PATCH 0/2] selinux,anon_inodes: Use a separate SELinux class for each type of anon inode

[Paul Moore]

On Thu, Apr 22, 2021 at 7:40 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Apr 21, 2021 at 10:38 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > This series aims to correct a design flaw in the original anon_inode
> > > SELinux support that would make it hard to write policies for anonymous
> > > inodes once more types of them are supported (currently only userfaultfd
> > > inodes are). A more detailed rationale is provided in the second patch.
> > >
> > > The first patch extends the anon_inode_getfd_secure() function to accept
> > > an additional numeric identifier that represents the type of the
> > > anonymous inode being created, which is passed to the LSMs via
> > > security_inode_init_security_anon().
> > >
> > > The second patch then introduces a new SELinux policy capability that
> > > allow policies to opt-in to have a separate class used for each type of
> > > anon inode. That means that the "old way" will still
> >
> > ... will what? :)
>
> Whoops, I thought I had gone over all the text enough times, but
> apparently not :) It should have said something along the lines of:
>
> ...will still work and will be used by default.

That's what I figured from my quick glance at the code, but I wanted
to make sure.

> > I think it would be a very good idea if you could provide some
> > concrete examples of actual policy problems encountered using the
> > current approach.  I haven't looked at these patches very seriously
> > yet, but my initial reaction is not "oh yes, we definitely need this".
>
> An example is provided in patch 2. It is a generalized problem that we
> would eventually run into in Fedora policy (at least) with the
> unconfined_domain_type attribute and so far only hypothetical future
> types of anon inodes.

Yes, I read the example you provided in patch 2, but it was still a
little too abstract for my liking.  I have the same concern that
Stephen mentioned, I was just giving you an opportunity to show that
in this case the additional object classes were warranted.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 2/2] selinux: add capability to map anon inode types to separate classes

[Ondrej Mosnacek]

On Thu, Apr 22, 2021 at 3:21 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Unfortunately, the approach chosen in commit 29cd6591ab6f ("selinux:
> > teach SELinux about anonymous inodes") to use a single class for all
> > anon inodes and let the policy distinguish between them using named
> > transitions turned out to have a rather unfortunate drawback.
> >
> > For example, suppose we have two types of anon inodes, "A" and "B", and
> > we want to allow a set of domains (represented by an attribute "attr_x")
> > certain set of permissions on anon inodes of type "A" that were created
> > by the same domain, but at the same time disallow this set to access
> > anon inodes of type "B" entirely. Since all inodes share the same class
> > and we want to distinguish both the inode types and the domains that
> > created them, we have no choice than to create separate types for the
> > cartesian product of (domains that belong to attr_x) x ("A", "B") and
> > add all the necessary allow and transition rules for each domain
> > individually.
> >
> > This makes it very impractical to write sane policies for anon inodes in
> > the future, as more anon inode types are added. Therefore, this patch
> > implements an alternative approach that assigns a separate class to each
> > type of anon inode. This allows the example above to be implemented
> > without any transition rules and with just a single allow rule:
> >
> > allow attr_x self:A { ... };
> >
> > In order to not break possible existing users of the already merged
> > original approach, this patch also adds a new policy capability
> > "extended_anon_inode_class" that needs to be set by the policy to enable
> > the new behavior.
> >
> > I decided to keep the named transition mechanism in the new variant,
> > since there might eventually be some extra information in the anon inode
> > name that could be used in transitions.
> >
> > One minor annoyance is that the kernel still expects the policy to
> > provide both classes (anon_inode and userfaultfd) regardless of the
> > capability setting and if one of them is not defined in the policy, the
> > kernel will print a warning when loading the policy. However, it doesn't
> > seem worth to work around that in the kernel, as the policy can provide
> > just the definition of the unused class(es) (and permissions) to avoid
> > this warning. Keeping the legacy anon_inode class with some fallback
> > rules may also be desirable to keep the policy compatible with kernels
> > that only support anon_inode.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> NAK.  We do not want to introduce a new security class for every user
> of anon inodes - that isn't what security classes are for.
> For things like kvm device inodes, those should ultimately use the
> inherited context from the related inode (the /dev/kvm inode itself).
> That was the original intent of supporting the related inode.

Hmm, so are you implying that anon inodes should be thought of the
same as control /dev nodes? I.e. that even though there may be many
one-time actual inodes created by different processes, they should be
thought of as a single "static interface" to the respective kernel
functionality? That would justify having a common type/label for all
of them, but I'm not sure if it doesn't open some gap due to the
possibility to pass the associated file descriptors between processes
(as AFAIK, these can hold some context)...

I thought this was supposed to resemble more the way BPF, perf_event,
etc. support was implemented - the BPF and perf_event fds are also
anon inodes under the hood, BTW - where each file descriptor is
considered a separate object that inherits the label of its creator
and there is some class separation (e.g. bpf vs. perf_event).

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [RFC PATCH 2/2] selinux: add capability to map anon inode types to separate classes

[Stephen Smalley]

On Fri, Apr 23, 2021 at 9:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Thu, Apr 22, 2021 at 3:21 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > Unfortunately, the approach chosen in commit 29cd6591ab6f ("selinux:
> > > teach SELinux about anonymous inodes") to use a single class for all
> > > anon inodes and let the policy distinguish between them using named
> > > transitions turned out to have a rather unfortunate drawback.
> > >
> > > For example, suppose we have two types of anon inodes, "A" and "B", and
> > > we want to allow a set of domains (represented by an attribute "attr_x")
> > > certain set of permissions on anon inodes of type "A" that were created
> > > by the same domain, but at the same time disallow this set to access
> > > anon inodes of type "B" entirely. Since all inodes share the same class
> > > and we want to distinguish both the inode types and the domains that
> > > created them, we have no choice than to create separate types for the
> > > cartesian product of (domains that belong to attr_x) x ("A", "B") and
> > > add all the necessary allow and transition rules for each domain
> > > individually.
> > >
> > > This makes it very impractical to write sane policies for anon inodes in
> > > the future, as more anon inode types are added. Therefore, this patch
> > > implements an alternative approach that assigns a separate class to each
> > > type of anon inode. This allows the example above to be implemented
> > > without any transition rules and with just a single allow rule:
> > >
> > > allow attr_x self:A { ... };
> > >
> > > In order to not break possible existing users of the already merged
> > > original approach, this patch also adds a new policy capability
> > > "extended_anon_inode_class" that needs to be set by the policy to enable
> > > the new behavior.
> > >
> > > I decided to keep the named transition mechanism in the new variant,
> > > since there might eventually be some extra information in the anon inode
> > > name that could be used in transitions.
> > >
> > > One minor annoyance is that the kernel still expects the policy to
> > > provide both classes (anon_inode and userfaultfd) regardless of the
> > > capability setting and if one of them is not defined in the policy, the
> > > kernel will print a warning when loading the policy. However, it doesn't
> > > seem worth to work around that in the kernel, as the policy can provide
> > > just the definition of the unused class(es) (and permissions) to avoid
> > > this warning. Keeping the legacy anon_inode class with some fallback
> > > rules may also be desirable to keep the policy compatible with kernels
> > > that only support anon_inode.
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> >
> > NAK.  We do not want to introduce a new security class for every user
> > of anon inodes - that isn't what security classes are for.
> > For things like kvm device inodes, those should ultimately use the
> > inherited context from the related inode (the /dev/kvm inode itself).
> > That was the original intent of supporting the related inode.
>
> Hmm, so are you implying that anon inodes should be thought of the
> same as control /dev nodes? I.e. that even though there may be many
> one-time actual inodes created by different processes, they should be
> thought of as a single "static interface" to the respective kernel
> functionality? That would justify having a common type/label for all
> of them, but I'm not sure if it doesn't open some gap due to the
> possibility to pass the associated file descriptors between processes
> (as AFAIK, these can hold some context)...

That was the original design (and the original patchset that we posted
in parallel with Google's independently developed one). We even had
example policy/controls for /dev/kvm ioctls.
Imagine trying to write policy over /dev/kvm ioctls where you have to
deal with N different classes and/or types and remember which ioctl
commands are exercised on which class or type even though from the
users' perspective they all occurred through the /dev/kvm interface.
It seemed super fragile and difficult to maintain/analyze that way.
Versus writing a single allow rule for all /dev/kvm ioctls.

I guess we could discuss the alternatives but please have a look at
those original patches and examples.  If we go down this road, we need
some way to deal with scaling because we only have a limited number of
discrete classes available to us and potentially unbounded set of
distinct anon inode users (although hopefully in practice only a few
that we care about distinguishing).

> I thought this was supposed to resemble more the way BPF, perf_event,
> etc. support was implemented - the BPF and perf_event fds are also
> anon inodes under the hood, BTW - where each file descriptor is
> considered a separate object that inherits the label of its creator
> and there is some class separation (e.g. bpf vs. perf_event).

Re: [RFC PATCH 2/2] selinux: add capability to map anon inode types to separate classes

[Stephen Smalley]

On Fri, Apr 23, 2021 at 10:22 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Apr 23, 2021 at 9:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Thu, Apr 22, 2021 at 3:21 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > > On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > >
> > > > Unfortunately, the approach chosen in commit 29cd6591ab6f ("selinux:
> > > > teach SELinux about anonymous inodes") to use a single class for all
> > > > anon inodes and let the policy distinguish between them using named
> > > > transitions turned out to have a rather unfortunate drawback.
> > > >
> > > > For example, suppose we have two types of anon inodes, "A" and "B", and
> > > > we want to allow a set of domains (represented by an attribute "attr_x")
> > > > certain set of permissions on anon inodes of type "A" that were created
> > > > by the same domain, but at the same time disallow this set to access
> > > > anon inodes of type "B" entirely. Since all inodes share the same class
> > > > and we want to distinguish both the inode types and the domains that
> > > > created them, we have no choice than to create separate types for the
> > > > cartesian product of (domains that belong to attr_x) x ("A", "B") and
> > > > add all the necessary allow and transition rules for each domain
> > > > individually.
> > > >
> > > > This makes it very impractical to write sane policies for anon inodes in
> > > > the future, as more anon inode types are added. Therefore, this patch
> > > > implements an alternative approach that assigns a separate class to each
> > > > type of anon inode. This allows the example above to be implemented
> > > > without any transition rules and with just a single allow rule:
> > > >
> > > > allow attr_x self:A { ... };
> > > >
> > > > In order to not break possible existing users of the already merged
> > > > original approach, this patch also adds a new policy capability
> > > > "extended_anon_inode_class" that needs to be set by the policy to enable
> > > > the new behavior.
> > > >
> > > > I decided to keep the named transition mechanism in the new variant,
> > > > since there might eventually be some extra information in the anon inode
> > > > name that could be used in transitions.
> > > >
> > > > One minor annoyance is that the kernel still expects the policy to
> > > > provide both classes (anon_inode and userfaultfd) regardless of the
> > > > capability setting and if one of them is not defined in the policy, the
> > > > kernel will print a warning when loading the policy. However, it doesn't
> > > > seem worth to work around that in the kernel, as the policy can provide
> > > > just the definition of the unused class(es) (and permissions) to avoid
> > > > this warning. Keeping the legacy anon_inode class with some fallback
> > > > rules may also be desirable to keep the policy compatible with kernels
> > > > that only support anon_inode.
> > > >
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > >
> > > NAK.  We do not want to introduce a new security class for every user
> > > of anon inodes - that isn't what security classes are for.
> > > For things like kvm device inodes, those should ultimately use the
> > > inherited context from the related inode (the /dev/kvm inode itself).
> > > That was the original intent of supporting the related inode.
> >
> > Hmm, so are you implying that anon inodes should be thought of the
> > same as control /dev nodes? I.e. that even though there may be many
> > one-time actual inodes created by different processes, they should be
> > thought of as a single "static interface" to the respective kernel
> > functionality? That would justify having a common type/label for all
> > of them, but I'm not sure if it doesn't open some gap due to the
> > possibility to pass the associated file descriptors between processes
> > (as AFAIK, these can hold some context)...
>
> That was the original design (and the original patchset that we posted
> in parallel with Google's independently developed one). We even had
> example policy/controls for /dev/kvm ioctls.
> Imagine trying to write policy over /dev/kvm ioctls where you have to
> deal with N different classes and/or types and remember which ioctl
> commands are exercised on which class or type even though from the
> users' perspective they all occurred through the /dev/kvm interface.
> It seemed super fragile and difficult to maintain/analyze that way.
> Versus writing a single allow rule for all /dev/kvm ioctls.
>
> I guess we could discuss the alternatives but please have a look at
> those original patches and examples.  If we go down this road, we need
> some way to deal with scaling because we only have a limited number of
> discrete classes available to us and potentially unbounded set of
> distinct anon inode users (although hopefully in practice only a few
> that we care about distinguishing).

Actually, on second thought, we shouldn't be in any danger of running
out of classes so nevermind on that point.

>
> > I thought this was supposed to resemble more the way BPF, perf_event,
> > etc. support was implemented - the BPF and perf_event fds are also
> > anon inodes under the hood, BTW - where each file descriptor is
> > considered a separate object that inherits the label of its creator
> > and there is some class separation (e.g. bpf vs. perf_event).

Re: [RFC PATCH 2/2] selinux: add capability to map anon inode types to separate classes

[Ondrej Mosnacek]

On Fri, Apr 23, 2021 at 5:20 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Fri, Apr 23, 2021 at 10:22 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Apr 23, 2021 at 9:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > On Thu, Apr 22, 2021 at 3:21 PM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > > On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > >
> > > > > Unfortunately, the approach chosen in commit 29cd6591ab6f ("selinux:
> > > > > teach SELinux about anonymous inodes") to use a single class for all
> > > > > anon inodes and let the policy distinguish between them using named
> > > > > transitions turned out to have a rather unfortunate drawback.
> > > > >
> > > > > For example, suppose we have two types of anon inodes, "A" and "B", and
> > > > > we want to allow a set of domains (represented by an attribute "attr_x")
> > > > > certain set of permissions on anon inodes of type "A" that were created
> > > > > by the same domain, but at the same time disallow this set to access
> > > > > anon inodes of type "B" entirely. Since all inodes share the same class
> > > > > and we want to distinguish both the inode types and the domains that
> > > > > created them, we have no choice than to create separate types for the
> > > > > cartesian product of (domains that belong to attr_x) x ("A", "B") and
> > > > > add all the necessary allow and transition rules for each domain
> > > > > individually.
> > > > >
> > > > > This makes it very impractical to write sane policies for anon inodes in
> > > > > the future, as more anon inode types are added. Therefore, this patch
> > > > > implements an alternative approach that assigns a separate class to each
> > > > > type of anon inode. This allows the example above to be implemented
> > > > > without any transition rules and with just a single allow rule:
> > > > >
> > > > > allow attr_x self:A { ... };
> > > > >
> > > > > In order to not break possible existing users of the already merged
> > > > > original approach, this patch also adds a new policy capability
> > > > > "extended_anon_inode_class" that needs to be set by the policy to enable
> > > > > the new behavior.
> > > > >
> > > > > I decided to keep the named transition mechanism in the new variant,
> > > > > since there might eventually be some extra information in the anon inode
> > > > > name that could be used in transitions.
> > > > >
> > > > > One minor annoyance is that the kernel still expects the policy to
> > > > > provide both classes (anon_inode and userfaultfd) regardless of the
> > > > > capability setting and if one of them is not defined in the policy, the
> > > > > kernel will print a warning when loading the policy. However, it doesn't
> > > > > seem worth to work around that in the kernel, as the policy can provide
> > > > > just the definition of the unused class(es) (and permissions) to avoid
> > > > > this warning. Keeping the legacy anon_inode class with some fallback
> > > > > rules may also be desirable to keep the policy compatible with kernels
> > > > > that only support anon_inode.
> > > > >
> > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > >
> > > > NAK.  We do not want to introduce a new security class for every user
> > > > of anon inodes - that isn't what security classes are for.
> > > > For things like kvm device inodes, those should ultimately use the
> > > > inherited context from the related inode (the /dev/kvm inode itself).
> > > > That was the original intent of supporting the related inode.
> > >
> > > Hmm, so are you implying that anon inodes should be thought of the
> > > same as control /dev nodes? I.e. that even though there may be many
> > > one-time actual inodes created by different processes, they should be
> > > thought of as a single "static interface" to the respective kernel
> > > functionality? That would justify having a common type/label for all
> > > of them, but I'm not sure if it doesn't open some gap due to the
> > > possibility to pass the associated file descriptors between processes
> > > (as AFAIK, these can hold some context)...
> >
> > That was the original design (and the original patchset that we posted
> > in parallel with Google's independently developed one). We even had
> > example policy/controls for /dev/kvm ioctls.
> > Imagine trying to write policy over /dev/kvm ioctls where you have to
> > deal with N different classes and/or types and remember which ioctl
> > commands are exercised on which class or type even though from the
> > users' perspective they all occurred through the /dev/kvm interface.
> > It seemed super fragile and difficult to maintain/analyze that way.
> > Versus writing a single allow rule for all /dev/kvm ioctls.

So I went back and read the conversations on the original patches and
after thinking a bit more about it I'm getting more comfortable with
the idea to treat anonymous inodes as a kind of opened device node
file (which can be either an "imaginary" one as in the userfaultfd(2)
case or an existing real device node as in the KVM example), which has
just one type regardless of what process has created it.

I suppose if there are any properties of an open anonymous inode that
would be interesting from SELinux POV, they could be checked via a
separate class with an appropriate permission. For example, if we
wanted to control which domains can create & use usefaultfds without
vs. with the UFFD_USER_MODE_ONLY flag, we could introduce a new hook,
which would allow us to check an access vector like (current_sid,
current_sid, userfaultfd, kernel_mode) on any attempt to create or
inherit/receive an uffd without the UFFD_USER_MODE_ONLY flag. (In
fact, this might actually be a useful enhancement...)

So I'm retracting these patches for now.

> >
> > I guess we could discuss the alternatives but please have a look at
> > those original patches and examples.  If we go down this road, we need
> > some way to deal with scaling because we only have a limited number of
> > discrete classes available to us and potentially unbounded set of
> > distinct anon inode users (although hopefully in practice only a few
> > that we care about distinguishing).
>
> Actually, on second thought, we shouldn't be in any danger of running
> out of classes so nevermind on that point.
>
> >
> > > I thought this was supposed to resemble more the way BPF, perf_event,
> > > etc. support was implemented - the BPF and perf_event fds are also
> > > anon inodes under the hood, BTW - where each file descriptor is
> > > considered a separate object that inherits the label of its creator
> > > and there is some class separation (e.g. bpf vs. perf_event).
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.