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
Changes from the twelfth version of the patch:
- Replaced FILE__CREATE with ANON_INODE__CREATE while initializing
anon-inode's SELinux security struct.
- Check context_inode's SELinux label and return -EACCES if it's
invalid.
[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/linux-fsdevel/[email protected]/
[3] https://lore.kernel.org/lkml/[email protected]/
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 | 56 +++++++++++
security/selinux/include/classmap.h | 2 +
10 files changed, 212 insertions(+), 54 deletions(-)
--
2.29.2.299.gdc1121823c-goog
From: Daniel Colascione <[email protected]>
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 <[email protected]>
Signed-off-by: Lokesh Gidra <[email protected]>
---
security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 ++
2 files changed, 58 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6b1826fc3658..d092aa512868 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2927,6 +2927,61 @@ 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);
+ if (context_isec->initialized != LABEL_INITIALIZED)
+ return -EACCES;
+
+ 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,
+ ANON_INODE__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 +7047,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.2.299.gdc1121823c-goog
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 <[email protected]>
Reviewed-by: Eric Biggers <[email protected]>
---
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.2.299.gdc1121823c-goog
On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <[email protected]> wrote:
> From: Daniel Colascione <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Lokesh Gidra <[email protected]>
> ---
> security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 2 ++
> 2 files changed, 58 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6b1826fc3658..d092aa512868 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2927,6 +2927,61 @@ 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);
> + if (context_isec->initialized != LABEL_INITIALIZED)
> + return -EACCES;
> +
> + isec->sclass = context_isec->sclass;
Taking the object class directly from the context_inode is
interesting, and I suspect problematic. In the case below where no
context_inode is supplied the object class is set to
SECCLASS_ANON_INODE, which is correct, but when a context_inode is
supplied there is no guarantee that the object class will be set to
SECCLASS_ANON_INODE. This could both pose a problem for policy
writers (how do you distinguish the anon inode from other normal file
inodes in this case?) as well as an outright fault later in this
function when we try to check the ANON_INODE__CREATE on an object
other than a SECCLASS_ANON_INODE object.
It works in the userfaultfd case because the context_inode is
originally created with this function so the object class is correctly
set to SECCLASS_ANON_INODE, but can we always guarantee that to be the
case? Do we ever need or want to support using a context_inode that
is not SECCLASS_ANON_INODE?
> + 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,
> + ANON_INODE__CREATE,
> + &ad);
> +}
--
paul moore
http://www.paul-moore.com
On Wed, Jan 6, 2021 at 7:03 PM Paul Moore <[email protected]> wrote:
>
> On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <[email protected]> wrote:
> > From: Daniel Colascione <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Lokesh Gidra <[email protected]>
> > ---
> > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++
> > security/selinux/include/classmap.h | 2 ++
> > 2 files changed, 58 insertions(+)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 6b1826fc3658..d092aa512868 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2927,6 +2927,61 @@ 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);
> > + if (context_isec->initialized != LABEL_INITIALIZED)
> > + return -EACCES;
> > +
> > + isec->sclass = context_isec->sclass;
>
> Taking the object class directly from the context_inode is
> interesting, and I suspect problematic. In the case below where no
> context_inode is supplied the object class is set to
> SECCLASS_ANON_INODE, which is correct, but when a context_inode is
> supplied there is no guarantee that the object class will be set to
> SECCLASS_ANON_INODE. This could both pose a problem for policy
> writers (how do you distinguish the anon inode from other normal file
> inodes in this case?) as well as an outright fault later in this
> function when we try to check the ANON_INODE__CREATE on an object
> other than a SECCLASS_ANON_INODE object.
>
Thanks for catching this. I'll initialize 'sclass' unconditionally to
SECCLASS_ANON_INODE in the next version. Also, do you think I should
add a check that context_inode's sclass must be SECCLASS_ANON_INODE to
confirm that we never receive a regular inode as context_inode?
> It works in the userfaultfd case because the context_inode is
> originally created with this function so the object class is correctly
> set to SECCLASS_ANON_INODE, but can we always guarantee that to be the
> case? Do we ever need or want to support using a context_inode that
> is not SECCLASS_ANON_INODE?
>
I don't think there is any requirement of supporting context_inode
which isn't anon-inode. And even if there is, as you described
earlier, for ANON_INODE__CREATE to work the sclass has to be
SECCLASS_ANON_INODE. I'll appreciate comments on this from others,
particularly Daniel and Stephen who originally discussed and
implemented this patch.
> > + 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,
> > + ANON_INODE__CREATE,
> > + &ad);
> > +}
>
> --
> paul moore
> http://www.paul-moore.com
On Wed, Jan 6, 2021 at 10:55 PM Lokesh Gidra <[email protected]> wrote:
> On Wed, Jan 6, 2021 at 7:03 PM Paul Moore <[email protected]> wrote:
> > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <[email protected]> wrote:
> > > From: Daniel Colascione <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Lokesh Gidra <[email protected]>
> > > ---
> > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++
> > > security/selinux/include/classmap.h | 2 ++
> > > 2 files changed, 58 insertions(+)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 6b1826fc3658..d092aa512868 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2927,6 +2927,61 @@ 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);
> > > + if (context_isec->initialized != LABEL_INITIALIZED)
> > > + return -EACCES;
> > > +
> > > + isec->sclass = context_isec->sclass;
> >
> > Taking the object class directly from the context_inode is
> > interesting, and I suspect problematic. In the case below where no
> > context_inode is supplied the object class is set to
> > SECCLASS_ANON_INODE, which is correct, but when a context_inode is
> > supplied there is no guarantee that the object class will be set to
> > SECCLASS_ANON_INODE. This could both pose a problem for policy
> > writers (how do you distinguish the anon inode from other normal file
> > inodes in this case?) as well as an outright fault later in this
> > function when we try to check the ANON_INODE__CREATE on an object
> > other than a SECCLASS_ANON_INODE object.
> >
> Thanks for catching this. I'll initialize 'sclass' unconditionally to
> SECCLASS_ANON_INODE in the next version. Also, do you think I should
> add a check that context_inode's sclass must be SECCLASS_ANON_INODE to
> confirm that we never receive a regular inode as context_inode?
This is one of the reasons why I was asking if you ever saw the need
to use a regular inode here. It seems much safer to me to add a check
to ensure that context_inode is SECCLASS_ANON_INODE and return an
error otherwise; I would also suggest emitting an error using pr_err()
with something along the lines of "SELinux: initializing anonymous
inode with inappropriate inode" (or something similar).
If something changes in the future we can always reconsider this restriction.
> > It works in the userfaultfd case because the context_inode is
> > originally created with this function so the object class is correctly
> > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the
> > case? Do we ever need or want to support using a context_inode that
> > is not SECCLASS_ANON_INODE?
>
> I don't think there is any requirement of supporting context_inode
> which isn't anon-inode. And even if there is, as you described
> earlier, for ANON_INODE__CREATE to work the sclass has to be
> SECCLASS_ANON_INODE. I'll appreciate comments on this from others,
> particularly Daniel and Stephen who originally discussed and
> implemented this patch.
I would encourage you not to wait too long for additional feedback
before sending the next revision.
--
paul moore
http://www.paul-moore.com
On Thu, Jan 7, 2021 at 2:30 PM Paul Moore <[email protected]> wrote:
>
> On Wed, Jan 6, 2021 at 10:55 PM Lokesh Gidra <[email protected]> wrote:
> > On Wed, Jan 6, 2021 at 7:03 PM Paul Moore <[email protected]> wrote:
> > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <[email protected]> wrote:
> > > > From: Daniel Colascione <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Lokesh Gidra <[email protected]>
> > > > ---
> > > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++
> > > > security/selinux/include/classmap.h | 2 ++
> > > > 2 files changed, 58 insertions(+)
> > > >
> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index 6b1826fc3658..d092aa512868 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -2927,6 +2927,61 @@ 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);
> > > > + if (context_isec->initialized != LABEL_INITIALIZED)
> > > > + return -EACCES;
> > > > +
> > > > + isec->sclass = context_isec->sclass;
> > >
> > > Taking the object class directly from the context_inode is
> > > interesting, and I suspect problematic. In the case below where no
> > > context_inode is supplied the object class is set to
> > > SECCLASS_ANON_INODE, which is correct, but when a context_inode is
> > > supplied there is no guarantee that the object class will be set to
> > > SECCLASS_ANON_INODE. This could both pose a problem for policy
> > > writers (how do you distinguish the anon inode from other normal file
> > > inodes in this case?) as well as an outright fault later in this
> > > function when we try to check the ANON_INODE__CREATE on an object
> > > other than a SECCLASS_ANON_INODE object.
> > >
> > Thanks for catching this. I'll initialize 'sclass' unconditionally to
> > SECCLASS_ANON_INODE in the next version. Also, do you think I should
> > add a check that context_inode's sclass must be SECCLASS_ANON_INODE to
> > confirm that we never receive a regular inode as context_inode?
>
> This is one of the reasons why I was asking if you ever saw the need
> to use a regular inode here. It seems much safer to me to add a check
> to ensure that context_inode is SECCLASS_ANON_INODE and return an
> error otherwise; I would also suggest emitting an error using pr_err()
> with something along the lines of "SELinux: initializing anonymous
> inode with inappropriate inode" (or something similar).
>
Thanks. I'll do that.
> If something changes in the future we can always reconsider this restriction.
>
> > > It works in the userfaultfd case because the context_inode is
> > > originally created with this function so the object class is correctly
> > > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the
> > > case? Do we ever need or want to support using a context_inode that
> > > is not SECCLASS_ANON_INODE?
> >
> > I don't think there is any requirement of supporting context_inode
> > which isn't anon-inode. And even if there is, as you described
> > earlier, for ANON_INODE__CREATE to work the sclass has to be
> > SECCLASS_ANON_INODE. I'll appreciate comments on this from others,
> > particularly Daniel and Stephen who originally discussed and
> > implemented this patch.
>
> I would encourage you not to wait too long for additional feedback
> before sending the next revision.
Certainly. I'll send next version in a day or two.
>
> --
> paul moore
> http://www.paul-moore.com
On Wed, Jan 6, 2021 at 10:03 PM Paul Moore <[email protected]> wrote:
>
> On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <[email protected]> wrote:
> > From: Daniel Colascione <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Lokesh Gidra <[email protected]>
> > ---
> > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++
> > security/selinux/include/classmap.h | 2 ++
> > 2 files changed, 58 insertions(+)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 6b1826fc3658..d092aa512868 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2927,6 +2927,61 @@ 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);
> > + if (context_isec->initialized != LABEL_INITIALIZED)
> > + return -EACCES;
> > +
> > + isec->sclass = context_isec->sclass;
>
> Taking the object class directly from the context_inode is
> interesting, and I suspect problematic. In the case below where no
> context_inode is supplied the object class is set to
> SECCLASS_ANON_INODE, which is correct, but when a context_inode is
> supplied there is no guarantee that the object class will be set to
> SECCLASS_ANON_INODE. This could both pose a problem for policy
> writers (how do you distinguish the anon inode from other normal file
> inodes in this case?) as well as an outright fault later in this
> function when we try to check the ANON_INODE__CREATE on an object
> other than a SECCLASS_ANON_INODE object.
>
> It works in the userfaultfd case because the context_inode is
> originally created with this function so the object class is correctly
> set to SECCLASS_ANON_INODE, but can we always guarantee that to be the
> case? Do we ever need or want to support using a context_inode that
> is not SECCLASS_ANON_INODE?
Sorry, I haven't been following this. IIRC, the original reason for
passing a context_inode was to support the /dev/kvm or similar use
cases where the driver is creating anonymous inodes to represent
specific objects/interfaces derived from the device node and we want
to be able to control subsequent ioctl operations on those anonymous
inodes in the same manner as for the device node. For example, ioctl
operations on /dev/kvm can end up returning file descriptors for
anonymous inodes representing a specific VM or VCPU or similar. If we
propagate the security class and SID from the /dev/kvm inode (the
context inode) to the new anonymous inode, we can write a single
policy rule over all ioctl operations related to /dev/kvm. That's
also why we used the FILE__CREATE permission here originally; that was
also intentional. All the file-related classes including anon_inode
inherit a common set of file permissions including create and thus we
often use the FILE__<permission> in common code when checking
permission against any potentially derived class.
On Fri, Jan 8, 2021 at 11:35 AM Stephen Smalley
<[email protected]> wrote:
>
> On Wed, Jan 6, 2021 at 10:03 PM Paul Moore <[email protected]> wrote:
> >
> > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <[email protected]> wrote:
> > > From: Daniel Colascione <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Lokesh Gidra <[email protected]>
> > > ---
> > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++
> > > security/selinux/include/classmap.h | 2 ++
> > > 2 files changed, 58 insertions(+)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 6b1826fc3658..d092aa512868 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2927,6 +2927,61 @@ 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);
> > > + if (context_isec->initialized != LABEL_INITIALIZED)
> > > + return -EACCES;
Stephen, as per your explanation below, is this check also
problematic? I mean is it possible that /dev/kvm context_inode may not
have its label initialized? If so, then v12 of the patch series can be
used as is. Otherwise, I will send the next version which rollbacks
v14 and v13, except for this check. Kindly confirm.
> > > +
> > > + isec->sclass = context_isec->sclass;
> >
> > Taking the object class directly from the context_inode is
> > interesting, and I suspect problematic. In the case below where no
> > context_inode is supplied the object class is set to
> > SECCLASS_ANON_INODE, which is correct, but when a context_inode is
> > supplied there is no guarantee that the object class will be set to
> > SECCLASS_ANON_INODE. This could both pose a problem for policy
> > writers (how do you distinguish the anon inode from other normal file
> > inodes in this case?) as well as an outright fault later in this
> > function when we try to check the ANON_INODE__CREATE on an object
> > other than a SECCLASS_ANON_INODE object.
> >
> > It works in the userfaultfd case because the context_inode is
> > originally created with this function so the object class is correctly
> > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the
> > case? Do we ever need or want to support using a context_inode that
> > is not SECCLASS_ANON_INODE?
>
> Sorry, I haven't been following this. IIRC, the original reason for
> passing a context_inode was to support the /dev/kvm or similar use
> cases where the driver is creating anonymous inodes to represent
> specific objects/interfaces derived from the device node and we want
> to be able to control subsequent ioctl operations on those anonymous
> inodes in the same manner as for the device node. For example, ioctl
> operations on /dev/kvm can end up returning file descriptors for
> anonymous inodes representing a specific VM or VCPU or similar. If we
> propagate the security class and SID from the /dev/kvm inode (the
> context inode) to the new anonymous inode, we can write a single
> policy rule over all ioctl operations related to /dev/kvm. That's
> also why we used the FILE__CREATE permission here originally; that was
> also intentional. All the file-related classes including anon_inode
> inherit a common set of file permissions including create and thus we
> often use the FILE__<permission> in common code when checking
> permission against any potentially derived class.
Thanks a lot for the explanation.
On Fri, Jan 8, 2021 at 2:35 PM Stephen Smalley
<[email protected]> wrote:
> On Wed, Jan 6, 2021 at 10:03 PM Paul Moore <[email protected]> wrote:
> > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <[email protected]> wrote:
> > > From: Daniel Colascione <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Lokesh Gidra <[email protected]>
> > > ---
> > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++
> > > security/selinux/include/classmap.h | 2 ++
> > > 2 files changed, 58 insertions(+)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 6b1826fc3658..d092aa512868 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2927,6 +2927,61 @@ 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);
> > > + if (context_isec->initialized != LABEL_INITIALIZED)
> > > + return -EACCES;
> > > +
> > > + isec->sclass = context_isec->sclass;
> >
> > Taking the object class directly from the context_inode is
> > interesting, and I suspect problematic. In the case below where no
> > context_inode is supplied the object class is set to
> > SECCLASS_ANON_INODE, which is correct, but when a context_inode is
> > supplied there is no guarantee that the object class will be set to
> > SECCLASS_ANON_INODE. This could both pose a problem for policy
> > writers (how do you distinguish the anon inode from other normal file
> > inodes in this case?) as well as an outright fault later in this
> > function when we try to check the ANON_INODE__CREATE on an object
> > other than a SECCLASS_ANON_INODE object.
> >
> > It works in the userfaultfd case because the context_inode is
> > originally created with this function so the object class is correctly
> > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the
> > case? Do we ever need or want to support using a context_inode that
> > is not SECCLASS_ANON_INODE?
>
> Sorry, I haven't been following this. IIRC, the original reason for
> passing a context_inode was to support the /dev/kvm or similar use
> cases where the driver is creating anonymous inodes to represent
> specific objects/interfaces derived from the device node and we want
> to be able to control subsequent ioctl operations on those anonymous
> inodes in the same manner as for the device node. For example, ioctl
> operations on /dev/kvm can end up returning file descriptors for
> anonymous inodes representing a specific VM or VCPU or similar. If we
> propagate the security class and SID from the /dev/kvm inode (the
> context inode) to the new anonymous inode, we can write a single
> policy rule over all ioctl operations related to /dev/kvm.
Thanks for the background, and the /dev/kvm example, that is what I was missing.
> That's
> also why we used the FILE__CREATE permission here originally; that was
> also intentional. All the file-related classes including anon_inode
> inherit a common set of file permissions including create and thus we
> often use the FILE__<permission> in common code when checking
> permission against any potentially derived class.
Yes, if all of the anonymous inodes are not going to fall into the
anon_inode object class then FILE__CREATE makes the most sense.
Thanks Stephen.
--
paul moore
http://www.paul-moore.com
On Fri, Jan 8, 2021 at 3:17 PM Lokesh Gidra <[email protected]> wrote:
>
> On Fri, Jan 8, 2021 at 11:35 AM Stephen Smalley
> <[email protected]> wrote:
> >
> > On Wed, Jan 6, 2021 at 10:03 PM Paul Moore <[email protected]> wrote:
> > >
> > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <[email protected]> wrote:
> > > > From: Daniel Colascione <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Lokesh Gidra <[email protected]>
> > > > ---
> > > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++
> > > > security/selinux/include/classmap.h | 2 ++
> > > > 2 files changed, 58 insertions(+)
> > > >
> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index 6b1826fc3658..d092aa512868 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -2927,6 +2927,61 @@ 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);
> > > > + if (context_isec->initialized != LABEL_INITIALIZED)
> > > > + return -EACCES;
> Stephen, as per your explanation below, is this check also
> problematic? I mean is it possible that /dev/kvm context_inode may not
> have its label initialized? If so, then v12 of the patch series can be
> used as is. Otherwise, I will send the next version which rollbacks
> v14 and v13, except for this check. Kindly confirm.
The context_inode should always be initialized already. I'm not fond
though of silently returning -EACCES here. At the least we should
have a pr_err() or pr_warn() here. In reality, this could only occur
in the case of a kernel bug or memory corruption so it used to be a
candidate for WARN_ON() or BUG_ON() or similar but I know that
BUG_ON() at least is frowned upon these days.
On Fri, Jan 8, 2021 at 1:24 PM Stephen Smalley
<[email protected]> wrote:
>
> On Fri, Jan 8, 2021 at 3:17 PM Lokesh Gidra <[email protected]> wrote:
> >
> > On Fri, Jan 8, 2021 at 11:35 AM Stephen Smalley
> > <[email protected]> wrote:
> > >
> > > On Wed, Jan 6, 2021 at 10:03 PM Paul Moore <[email protected]> wrote:
> > > >
> > > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra <[email protected]> wrote:
> > > > > From: Daniel Colascione <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > Signed-off-by: Lokesh Gidra <[email protected]>
> > > > > ---
> > > > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++
> > > > > security/selinux/include/classmap.h | 2 ++
> > > > > 2 files changed, 58 insertions(+)
> > > > >
> > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > > index 6b1826fc3658..d092aa512868 100644
> > > > > --- a/security/selinux/hooks.c
> > > > > +++ b/security/selinux/hooks.c
> > > > > @@ -2927,6 +2927,61 @@ 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);
> > > > > + if (context_isec->initialized != LABEL_INITIALIZED)
> > > > > + return -EACCES;
> > Stephen, as per your explanation below, is this check also
> > problematic? I mean is it possible that /dev/kvm context_inode may not
> > have its label initialized? If so, then v12 of the patch series can be
> > used as is. Otherwise, I will send the next version which rollbacks
> > v14 and v13, except for this check. Kindly confirm.
>
> The context_inode should always be initialized already. I'm not fond
> though of silently returning -EACCES here. At the least we should
> have a pr_err() or pr_warn() here. In reality, this could only occur
> in the case of a kernel bug or memory corruption so it used to be a
> candidate for WARN_ON() or BUG_ON() or similar but I know that
> BUG_ON() at least is frowned upon these days.
Got it. I'll add a pr_err(). Thanks a lot.