On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxx and
> + * linux-security-module@xxxxxxxxxxxxxxx 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
>
