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 allows administrators to limit userfaultfd to
servicing user-mode faults, increasing the difficulty of using
userfaultfd in exploit chains invoking delaying kernel faults.
A new anon_inodes interface allows callers to opt into SELinux
management of anonymous file objects. In this mode, anon_inodes
creates new ephemeral inodes for anonymous file objects instead of
reusing a singleton dummy inode. A new LSM hook gives security modules
an opportunity to configure and veto these ephemeral inodes.
Existing anon_inodes users must opt into the new functionality.
Daniel Colascione (6):
Add a new flags-accepting interface for anonymous inodes
Add a concept of a "secure" anonymous file
Teach SELinux about a new userfaultfd class
Wire UFFD up to SELinux
Let userfaultfd opt out of handling kernel-mode faults
Add a new sysctl for limiting userfaultfd to user mode faults
Documentation/admin-guide/sysctl/vm.rst | 13 ++++
fs/anon_inodes.c | 89 +++++++++++++++++--------
fs/userfaultfd.c | 29 ++++++--
include/linux/anon_inodes.h | 27 ++++++--
include/linux/lsm_hooks.h | 8 +++
include/linux/security.h | 2 +
include/linux/userfaultfd_k.h | 3 +
include/uapi/linux/userfaultfd.h | 9 +++
kernel/sysctl.c | 9 +++
security/security.c | 8 +++
security/selinux/hooks.c | 68 +++++++++++++++++++
security/selinux/include/classmap.h | 2 +
12 files changed, 229 insertions(+), 38 deletions(-)
--
2.25.0.225.g125e21ebc7-goog
Use the secure anonymous inode LSM hook we just added to let SELinux
policy place restrictions on userfaultfd use. The create operation
applies to processes creating new instances of these file objects;
transfer between processes is covered by restrictions on read, write,
and ioctl access already checked inside selinux_file_receive.
Signed-off-by: Daniel Colascione <[email protected]>
---
fs/userfaultfd.c | 4 +-
include/linux/userfaultfd_k.h | 2 +
security/selinux/hooks.c | 68 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 +
4 files changed, 73 insertions(+), 3 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 37df7c9eedb1..07b0f6e03849 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1014,8 +1014,6 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
}
}
-static const struct file_operations userfaultfd_fops;
-
static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
struct userfaultfd_ctx *new,
struct uffd_msg *msg)
@@ -1920,7 +1918,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f)
}
#endif
-static const struct file_operations userfaultfd_fops = {
+const struct file_operations userfaultfd_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = userfaultfd_show_fdinfo,
#endif
diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index ac9d71e24b81..549c8b0cca52 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -30,6 +30,8 @@
extern int sysctl_unprivileged_userfaultfd;
+extern const struct file_operations userfaultfd_fops;
+
extern vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason);
extern ssize_t mcopy_atomic(struct mm_struct *dst_mm, unsigned long dst_start,
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1659b59fb5d7..e178f6f40e93 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -92,6 +92,10 @@
#include <linux/fsnotify.h>
#include <linux/fanotify.h>
+#ifdef CONFIG_USERFAULTFD
+#include <linux/userfaultfd_k.h>
+#endif
+
#include "avc.h"
#include "objsec.h"
#include "netif.h"
@@ -2915,6 +2919,69 @@ 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 char *name,
+ const struct file_operations *fops)
+{
+ const struct task_security_struct *tsec = selinux_cred(current_cred());
+ struct common_audit_data ad;
+ struct inode_security_struct *isec;
+
+ if (unlikely(IS_PRIVATE(inode)))
+ return 0;
+
+ /*
+ * We shouldn't be creating secure anonymous inodes before LSM
+ * initialization completes.
+ */
+ if (unlikely(!selinux_state.initialized))
+ return -EBUSY;
+
+ 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, so check that the state is as
+ * inode_alloc_security left it.
+ */
+ BUG_ON(isec->initialized != LABEL_INVALID);
+ BUG_ON(isec->sclass != SECCLASS_FILE);
+
+#ifdef CONFIG_USERFAULTFD
+ if (fops == &userfaultfd_fops)
+ isec->sclass = SECCLASS_UFFD;
+#endif
+
+ if (isec->sclass == SECCLASS_FILE) {
+ printk(KERN_WARNING "refusing to create secure anonymous inode "
+ "of unknown type");
+ return -EOPNOTSUPP;
+ }
+ /*
+ * Always give secure anonymous inodes the sid of the
+ * creating task.
+ */
+
+ isec->sid = tsec->sid;
+ isec->initialized = LABEL_INITIALIZED;
+
+ /*
+ * Now that we've initialized security, check whether we're
+ * allowed to actually create this type of anonymous inode.
+ */
+
+ ad.type = LSM_AUDIT_DATA_INODE;
+ ad.u.inode = inode;
+
+ return avc_has_perm(&selinux_state,
+ tsec->sid,
+ isec->sid,
+ isec->sclass,
+ FILE__CREATE,
+ &ad);
+}
+
static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
{
return may_create(dir, dentry, SECCLASS_FILE);
@@ -6923,6 +6990,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 986f3ac14282..6d4f9ebf2017 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -248,6 +248,8 @@ struct security_class_mapping secclass_map[] = {
{"open", "cpu", "kernel", "tracepoint", "read", "write"} },
{ "lockdown",
{ "integrity", "confidentiality", NULL } },
+ { "uffd",
+ { COMMON_FILE_PERMS, NULL } },
{ NULL }
};
--
2.25.0.225.g125e21ebc7-goog
On 2/11/2020 2:55 PM, Daniel Colascione wrote:
> 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 allows administrators to limit userfaultfd to
> servicing user-mode faults, increasing the difficulty of using
> userfaultfd in exploit chains invoking delaying kernel faults.
>
> A new anon_inodes interface allows callers to opt into SELinux
> management of anonymous file objects. In this mode, anon_inodes
> creates new ephemeral inodes for anonymous file objects instead of
> reusing a singleton dummy inode. A new LSM hook gives security modules
> an opportunity to configure and veto these ephemeral inodes.
>
> Existing anon_inodes users must opt into the new functionality.
>
> Daniel Colascione (6):
> Add a new flags-accepting interface for anonymous inodes
> Add a concept of a "secure" anonymous file
> Teach SELinux about a new userfaultfd class
> Wire UFFD up to SELinux
> Let userfaultfd opt out of handling kernel-mode faults
> Add a new sysctl for limiting userfaultfd to user mode faults
This must be posted to the linux Security Module list
<[email protected]>
>
> Documentation/admin-guide/sysctl/vm.rst | 13 ++++
> fs/anon_inodes.c | 89 +++++++++++++++++--------
> fs/userfaultfd.c | 29 ++++++--
> include/linux/anon_inodes.h | 27 ++++++--
> include/linux/lsm_hooks.h | 8 +++
> include/linux/security.h | 2 +
> include/linux/userfaultfd_k.h | 3 +
> include/uapi/linux/userfaultfd.h | 9 +++
> kernel/sysctl.c | 9 +++
> security/security.c | 8 +++
> security/selinux/hooks.c | 68 +++++++++++++++++++
> security/selinux/include/classmap.h | 2 +
> 12 files changed, 229 insertions(+), 38 deletions(-)
>
On Tue, Feb 11, 2020 at 3:13 PM Casey Schaufler <[email protected]> wrote:
>
> On 2/11/2020 2:55 PM, Daniel Colascione wrote:
> > 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 allows administrators to limit userfaultfd to
> > servicing user-mode faults, increasing the difficulty of using
> > userfaultfd in exploit chains invoking delaying kernel faults.
> >
> > A new anon_inodes interface allows callers to opt into SELinux
> > management of anonymous file objects. In this mode, anon_inodes
> > creates new ephemeral inodes for anonymous file objects instead of
> > reusing a singleton dummy inode. A new LSM hook gives security modules
> > an opportunity to configure and veto these ephemeral inodes.
> >
> > Existing anon_inodes users must opt into the new functionality.
> >
> > Daniel Colascione (6):
> > Add a new flags-accepting interface for anonymous inodes
> > Add a concept of a "secure" anonymous file
> > Teach SELinux about a new userfaultfd class
> > Wire UFFD up to SELinux
> > Let userfaultfd opt out of handling kernel-mode faults
> > Add a new sysctl for limiting userfaultfd to user mode faults
>
> This must be posted to the linux Security Module list
> <[email protected]>
Added. I thought selinux@ was sufficient.
Hi!
Firstly, thanks for working on this! It's been on my TODO list for a
while. :)
Casey already recommended including the LSM list to CC (since this is a
new LSM -- there are many LSMs). Additionally, the series should
probably be sent _to_ the userfaultfd maintainers:
Andrea Arcangeli <[email protected]>
Mike Rapoport <[email protected]>
and I'd also CC a couple other people that have done recent work:
Peter Xu <[email protected]>
Jann Horn <[email protected]>
More notes below...
On Tue, Feb 11, 2020 at 02:55:41PM -0800, Daniel Colascione wrote:
> 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 allows administrators to limit userfaultfd to
> servicing user-mode faults, increasing the difficulty of using
> userfaultfd in exploit chains invoking delaying kernel faults.
I actually think these are two very different goals and likely the
series could be split into two for them. One is LSM hooking of
userfaultfd and the SELinux attachment, and the other is the user-mode
fault restrictions. And they would likely go via separate trees (LSM
through James's LSM tree, and probably akpm's -mm tree for the sysctl).
> A new anon_inodes interface allows callers to opt into SELinux
> management of anonymous file objects. In this mode, anon_inodes
> creates new ephemeral inodes for anonymous file objects instead of
> reusing a singleton dummy inode. A new LSM hook gives security modules
> an opportunity to configure and veto these ephemeral inodes.
>
> Existing anon_inodes users must opt into the new functionality.
>
> Daniel Colascione (6):
> Add a new flags-accepting interface for anonymous inodes
> Add a concept of a "secure" anonymous file
> Teach SELinux about a new userfaultfd class
> Wire UFFD up to SELinux
The above is the first "series"... I don't have much opinion about it,
though I do like the idea of making userfaultfd visible to the LSM.
> Let userfaultfd opt out of handling kernel-mode faults
> Add a new sysctl for limiting userfaultfd to user mode faults
Now this I'm very interested in. Can you go into more detail about two
things:
- What is the threat being solved? (I understand the threat, but detailing
it in the commit log is important for people who don't know it. Existing
commit cefdca0a86be517bc390fc4541e3674b8e7803b0 gets into some of the
details already, but I'd like to see reference to external sources like
https://duasynt.com/blog/linux-kernel-heap-spray)
- Why is this needed in addition to the existing vm.unprivileged_userfaultfd
sysctl? (And should this maybe just be another setting for that
sysctl, like "2"?)
As to the mechanics of the change, I'm not sure I like the idea of adding
a UAPI flag for this. Why not just retain the permission check done at
open() and if kernelmode faults aren't allowed, ignore them? This would
require no changes to existing programs and gains the desired defense.
(And, I think, the sysctl value could be bumped to "2" as that's a
better default state -- does qemu actually need kernelmode traps?)
Thanks again for the patches!
-Kees
>
> Documentation/admin-guide/sysctl/vm.rst | 13 ++++
> fs/anon_inodes.c | 89 +++++++++++++++++--------
> fs/userfaultfd.c | 29 ++++++--
> include/linux/anon_inodes.h | 27 ++++++--
> include/linux/lsm_hooks.h | 8 +++
> include/linux/security.h | 2 +
> include/linux/userfaultfd_k.h | 3 +
> include/uapi/linux/userfaultfd.h | 9 +++
> kernel/sysctl.c | 9 +++
> security/security.c | 8 +++
> security/selinux/hooks.c | 68 +++++++++++++++++++
> security/selinux/include/classmap.h | 2 +
> 12 files changed, 229 insertions(+), 38 deletions(-)
>
> --
> 2.25.0.225.g125e21ebc7-goog
>
--
Kees Cook
On 2/11/20 6:27 PM, Daniel Colascione wrote:
> On Tue, Feb 11, 2020 at 3:13 PM Casey Schaufler <[email protected]> wrote:
>>
>> On 2/11/2020 2:55 PM, Daniel Colascione wrote:
>>> 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 allows administrators to limit userfaultfd to
>>> servicing user-mode faults, increasing the difficulty of using
>>> userfaultfd in exploit chains invoking delaying kernel faults.
>>>
>>> A new anon_inodes interface allows callers to opt into SELinux
>>> management of anonymous file objects. In this mode, anon_inodes
>>> creates new ephemeral inodes for anonymous file objects instead of
>>> reusing a singleton dummy inode. A new LSM hook gives security modules
>>> an opportunity to configure and veto these ephemeral inodes.
>>>
>>> Existing anon_inodes users must opt into the new functionality.
>>>
>>> Daniel Colascione (6):
>>> Add a new flags-accepting interface for anonymous inodes
>>> Add a concept of a "secure" anonymous file
>>> Teach SELinux about a new userfaultfd class
>>> Wire UFFD up to SELinux
>>> Let userfaultfd opt out of handling kernel-mode faults
>>> Add a new sysctl for limiting userfaultfd to user mode faults
>>
>> This must be posted to the linux Security Module list
>> <[email protected]>
>
> Added. I thought selinux@ was sufficient.
scripts/get_maintainer.pl can be helpful in identifying relevant lists
and maintainers for each patch. I don't use its output blindly as it
tends to over-approximate but since your patches span the VFS, LSM
framework, and selinux, you do need to include relevant
maintainers/lists for each.
On Wed, Feb 12, 2020 at 8:51 AM Kees Cook <[email protected]> wrote:
> On Tue, Feb 11, 2020 at 02:55:41PM -0800, Daniel Colascione wrote:
> > Let userfaultfd opt out of handling kernel-mode faults
> > Add a new sysctl for limiting userfaultfd to user mode faults
>
> Now this I'm very interested in. Can you go into more detail about two
> things:
[...]
> - Why is this needed in addition to the existing vm.unprivileged_userfaultfd
> sysctl? (And should this maybe just be another setting for that
> sysctl, like "2"?)
>
> As to the mechanics of the change, I'm not sure I like the idea of adding
> a UAPI flag for this. Why not just retain the permission check done at
> open() and if kernelmode faults aren't allowed, ignore them? This would
> require no changes to existing programs and gains the desired defense.
> (And, I think, the sysctl value could be bumped to "2" as that's a
> better default state -- does qemu actually need kernelmode traps?)
I think this might be necessary for I/O emulation? As in, if before
getting migrated, the guest writes some data into a buffer, then the
guest gets migrated, and then while the postcopy migration stuff is
still running, the guest tells QEMU to write that data from
guest-physical memory to disk or whatever; I think in that case, QEMU
will do something like a pwrite() syscall where the userspace pointer
points into the memory area containing guest-physical memory, which
would return -EFAULT if userfaultfd was restricted to userspace
accesses.
This was described in this old presentation about why userfaultfd is
better than a SIGSEGV handler:
https://drive.google.com/file/d/0BzyAwvVlQckeSzlCSDFmRHVybzQ/view
(slide 6) (recording at https://youtu.be/pC8cWWRVSPw?t=463)
On 2/11/20 5:55 PM, Daniel Colascione wrote:
> Use the secure anonymous inode LSM hook we just added to let SELinux
> policy place restrictions on userfaultfd use. The create operation
> applies to processes creating new instances of these file objects;
> transfer between processes is covered by restrictions on read, write,
> and ioctl access already checked inside selinux_file_receive.
>
> Signed-off-by: Daniel Colascione <[email protected]>
(please add linux-fsdevel and viro to the cc for future versions of this
patch since it changes the VFS)
> ---
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1659b59fb5d7..e178f6f40e93 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2915,6 +2919,69 @@ 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 char *name,
> + const struct file_operations *fops)
> +{
> + const struct task_security_struct *tsec = selinux_cred(current_cred());
> + struct common_audit_data ad;
> + struct inode_security_struct *isec;
> +
> + if (unlikely(IS_PRIVATE(inode)))
> + return 0;
Seems like this is precluded by the caller and would be a bug? If
needed at all, take it to the security_inode_init_security_anon() so it
doesn't have to be repeated in each security module.
> +
> + /*
> + * We shouldn't be creating secure anonymous inodes before LSM
> + * initialization completes.
> + */
> + if (unlikely(!selinux_state.initialized))
> + return -EBUSY;
I don't think this is viable; any arbitrary actions are possible before
policy is loaded, and a Linux distro can be brought up fully with
SELinux enabled and no policy loaded. You'll just need to have a
default behavior prior to initialization.
> +
> + 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, so check that the state is as
> + * inode_alloc_security left it.
> + */
> + BUG_ON(isec->initialized != LABEL_INVALID);
> + BUG_ON(isec->sclass != SECCLASS_FILE);
I think the kernel discourages overuse of BUG_ON/BUG/...
> +
> +#ifdef CONFIG_USERFAULTFD
> + if (fops == &userfaultfd_fops)
> + isec->sclass = SECCLASS_UFFD;
> +#endif
Not sure we want or need to introduce a new security class for each user
of anonymous inodes since the permissions should be the same as for
file. Also not sure we want to be testing fops for each such case. We
were looking at possibly leveraging the name as a key and using
security_transition_sid() to generate a distinct SID/context/type for
the inode via type_transition rules in policy. We have some WIP along
those lines.
> +
> + if (isec->sclass == SECCLASS_FILE) {
> + printk(KERN_WARNING "refusing to create secure anonymous inode "
> + "of unknown type");
> + return -EOPNOTSUPP;
> + }
> + /*
> + * Always give secure anonymous inodes the sid of the
> + * creating task.
> + */
> +
> + isec->sid = tsec->sid;
This doesn't generalize for other users of anonymous inodes, e.g. the
/dev/kvm case where we'd rather inherit the SID and class from the
original /dev/kvm inode itself.
Thanks for taking a look and for the fast reply!
On Tue, Feb 11, 2020 at 11:51 PM Kees Cook <[email protected]> wrote:
>
> Hi!
>
> Firstly, thanks for working on this! It's been on my TODO list for a
> while. :)
>
> Casey already recommended including the LSM list to CC (since this is a
> new LSM -- there are many LSMs). Additionally, the series should
> probably be sent _to_ the userfaultfd maintainers:
> Andrea Arcangeli <[email protected]>
> Mike Rapoport <[email protected]>
> and I'd also CC a couple other people that have done recent work:
> Peter Xu <[email protected]>
> Jann Horn <[email protected]>
>
> More notes below...
In general, in the event that a patch series doesn't include all
needed parties on the to-line, what's the right way to fix the
situation without spamming everyone and forking the thread? In this
case, since I'm splitting the patch series anyway, I can just expand
the to-line in the reroll.
> On Tue, Feb 11, 2020 at 02:55:41PM -0800, Daniel Colascione wrote:
> > 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 allows administrators to limit userfaultfd to
> > servicing user-mode faults, increasing the difficulty of using
> > userfaultfd in exploit chains invoking delaying kernel faults.
>
> I actually think these are two very different goals and likely the
> series could be split into two for them. One is LSM hooking of
> userfaultfd and the SELinux attachment, and the other is the user-mode
> fault restrictions. And they would likely go via separate trees (LSM
> through James's LSM tree, and probably akpm's -mm tree for the sysctl).
>
> > A new anon_inodes interface allows callers to opt into SELinux
> > management of anonymous file objects. In this mode, anon_inodes
> > creates new ephemeral inodes for anonymous file objects instead of
> > reusing a singleton dummy inode. A new LSM hook gives security modules
> > an opportunity to configure and veto these ephemeral inodes.
> >
> > Existing anon_inodes users must opt into the new functionality.
> >
> > Daniel Colascione (6):
> > Add a new flags-accepting interface for anonymous inodes
> > Add a concept of a "secure" anonymous file
> > Teach SELinux about a new userfaultfd class
> > Wire UFFD up to SELinux
>
> The above is the first "series"... I don't have much opinion about it,
> though I do like the idea of making userfaultfd visible to the LSM.
Yeah. The interesting part there is the anon_inodes API change. I'll
split that half of the series out.
> > Let userfaultfd opt out of handling kernel-mode faults
> > Add a new sysctl for limiting userfaultfd to user mode faults
>
> Now this I'm very interested in. Can you go into more detail about two
> things:
>
> - What is the threat being solved? (I understand the threat, but detailing
> it in the commit log is important for people who don't know it. Existing
> commit cefdca0a86be517bc390fc4541e3674b8e7803b0 gets into some of the
> details already, but I'd like to see reference to external sources like
> https://duasynt.com/blog/linux-kernel-heap-spray)
Sure. I can add a reference to that and a more general discussion of
how delaying kernel fault handling can broaden race windows for other
exploits.
> - Why is this needed in addition to the existing vm.unprivileged_userfaultfd
> sysctl? (And should this maybe just be another setting for that
> sysctl, like "2"?)
We want to use UFFD for a new garbage collector in Android, and we
want unprivileged processes to be able to use this new garbage
collector. Giving them CAP_SYS_PTRACE is dangerous.
> As to the mechanics of the change, I'm not sure I like the idea of adding
> a UAPI flag for this. Why not just retain the permission check done at
> open() and if kernelmode faults aren't allowed, ignore them? This would
> require no changes to existing programs and gains the desired defense.
As Jann mentions below, it's a matter of the kernel's contractual
obligation to userspace. Right now, userfaultfd(2) either succeeds or
it fails with one of the enumerated error codes. You're proposing
having the userfaultfd(2) succeed but return a file descriptor that
doesn't do the job the kernel promised it would do. If we were to
adopt your proposal, an application would see UFFD succeed, then see
unexpeced EFAULTs from system calls, which would probably cause the
application to malfunctioning in exciting ways. An explicit "sorry, I
can't do that" error code is better: an application that gets an error
code from userfaultfd(2) can fall back to something else, but an
application that silently gets an underfeatured UFFD doesn't know
anything is wrong until it's too late. The additional flag preserves
the UFFD contract and gives applications a way to probe for feature
support.
> (And, I think, the sysctl value could be bumped to "2" as that's a
> better default state -- does qemu actually need kernelmode traps?)
I prefer a default of one for regular systems because I don't want to
make experimentation with novel ways to use UFFD more difficult, and a
default of two would require users go out of their way to handle user
faults, and few will. For a more constrained system like Android, two
is fine.
On Wed, Feb 12, 2020 at 05:54:35PM +0100, Jann Horn wrote:
> On Wed, Feb 12, 2020 at 8:51 AM Kees Cook <[email protected]> wrote:
> > On Tue, Feb 11, 2020 at 02:55:41PM -0800, Daniel Colascione wrote:
> > > Let userfaultfd opt out of handling kernel-mode faults
> > > Add a new sysctl for limiting userfaultfd to user mode faults
> >
> > Now this I'm very interested in. Can you go into more detail about two
> > things:
> [...]
> > - Why is this needed in addition to the existing vm.unprivileged_userfaultfd
> > sysctl? (And should this maybe just be another setting for that
> > sysctl, like "2"?)
> >
> > As to the mechanics of the change, I'm not sure I like the idea of adding
> > a UAPI flag for this. Why not just retain the permission check done at
> > open() and if kernelmode faults aren't allowed, ignore them? This would
> > require no changes to existing programs and gains the desired defense.
> > (And, I think, the sysctl value could be bumped to "2" as that's a
> > better default state -- does qemu actually need kernelmode traps?)
>
> I think this might be necessary for I/O emulation? As in, if before
> getting migrated, the guest writes some data into a buffer, then the
> guest gets migrated, and then while the postcopy migration stuff is
> still running, the guest tells QEMU to write that data from
> guest-physical memory to disk or whatever; I think in that case, QEMU
> will do something like a pwrite() syscall where the userspace pointer
> points into the memory area containing guest-physical memory, which
> would return -EFAULT if userfaultfd was restricted to userspace
> accesses.
>
> This was described in this old presentation about why userfaultfd is
> better than a SIGSEGV handler:
> https://drive.google.com/file/d/0BzyAwvVlQckeSzlCSDFmRHVybzQ/view
> (slide 6) (recording at https://youtu.be/pC8cWWRVSPw?t=463)
Right. AFAICT QEMU uses it far more than disk IOs. A guest page can
be accessed by any kernel component on the destination host during a
postcopy procedure. It can be as simple as when a vcpu writes to a
missing guest page which still resides on the source host, then KVM
will get a page fault and trap into userfaultfd asking for that page.
The same thing happens to other modules like vhost, etc., as long as a
missing guest page is touched by a kernel module.
Thanks,
--
Peter Xu
Thanks for taking a look.
On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <[email protected]> wrote:
>
> On 2/11/20 5:55 PM, Daniel Colascione wrote:
> > Use the secure anonymous inode LSM hook we just added to let SELinux
> > policy place restrictions on userfaultfd use. The create operation
> > applies to processes creating new instances of these file objects;
> > transfer between processes is covered by restrictions on read, write,
> > and ioctl access already checked inside selinux_file_receive.
> >
> > Signed-off-by: Daniel Colascione <[email protected]>
>
> (please add linux-fsdevel and viro to the cc for future versions of this
> patch since it changes the VFS)
>
> > ---
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 1659b59fb5d7..e178f6f40e93 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2915,6 +2919,69 @@ 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 char *name,
> > + const struct file_operations *fops)
> > +{
> > + const struct task_security_struct *tsec = selinux_cred(current_cred());
> > + struct common_audit_data ad;
> > + struct inode_security_struct *isec;
> > +
> > + if (unlikely(IS_PRIVATE(inode)))
> > + return 0;
>
> Seems like this is precluded by the caller and would be a bug? If
> needed at all, take it to the security_inode_init_security_anon() so it
> doesn't have to be repeated in each security module.
>
> > +
> > + /*
> > + * We shouldn't be creating secure anonymous inodes before LSM
> > + * initialization completes.
> > + */
> > + if (unlikely(!selinux_state.initialized))
> > + return -EBUSY;
>
> I don't think this is viable; any arbitrary actions are possible before
> policy is loaded, and a Linux distro can be brought up fully with
> SELinux enabled and no policy loaded. You'll just need to have a
> default behavior prior to initialization.
We'd have to fail open then, I think, and return an S_PRIVATE inode
(the regular anon inode).
> > +
> > + 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, so check that the state is as
> > + * inode_alloc_security left it.
> > + */
> > + BUG_ON(isec->initialized != LABEL_INVALID);
> > + BUG_ON(isec->sclass != SECCLASS_FILE);
>
> I think the kernel discourages overuse of BUG_ON/BUG/...
I'm not sure what counts as overuse.
> > +
> > +#ifdef CONFIG_USERFAULTFD
> > + if (fops == &userfaultfd_fops)
> > + isec->sclass = SECCLASS_UFFD;
> > +#endif
>
> Not sure we want or need to introduce a new security class for each user
> of anonymous inodes since the permissions should be the same as for
> file.
The purpose of this change is to apply special policy to userfaultfd
FDs in particular. Isn't having a UFFD security class the best way to
go about that? (There's no path.) Am I missing something?
> Also not sure we want to be testing fops for each such case.
I was also thinking of just providing some kind of context string
(maybe the name), which might be friendlier to modules, but the loose
coupling kind of scares me, and for this particular application, since
UFFD is always in the core and never in a module, checking the fops
seems a bit more robust and doesn't hurt anything.
> We
> were looking at possibly leveraging the name as a key and using
> security_transition_sid() to generate a distinct SID/context/type for
> the inode via type_transition rules in policy. We have some WIP along
> those lines.
Where? Any chance it would be ready soon? I'd rather not hold up this
work for a more general mechanism.
> > +
> > + if (isec->sclass == SECCLASS_FILE) {
> > + printk(KERN_WARNING "refusing to create secure anonymous inode "
> > + "of unknown type");
> > + return -EOPNOTSUPP;
> > + }
> > + /*
> > + * Always give secure anonymous inodes the sid of the
> > + * creating task.
> > + */
> > +
> > + isec->sid = tsec->sid;
>
> This doesn't generalize for other users of anonymous inodes, e.g. the
> /dev/kvm case where we'd rather inherit the SID and class from the
> original /dev/kvm inode itself.
I think someone mentioned on the first version of this patch that we
could make it more flexible if the need arose. If we do want to do it
now, we could have the anon_inode security hook accept a "parent" or
"context" inode that modules could inspect for the purposes of forming
the new inode's SID. Does that make sense to you?
On 2/12/20 12:19 PM, Daniel Colascione wrote:
> Thanks for taking a look.
>
> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <[email protected]> wrote:
>>
>> On 2/11/20 5:55 PM, Daniel Colascione wrote:
>>> Use the secure anonymous inode LSM hook we just added to let SELinux
>>> policy place restrictions on userfaultfd use. The create operation
>>> applies to processes creating new instances of these file objects;
>>> transfer between processes is covered by restrictions on read, write,
>>> and ioctl access already checked inside selinux_file_receive.
>>>
>>> Signed-off-by: Daniel Colascione <[email protected]>
>>
>> (please add linux-fsdevel and viro to the cc for future versions of this
>> patch since it changes the VFS)
>>
>>> ---
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 1659b59fb5d7..e178f6f40e93 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>>> +
>>> + /*
>>> + * We shouldn't be creating secure anonymous inodes before LSM
>>> + * initialization completes.
>>> + */
>>> + if (unlikely(!selinux_state.initialized))
>>> + return -EBUSY;
>>
>> I don't think this is viable; any arbitrary actions are possible before
>> policy is loaded, and a Linux distro can be brought up fully with
>> SELinux enabled and no policy loaded. You'll just need to have a
>> default behavior prior to initialization.
>
> We'd have to fail open then, I think, and return an S_PRIVATE inode
> (the regular anon inode).
Not sure why. You aren't doing anything in the hook that actually
relies on selinux_state.initialized being set (i.e. nothing requires a
policy). The avc_has_perm() call will just succeed until a policy is
loaded. So if these inodes are created prior to policy load, they will
get assigned the task SID (which would be the kernel SID prior to policy
load or first exec or write to /proc/self/attr/current afterward) and
UFFD class (in your current code), be permitted, and then once policy is
loaded any further access will get checked against the kernel SID.
>>> + /*
>>> + * We only get here once per ephemeral inode. The inode has
>>> + * been initialized via inode_alloc_security but is otherwise
>>> + * untouched, so check that the state is as
>>> + * inode_alloc_security left it.
>>> + */
>>> + BUG_ON(isec->initialized != LABEL_INVALID);
>>> + BUG_ON(isec->sclass != SECCLASS_FILE);
>>
>> I think the kernel discourages overuse of BUG_ON/BUG/...
>
> I'm not sure what counts as overuse.
Me either (not my rule) but I'm pretty sure this counts or you'd see a
lot more of these kinds of BUG_ON() checks throughout. Try to reserve
them for really critical cases.
>>> +
>>> +#ifdef CONFIG_USERFAULTFD
>>> + if (fops == &userfaultfd_fops)
>>> + isec->sclass = SECCLASS_UFFD;
>>> +#endif
>>
>> Not sure we want or need to introduce a new security class for each user
>> of anonymous inodes since the permissions should be the same as for
>> file.
>
> The purpose of this change is to apply special policy to userfaultfd
> FDs in particular. Isn't having a UFFD security class the best way to
> go about that? (There's no path.) Am I missing something?
It is probably the simplest approach; it just doesn't generalize to all
users of anonymous inodes. We can distinguish them in one of two ways:
use a different class like you did (requires a code change every time we
add a new one and yet another duplicate of the file class) or use a
different SID/context/type. The latter could be achieved by calling
security_transition_sid() with the provided name wrapped in a qstr and
specifying type_transition rules on the name. Then policy could define
derived types for each domain, ala
type_transition init self:file "[userfaultfd]" init_userfaultfd;
type_transition untrusted_app self:file "[userfaultfd]"
untrusted_app_userfaultfd;
...
>> Also not sure we want to be testing fops for each such case.
>
> I was also thinking of just providing some kind of context string
> (maybe the name), which might be friendlier to modules, but the loose
> coupling kind of scares me, and for this particular application, since
> UFFD is always in the core and never in a module, checking the fops
> seems a bit more robust and doesn't hurt anything.
Yes, not sure how the vfs folks feel about either coupling (the
name-based one or the fops-based one). Neither seems great.
>> We
>> were looking at possibly leveraging the name as a key and using
>> security_transition_sid() to generate a distinct SID/context/type for
>> the inode via type_transition rules in policy. We have some WIP along
>> those lines.
>
> Where? Any chance it would be ready soon? I'd rather not hold up this
> work for a more general mechanism.
Hopefully will have a patch available soon. But not saying this
necessarily has to wait either.
>>> + /*
>>> + * Always give secure anonymous inodes the sid of the
>>> + * creating task.
>>> + */
>>> +
>>> + isec->sid = tsec->sid;
>>
>> This doesn't generalize for other users of anonymous inodes, e.g. the
>> /dev/kvm case where we'd rather inherit the SID and class from the
>> original /dev/kvm inode itself.
>
> I think someone mentioned on the first version of this patch that we
> could make it more flexible if the need arose. If we do want to do it
> now, we could have the anon_inode security hook accept a "parent" or
> "context" inode that modules could inspect for the purposes of forming
> the new inode's SID. Does that make sense to you?
Yes, that's the approach in our current WIP, except we call it a
"related" inode since it isn't necessarily connected to the anon inode
in any vfs sense.
On 2/12/20 1:04 PM, Stephen Smalley wrote:
> On 2/12/20 12:19 PM, Daniel Colascione wrote:
>> Thanks for taking a look.
>>
>> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <[email protected]>
>> wrote:
>>>
>>> On 2/11/20 5:55 PM, Daniel Colascione wrote:
>>>> Use the secure anonymous inode LSM hook we just added to let SELinux
>>>> policy place restrictions on userfaultfd use. The create operation
>>>> applies to processes creating new instances of these file objects;
>>>> transfer between processes is covered by restrictions on read, write,
>>>> and ioctl access already checked inside selinux_file_receive.
>>>>
>>>> Signed-off-by: Daniel Colascione <[email protected]>
>>>
>>> (please add linux-fsdevel and viro to the cc for future versions of this
>>> patch since it changes the VFS)
>>>
>>>> ---
>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>> index 1659b59fb5d7..e178f6f40e93 100644
>>>> --- a/security/selinux/hooks.c
>>>> +++ b/security/selinux/hooks.c
>>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct
>>>> inode *inode, struct inode *dir,
>>>> +
>>>> +Â Â Â Â /*
>>>> +Â Â Â Â Â * We shouldn't be creating secure anonymous inodes before LSM
>>>> +Â Â Â Â Â * initialization completes.
>>>> +Â Â Â Â Â */
>>>> +Â Â Â Â if (unlikely(!selinux_state.initialized))
>>>> +Â Â Â Â Â Â Â Â Â Â Â Â return -EBUSY;
>>>
>>> I don't think this is viable; any arbitrary actions are possible before
>>> policy is loaded, and a Linux distro can be brought up fully with
>>> SELinux enabled and no policy loaded. You'll just need to have a
>>> default behavior prior to initialization.
>>
>> We'd have to fail open then, I think, and return an S_PRIVATE inode
>> (the regular anon inode).
>
> Not sure why. You aren't doing anything in the hook that actually
> relies on selinux_state.initialized being set (i.e. nothing requires a
> policy). The avc_has_perm() call will just succeed until a policy is
> loaded. So if these inodes are created prior to policy load, they will
> get assigned the task SID (which would be the kernel SID prior to policy
> load or first exec or write to /proc/self/attr/current afterward) and
> UFFD class (in your current code), be permitted, and then once policy is
> loaded any further access will get checked against the kernel SID.
>
>>>> +Â Â Â Â /*
>>>> +     * We only get here once per ephemeral inode. The inode has
>>>> +Â Â Â Â Â * been initialized via inode_alloc_security but is otherwise
>>>> +Â Â Â Â Â * untouched, so check that the state is as
>>>> +Â Â Â Â Â * inode_alloc_security left it.
>>>> +Â Â Â Â Â */
>>>> +Â Â Â Â BUG_ON(isec->initialized != LABEL_INVALID);
>>>> +Â Â Â Â BUG_ON(isec->sclass != SECCLASS_FILE);
>>>
>>> I think the kernel discourages overuse of BUG_ON/BUG/...
>>
>> I'm not sure what counts as overuse.
>
> Me either (not my rule) but I'm pretty sure this counts or you'd see a
> lot more of these kinds of BUG_ON() checks throughout. Try to reserve
> them for really critical cases.
>
>>>> +
>>>> +#ifdef CONFIG_USERFAULTFD
>>>> +Â Â Â Â if (fops == &userfaultfd_fops)
>>>> +Â Â Â Â Â Â Â Â Â Â Â Â isec->sclass = SECCLASS_UFFD;
>>>> +#endif
>>>
>>> Not sure we want or need to introduce a new security class for each user
>>> of anonymous inodes since the permissions should be the same as for
>>> file.
>>
>> The purpose of this change is to apply special policy to userfaultfd
>> FDs in particular. Isn't having a UFFD security class the best way to
>> go about that? (There's no path.) Am I missing something?
>
> It is probably the simplest approach; it just doesn't generalize to all
> users of anonymous inodes. We can distinguish them in one of two ways:
> use a different class like you did (requires a code change every time we
> add a new one and yet another duplicate of the file class) or use a
> different SID/context/type. The latter could be achieved by calling
> security_transition_sid() with the provided name wrapped in a qstr and
> specifying type_transition rules on the name. Then policy could define
> derived types for each domain, ala
> type_transition init self:file "[userfaultfd]" init_userfaultfd;
> type_transition untrusted_app self:file "[userfaultfd]"
> untrusted_app_userfaultfd;
> ...
>
>>> Also not sure we want to be testing fops for each such case.
>>
>> I was also thinking of just providing some kind of context string
>> (maybe the name), which might be friendlier to modules, but the loose
>> coupling kind of scares me, and for this particular application, since
>> UFFD is always in the core and never in a module, checking the fops
>> seems a bit more robust and doesn't hurt anything.
>
> Yes, not sure how the vfs folks feel about either coupling (the
> name-based one or the fops-based one). Neither seems great.
>
>>> We
>>> were looking at possibly leveraging the name as a key and using
>>> security_transition_sid() to generate a distinct SID/context/type for
>>> the inode via type_transition rules in policy. We have some WIP along
>>> those lines.
>>
>> Where? Any chance it would be ready soon? I'd rather not hold up this
>> work for a more general mechanism.
>
> Hopefully will have a patch available soon. But not saying this
> necessarily has to wait either.
>
>>>> +Â Â Â Â /*
>>>> +Â Â Â Â Â * Always give secure anonymous inodes the sid of the
>>>> +Â Â Â Â Â * creating task.
>>>> +Â Â Â Â Â */
>>>> +
>>>> +Â Â Â Â isec->sid = tsec->sid;
>>>
>>> This doesn't generalize for other users of anonymous inodes, e.g. the
>>> /dev/kvm case where we'd rather inherit the SID and class from the
>>> original /dev/kvm inode itself.
>>
>> I think someone mentioned on the first version of this patch that we
>> could make it more flexible if the need arose. If we do want to do it
>> now, we could have the anon_inode security hook accept a "parent" or
>> "context" inode that modules could inspect for the purposes of forming
>> the new inode's SID. Does that make sense to you?
>
> Yes, that's the approach in our current WIP, except we call it a
> "related" inode since it isn't necessarily connected to the anon inode
> in any vfs sense.
The other key difference in our WIP approach is that we assumed that we
couldn't mandate allocating a separate anon inode for each of these fds
and we wanted to cover all anonymous inodes (not opt-in), so we are
storing the SID/class pair as additional fields in the
file_security_struct and have modified file_has_perm() and others to
look there for anonymous inodes.
On Wed, Feb 12, 2020 at 10:59 AM Stephen Smalley <[email protected]> wrote:
>
> On 2/12/20 1:04 PM, Stephen Smalley wrote:
> > On 2/12/20 12:19 PM, Daniel Colascione wrote:
> >> Thanks for taking a look.
> >>
> >> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <[email protected]>
> >> wrote:
> >>>
> >>> On 2/11/20 5:55 PM, Daniel Colascione wrote:
> >>>> Use the secure anonymous inode LSM hook we just added to let SELinux
> >>>> policy place restrictions on userfaultfd use. The create operation
> >>>> applies to processes creating new instances of these file objects;
> >>>> transfer between processes is covered by restrictions on read, write,
> >>>> and ioctl access already checked inside selinux_file_receive.
> >>>>
> >>>> Signed-off-by: Daniel Colascione <[email protected]>
> >>>
> >>> (please add linux-fsdevel and viro to the cc for future versions of this
> >>> patch since it changes the VFS)
> >>>
> >>>> ---
> >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>>> index 1659b59fb5d7..e178f6f40e93 100644
> >>>> --- a/security/selinux/hooks.c
> >>>> +++ b/security/selinux/hooks.c
> >>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct
> >>>> inode *inode, struct inode *dir,
> >>>> +
> >>>> + /*
> >>>> + * We shouldn't be creating secure anonymous inodes before LSM
> >>>> + * initialization completes.
> >>>> + */
> >>>> + if (unlikely(!selinux_state.initialized))
> >>>> + return -EBUSY;
> >>>
> >>> I don't think this is viable; any arbitrary actions are possible before
> >>> policy is loaded, and a Linux distro can be brought up fully with
> >>> SELinux enabled and no policy loaded. You'll just need to have a
> >>> default behavior prior to initialization.
> >>
> >> We'd have to fail open then, I think, and return an S_PRIVATE inode
> >> (the regular anon inode).
> >
> > Not sure why. You aren't doing anything in the hook that actually
> > relies on selinux_state.initialized being set (i.e. nothing requires a
> > policy). The avc_has_perm() call will just succeed until a policy is
> > loaded. So if these inodes are created prior to policy load, they will
> > get assigned the task SID (which would be the kernel SID prior to policy
> > load or first exec or write to /proc/self/attr/current afterward) and
> > UFFD class (in your current code), be permitted, and then once policy is
> > loaded any further access will get checked against the kernel SID.
> >
> >>>> + /*
> >>>> + * We only get here once per ephemeral inode. The inode has
> >>>> + * been initialized via inode_alloc_security but is otherwise
> >>>> + * untouched, so check that the state is as
> >>>> + * inode_alloc_security left it.
> >>>> + */
> >>>> + BUG_ON(isec->initialized != LABEL_INVALID);
> >>>> + BUG_ON(isec->sclass != SECCLASS_FILE);
> >>>
> >>> I think the kernel discourages overuse of BUG_ON/BUG/...
> >>
> >> I'm not sure what counts as overuse.
> >
> > Me either (not my rule) but I'm pretty sure this counts or you'd see a
> > lot more of these kinds of BUG_ON() checks throughout. Try to reserve
> > them for really critical cases.
> >
> >>>> +
> >>>> +#ifdef CONFIG_USERFAULTFD
> >>>> + if (fops == &userfaultfd_fops)
> >>>> + isec->sclass = SECCLASS_UFFD;
> >>>> +#endif
> >>>
> >>> Not sure we want or need to introduce a new security class for each user
> >>> of anonymous inodes since the permissions should be the same as for
> >>> file.
> >>
> >> The purpose of this change is to apply special policy to userfaultfd
> >> FDs in particular. Isn't having a UFFD security class the best way to
> >> go about that? (There's no path.) Am I missing something?
> >
> > It is probably the simplest approach; it just doesn't generalize to all
> > users of anonymous inodes. We can distinguish them in one of two ways:
> > use a different class like you did (requires a code change every time we
> > add a new one and yet another duplicate of the file class) or use a
> > different SID/context/type. The latter could be achieved by calling
> > security_transition_sid() with the provided name wrapped in a qstr and
> > specifying type_transition rules on the name. Then policy could define
> > derived types for each domain, ala
> > type_transition init self:file "[userfaultfd]" init_userfaultfd;
> > type_transition untrusted_app self:file "[userfaultfd]"
> > untrusted_app_userfaultfd;
> > ...
> >
> >>> Also not sure we want to be testing fops for each such case.
> >>
> >> I was also thinking of just providing some kind of context string
> >> (maybe the name), which might be friendlier to modules, but the loose
> >> coupling kind of scares me, and for this particular application, since
> >> UFFD is always in the core and never in a module, checking the fops
> >> seems a bit more robust and doesn't hurt anything.
> >
> > Yes, not sure how the vfs folks feel about either coupling (the
> > name-based one or the fops-based one). Neither seems great.
> >
> >>> We
> >>> were looking at possibly leveraging the name as a key and using
> >>> security_transition_sid() to generate a distinct SID/context/type for
> >>> the inode via type_transition rules in policy. We have some WIP along
> >>> those lines.
> >>
> >> Where? Any chance it would be ready soon? I'd rather not hold up this
> >> work for a more general mechanism.
> >
> > Hopefully will have a patch available soon. But not saying this
> > necessarily has to wait either.
> >
> >>>> + /*
> >>>> + * Always give secure anonymous inodes the sid of the
> >>>> + * creating task.
> >>>> + */
> >>>> +
> >>>> + isec->sid = tsec->sid;
> >>>
> >>> This doesn't generalize for other users of anonymous inodes, e.g. the
> >>> /dev/kvm case where we'd rather inherit the SID and class from the
> >>> original /dev/kvm inode itself.
> >>
> >> I think someone mentioned on the first version of this patch that we
> >> could make it more flexible if the need arose. If we do want to do it
> >> now, we could have the anon_inode security hook accept a "parent" or
> >> "context" inode that modules could inspect for the purposes of forming
> >> the new inode's SID. Does that make sense to you?
> >
> > Yes, that's the approach in our current WIP, except we call it a
> > "related" inode since it isn't necessarily connected to the anon inode
> > in any vfs sense.
>
> The other key difference in our WIP approach is that we assumed that we
> couldn't mandate allocating a separate anon inode for each of these fds
> and we wanted to cover all anonymous inodes (not opt-in), so we are
> storing the SID/class pair as additional fields in the
> file_security_struct and have modified file_has_perm() and others to
> look there for anonymous inodes.
A separate inode seems like the simpler approach for now, because it
means that we have fewer places to check for security information ---
and it's not as if an inode is particularly expensive. We can always
switch later.
On 2/12/20 2:04 PM, Daniel Colascione wrote:
> On Wed, Feb 12, 2020 at 10:59 AM Stephen Smalley <[email protected]> wrote:
>>
>> On 2/12/20 1:04 PM, Stephen Smalley wrote:
>>> On 2/12/20 12:19 PM, Daniel Colascione wrote:
>>>> Thanks for taking a look.
>>>>
>>>> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <[email protected]>
>>>> wrote:
>>>>>
>>>>> On 2/11/20 5:55 PM, Daniel Colascione wrote:
>>>>>> Use the secure anonymous inode LSM hook we just added to let SELinux
>>>>>> policy place restrictions on userfaultfd use. The create operation
>>>>>> applies to processes creating new instances of these file objects;
>>>>>> transfer between processes is covered by restrictions on read, write,
>>>>>> and ioctl access already checked inside selinux_file_receive.
>>>>>>
>>>>>> Signed-off-by: Daniel Colascione <[email protected]>
>>>>>
>>>>> (please add linux-fsdevel and viro to the cc for future versions of this
>>>>> patch since it changes the VFS)
>>>>>
>>>>>> ---
>>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>>> index 1659b59fb5d7..e178f6f40e93 100644
>>>>>> --- a/security/selinux/hooks.c
>>>>>> +++ b/security/selinux/hooks.c
>>>>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct
>>>>>> inode *inode, struct inode *dir,
>>>>>> +
>>>>>> + /*
>>>>>> + * We shouldn't be creating secure anonymous inodes before LSM
>>>>>> + * initialization completes.
>>>>>> + */
>>>>>> + if (unlikely(!selinux_state.initialized))
>>>>>> + return -EBUSY;
>>>>>
>>>>> I don't think this is viable; any arbitrary actions are possible before
>>>>> policy is loaded, and a Linux distro can be brought up fully with
>>>>> SELinux enabled and no policy loaded. You'll just need to have a
>>>>> default behavior prior to initialization.
>>>>
>>>> We'd have to fail open then, I think, and return an S_PRIVATE inode
>>>> (the regular anon inode).
>>>
>>> Not sure why. You aren't doing anything in the hook that actually
>>> relies on selinux_state.initialized being set (i.e. nothing requires a
>>> policy). The avc_has_perm() call will just succeed until a policy is
>>> loaded. So if these inodes are created prior to policy load, they will
>>> get assigned the task SID (which would be the kernel SID prior to policy
>>> load or first exec or write to /proc/self/attr/current afterward) and
>>> UFFD class (in your current code), be permitted, and then once policy is
>>> loaded any further access will get checked against the kernel SID.
>>>
>>>>>> + /*
>>>>>> + * We only get here once per ephemeral inode. The inode has
>>>>>> + * been initialized via inode_alloc_security but is otherwise
>>>>>> + * untouched, so check that the state is as
>>>>>> + * inode_alloc_security left it.
>>>>>> + */
>>>>>> + BUG_ON(isec->initialized != LABEL_INVALID);
>>>>>> + BUG_ON(isec->sclass != SECCLASS_FILE);
>>>>>
>>>>> I think the kernel discourages overuse of BUG_ON/BUG/...
>>>>
>>>> I'm not sure what counts as overuse.
>>>
>>> Me either (not my rule) but I'm pretty sure this counts or you'd see a
>>> lot more of these kinds of BUG_ON() checks throughout. Try to reserve
>>> them for really critical cases.
>>>
>>>>>> +
>>>>>> +#ifdef CONFIG_USERFAULTFD
>>>>>> + if (fops == &userfaultfd_fops)
>>>>>> + isec->sclass = SECCLASS_UFFD;
>>>>>> +#endif
>>>>>
>>>>> Not sure we want or need to introduce a new security class for each user
>>>>> of anonymous inodes since the permissions should be the same as for
>>>>> file.
>>>>
>>>> The purpose of this change is to apply special policy to userfaultfd
>>>> FDs in particular. Isn't having a UFFD security class the best way to
>>>> go about that? (There's no path.) Am I missing something?
>>>
>>> It is probably the simplest approach; it just doesn't generalize to all
>>> users of anonymous inodes. We can distinguish them in one of two ways:
>>> use a different class like you did (requires a code change every time we
>>> add a new one and yet another duplicate of the file class) or use a
>>> different SID/context/type. The latter could be achieved by calling
>>> security_transition_sid() with the provided name wrapped in a qstr and
>>> specifying type_transition rules on the name. Then policy could define
>>> derived types for each domain, ala
>>> type_transition init self:file "[userfaultfd]" init_userfaultfd;
>>> type_transition untrusted_app self:file "[userfaultfd]"
>>> untrusted_app_userfaultfd;
>>> ...
>>>
>>>>> Also not sure we want to be testing fops for each such case.
>>>>
>>>> I was also thinking of just providing some kind of context string
>>>> (maybe the name), which might be friendlier to modules, but the loose
>>>> coupling kind of scares me, and for this particular application, since
>>>> UFFD is always in the core and never in a module, checking the fops
>>>> seems a bit more robust and doesn't hurt anything.
>>>
>>> Yes, not sure how the vfs folks feel about either coupling (the
>>> name-based one or the fops-based one). Neither seems great.
>>>
>>>>> We
>>>>> were looking at possibly leveraging the name as a key and using
>>>>> security_transition_sid() to generate a distinct SID/context/type for
>>>>> the inode via type_transition rules in policy. We have some WIP along
>>>>> those lines.
>>>>
>>>> Where? Any chance it would be ready soon? I'd rather not hold up this
>>>> work for a more general mechanism.
>>>
>>> Hopefully will have a patch available soon. But not saying this
>>> necessarily has to wait either.
>>>
>>>>>> + /*
>>>>>> + * Always give secure anonymous inodes the sid of the
>>>>>> + * creating task.
>>>>>> + */
>>>>>> +
>>>>>> + isec->sid = tsec->sid;
>>>>>
>>>>> This doesn't generalize for other users of anonymous inodes, e.g. the
>>>>> /dev/kvm case where we'd rather inherit the SID and class from the
>>>>> original /dev/kvm inode itself.
>>>>
>>>> I think someone mentioned on the first version of this patch that we
>>>> could make it more flexible if the need arose. If we do want to do it
>>>> now, we could have the anon_inode security hook accept a "parent" or
>>>> "context" inode that modules could inspect for the purposes of forming
>>>> the new inode's SID. Does that make sense to you?
>>>
>>> Yes, that's the approach in our current WIP, except we call it a
>>> "related" inode since it isn't necessarily connected to the anon inode
>>> in any vfs sense.
>>
>> The other key difference in our WIP approach is that we assumed that we
>> couldn't mandate allocating a separate anon inode for each of these fds
>> and we wanted to cover all anonymous inodes (not opt-in), so we are
>> storing the SID/class pair as additional fields in the
>> file_security_struct and have modified file_has_perm() and others to
>> look there for anonymous inodes.
>
> A separate inode seems like the simpler approach for now, because it
> means that we have fewer places to check for security information ---
> and it's not as if an inode is particularly expensive. We can always
> switch later.
We'd prefer having a separate inode if possible but didn't think that
would fly with the vfs folks, especially if we try to apply this to all
anonymous inodes. It might be ok for userfaultfd usage as a specific
case but there is a reason why anonymous inodes were introduced and
creating a separate inode each time defeats that purpose IIUC. It will
be interesting to see how they respond.
On Wed, Feb 12, 2020 at 11:10 AM Stephen Smalley <[email protected]> wrote:
>
> On 2/12/20 2:04 PM, Daniel Colascione wrote:
> > On Wed, Feb 12, 2020 at 10:59 AM Stephen Smalley <[email protected]> wrote:
> >>
> >> On 2/12/20 1:04 PM, Stephen Smalley wrote:
> >>> On 2/12/20 12:19 PM, Daniel Colascione wrote:
> >>>> Thanks for taking a look.
> >>>>
> >>>> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <[email protected]>
> >>>> wrote:
> >>>>>
> >>>>> On 2/11/20 5:55 PM, Daniel Colascione wrote:
> >>>>>> Use the secure anonymous inode LSM hook we just added to let SELinux
> >>>>>> policy place restrictions on userfaultfd use. The create operation
> >>>>>> applies to processes creating new instances of these file objects;
> >>>>>> transfer between processes is covered by restrictions on read, write,
> >>>>>> and ioctl access already checked inside selinux_file_receive.
> >>>>>>
> >>>>>> Signed-off-by: Daniel Colascione <[email protected]>
> >>>>>
> >>>>> (please add linux-fsdevel and viro to the cc for future versions of this
> >>>>> patch since it changes the VFS)
> >>>>>
> >>>>>> ---
> >>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>>>>> index 1659b59fb5d7..e178f6f40e93 100644
> >>>>>> --- a/security/selinux/hooks.c
> >>>>>> +++ b/security/selinux/hooks.c
> >>>>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct
> >>>>>> inode *inode, struct inode *dir,
> >>>>>> +
> >>>>>> + /*
> >>>>>> + * We shouldn't be creating secure anonymous inodes before LSM
> >>>>>> + * initialization completes.
> >>>>>> + */
> >>>>>> + if (unlikely(!selinux_state.initialized))
> >>>>>> + return -EBUSY;
> >>>>>
> >>>>> I don't think this is viable; any arbitrary actions are possible before
> >>>>> policy is loaded, and a Linux distro can be brought up fully with
> >>>>> SELinux enabled and no policy loaded. You'll just need to have a
> >>>>> default behavior prior to initialization.
> >>>>
> >>>> We'd have to fail open then, I think, and return an S_PRIVATE inode
> >>>> (the regular anon inode).
> >>>
> >>> Not sure why. You aren't doing anything in the hook that actually
> >>> relies on selinux_state.initialized being set (i.e. nothing requires a
> >>> policy). The avc_has_perm() call will just succeed until a policy is
> >>> loaded. So if these inodes are created prior to policy load, they will
> >>> get assigned the task SID (which would be the kernel SID prior to policy
> >>> load or first exec or write to /proc/self/attr/current afterward) and
> >>> UFFD class (in your current code), be permitted, and then once policy is
> >>> loaded any further access will get checked against the kernel SID.
> >>>
> >>>>>> + /*
> >>>>>> + * We only get here once per ephemeral inode. The inode has
> >>>>>> + * been initialized via inode_alloc_security but is otherwise
> >>>>>> + * untouched, so check that the state is as
> >>>>>> + * inode_alloc_security left it.
> >>>>>> + */
> >>>>>> + BUG_ON(isec->initialized != LABEL_INVALID);
> >>>>>> + BUG_ON(isec->sclass != SECCLASS_FILE);
> >>>>>
> >>>>> I think the kernel discourages overuse of BUG_ON/BUG/...
> >>>>
> >>>> I'm not sure what counts as overuse.
> >>>
> >>> Me either (not my rule) but I'm pretty sure this counts or you'd see a
> >>> lot more of these kinds of BUG_ON() checks throughout. Try to reserve
> >>> them for really critical cases.
> >>>
> >>>>>> +
> >>>>>> +#ifdef CONFIG_USERFAULTFD
> >>>>>> + if (fops == &userfaultfd_fops)
> >>>>>> + isec->sclass = SECCLASS_UFFD;
> >>>>>> +#endif
> >>>>>
> >>>>> Not sure we want or need to introduce a new security class for each user
> >>>>> of anonymous inodes since the permissions should be the same as for
> >>>>> file.
> >>>>
> >>>> The purpose of this change is to apply special policy to userfaultfd
> >>>> FDs in particular. Isn't having a UFFD security class the best way to
> >>>> go about that? (There's no path.) Am I missing something?
> >>>
> >>> It is probably the simplest approach; it just doesn't generalize to all
> >>> users of anonymous inodes. We can distinguish them in one of two ways:
> >>> use a different class like you did (requires a code change every time we
> >>> add a new one and yet another duplicate of the file class) or use a
> >>> different SID/context/type. The latter could be achieved by calling
> >>> security_transition_sid() with the provided name wrapped in a qstr and
> >>> specifying type_transition rules on the name. Then policy could define
> >>> derived types for each domain, ala
> >>> type_transition init self:file "[userfaultfd]" init_userfaultfd;
> >>> type_transition untrusted_app self:file "[userfaultfd]"
> >>> untrusted_app_userfaultfd;
> >>> ...
> >>>
> >>>>> Also not sure we want to be testing fops for each such case.
> >>>>
> >>>> I was also thinking of just providing some kind of context string
> >>>> (maybe the name), which might be friendlier to modules, but the loose
> >>>> coupling kind of scares me, and for this particular application, since
> >>>> UFFD is always in the core and never in a module, checking the fops
> >>>> seems a bit more robust and doesn't hurt anything.
> >>>
> >>> Yes, not sure how the vfs folks feel about either coupling (the
> >>> name-based one or the fops-based one). Neither seems great.
> >>>
> >>>>> We
> >>>>> were looking at possibly leveraging the name as a key and using
> >>>>> security_transition_sid() to generate a distinct SID/context/type for
> >>>>> the inode via type_transition rules in policy. We have some WIP along
> >>>>> those lines.
> >>>>
> >>>> Where? Any chance it would be ready soon? I'd rather not hold up this
> >>>> work for a more general mechanism.
> >>>
> >>> Hopefully will have a patch available soon. But not saying this
> >>> necessarily has to wait either.
> >>>
> >>>>>> + /*
> >>>>>> + * Always give secure anonymous inodes the sid of the
> >>>>>> + * creating task.
> >>>>>> + */
> >>>>>> +
> >>>>>> + isec->sid = tsec->sid;
> >>>>>
> >>>>> This doesn't generalize for other users of anonymous inodes, e.g. the
> >>>>> /dev/kvm case where we'd rather inherit the SID and class from the
> >>>>> original /dev/kvm inode itself.
> >>>>
> >>>> I think someone mentioned on the first version of this patch that we
> >>>> could make it more flexible if the need arose. If we do want to do it
> >>>> now, we could have the anon_inode security hook accept a "parent" or
> >>>> "context" inode that modules could inspect for the purposes of forming
> >>>> the new inode's SID. Does that make sense to you?
> >>>
> >>> Yes, that's the approach in our current WIP, except we call it a
> >>> "related" inode since it isn't necessarily connected to the anon inode
> >>> in any vfs sense.
> >>
> >> The other key difference in our WIP approach is that we assumed that we
> >> couldn't mandate allocating a separate anon inode for each of these fds
> >> and we wanted to cover all anonymous inodes (not opt-in), so we are
> >> storing the SID/class pair as additional fields in the
> >> file_security_struct and have modified file_has_perm() and others to
> >> look there for anonymous inodes.
> >
> > A separate inode seems like the simpler approach for now, because it
> > means that we have fewer places to check for security information ---
> > and it's not as if an inode is particularly expensive. We can always
> > switch later.
>
> We'd prefer having a separate inode if possible but didn't think that
> would fly with the vfs folks,
Let's ask them.
> especially if we try to apply this to all
> anonymous inodes.
For the moment, we're not.
> It might be ok for userfaultfd usage as a specific
> case but there is a reason why anonymous inodes were introduced and
> creating a separate inode each time defeats that purpose IIUC. It will
> be interesting to see how they respond.
Sort of. Anonymous inodes also free other parts of the kernel from
having to deal with special-purpose filesystems (like pipefs) on which
to hang custom inodes. It's just a generic "just give me an inode and
I don't care about the filesystem" feature, and if we actually get a
new inode each time, we still do the job. Pipe seems to be good with
creating inodes each time.
On 2/12/20 2:11 PM, Stephen Smalley wrote:
> On 2/12/20 2:04 PM, Daniel Colascione wrote:
>> On Wed, Feb 12, 2020 at 10:59 AM Stephen Smalley <[email protected]>
>> wrote:
>>>
>>> On 2/12/20 1:04 PM, Stephen Smalley wrote:
>>>> On 2/12/20 12:19 PM, Daniel Colascione wrote:
>>>>> Thanks for taking a look.
>>>>>
>>>>> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <[email protected]>
>>>>> wrote:
>>>>>>
>>>>>> On 2/11/20 5:55 PM, Daniel Colascione wrote:
>>>>>>> Use the secure anonymous inode LSM hook we just added to let SELinux
>>>>>>> policy place restrictions on userfaultfd use. The create operation
>>>>>>> applies to processes creating new instances of these file objects;
>>>>>>> transfer between processes is covered by restrictions on read,
>>>>>>> write,
>>>>>>> and ioctl access already checked inside selinux_file_receive.
>>>>>>>
>>>>>>> Signed-off-by: Daniel Colascione <[email protected]>
>>>>>>
>>>>>> (please add linux-fsdevel and viro to the cc for future versions
>>>>>> of this
>>>>>> patch since it changes the VFS)
>>>>>>
>>>>>>> ---
>>>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>>>> index 1659b59fb5d7..e178f6f40e93 100644
>>>>>>> --- a/security/selinux/hooks.c
>>>>>>> +++ b/security/selinux/hooks.c
>>>>>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct
>>>>>>> inode *inode, struct inode *dir,
>>>>>>> +
>>>>>>> +Â Â Â Â /*
>>>>>>> +Â Â Â Â Â * We shouldn't be creating secure anonymous inodes before LSM
>>>>>>> +Â Â Â Â Â * initialization completes.
>>>>>>> +Â Â Â Â Â */
>>>>>>> +Â Â Â Â if (unlikely(!selinux_state.initialized))
>>>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â return -EBUSY;
>>>>>>
>>>>>> I don't think this is viable; any arbitrary actions are possible
>>>>>> before
>>>>>> policy is loaded, and a Linux distro can be brought up fully with
>>>>>> SELinux enabled and no policy loaded. You'll just need to have a
>>>>>> default behavior prior to initialization.
>>>>>
>>>>> We'd have to fail open then, I think, and return an S_PRIVATE inode
>>>>> (the regular anon inode).
>>>>
>>>> Not sure why. You aren't doing anything in the hook that actually
>>>> relies on selinux_state.initialized being set (i.e. nothing requires a
>>>> policy). The avc_has_perm() call will just succeed until a policy is
>>>> loaded. So if these inodes are created prior to policy load, they will
>>>> get assigned the task SID (which would be the kernel SID prior to
>>>> policy
>>>> load or first exec or write to /proc/self/attr/current afterward) and
>>>> UFFD class (in your current code), be permitted, and then once
>>>> policy is
>>>> loaded any further access will get checked against the kernel SID.
>>>>
>>>>>>> +Â Â Â Â /*
>>>>>>> +     * We only get here once per ephemeral inode. The inode has
>>>>>>> +Â Â Â Â Â * been initialized via inode_alloc_security but is otherwise
>>>>>>> +Â Â Â Â Â * untouched, so check that the state is as
>>>>>>> +Â Â Â Â Â * inode_alloc_security left it.
>>>>>>> +Â Â Â Â Â */
>>>>>>> +Â Â Â Â BUG_ON(isec->initialized != LABEL_INVALID);
>>>>>>> +Â Â Â Â BUG_ON(isec->sclass != SECCLASS_FILE);
>>>>>>
>>>>>> I think the kernel discourages overuse of BUG_ON/BUG/...
>>>>>
>>>>> I'm not sure what counts as overuse.
>>>>
>>>> Me either (not my rule) but I'm pretty sure this counts or you'd see a
>>>> lot more of these kinds of BUG_ON() checks throughout. Try to reserve
>>>> them for really critical cases.
>>>>
>>>>>>> +
>>>>>>> +#ifdef CONFIG_USERFAULTFD
>>>>>>> +Â Â Â Â if (fops == &userfaultfd_fops)
>>>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â isec->sclass = SECCLASS_UFFD;
>>>>>>> +#endif
>>>>>>
>>>>>> Not sure we want or need to introduce a new security class for
>>>>>> each user
>>>>>> of anonymous inodes since the permissions should be the same as for
>>>>>> file.
>>>>>
>>>>> The purpose of this change is to apply special policy to userfaultfd
>>>>> FDs in particular. Isn't having a UFFD security class the best way to
>>>>> go about that? (There's no path.) Am I missing something?
>>>>
>>>> It is probably the simplest approach; it just doesn't generalize to all
>>>> users of anonymous inodes. We can distinguish them in one of two ways:
>>>> use a different class like you did (requires a code change every
>>>> time we
>>>> add a new one and yet another duplicate of the file class) or use a
>>>> different SID/context/type. The latter could be achieved by calling
>>>> security_transition_sid() with the provided name wrapped in a qstr and
>>>> specifying type_transition rules on the name. Then policy could define
>>>> derived types for each domain, ala
>>>> type_transition init self:file "[userfaultfd]" init_userfaultfd;
>>>> type_transition untrusted_app self:file "[userfaultfd]"
>>>> untrusted_app_userfaultfd;
>>>> ...
>>>>
>>>>>> Also not sure we want to be testing fops for each such case.
>>>>>
>>>>> I was also thinking of just providing some kind of context string
>>>>> (maybe the name), which might be friendlier to modules, but the loose
>>>>> coupling kind of scares me, and for this particular application, since
>>>>> UFFD is always in the core and never in a module, checking the fops
>>>>> seems a bit more robust and doesn't hurt anything.
>>>>
>>>> Yes, not sure how the vfs folks feel about either coupling (the
>>>> name-based one or the fops-based one). Neither seems great.
>>>>
>>>>>> We
>>>>>> were looking at possibly leveraging the name as a key and using
>>>>>> security_transition_sid() to generate a distinct SID/context/type for
>>>>>> the inode via type_transition rules in policy. We have some WIP
>>>>>> along
>>>>>> those lines.
>>>>>
>>>>> Where? Any chance it would be ready soon? I'd rather not hold up this
>>>>> work for a more general mechanism.
>>>>
>>>> Hopefully will have a patch available soon. But not saying this
>>>> necessarily has to wait either.
>>>>
>>>>>>> +Â Â Â Â /*
>>>>>>> +Â Â Â Â Â * Always give secure anonymous inodes the sid of the
>>>>>>> +Â Â Â Â Â * creating task.
>>>>>>> +Â Â Â Â Â */
>>>>>>> +
>>>>>>> +Â Â Â Â isec->sid = tsec->sid;
>>>>>>
>>>>>> This doesn't generalize for other users of anonymous inodes, e.g. the
>>>>>> /dev/kvm case where we'd rather inherit the SID and class from the
>>>>>> original /dev/kvm inode itself.
>>>>>
>>>>> I think someone mentioned on the first version of this patch that we
>>>>> could make it more flexible if the need arose. If we do want to do it
>>>>> now, we could have the anon_inode security hook accept a "parent" or
>>>>> "context" inode that modules could inspect for the purposes of forming
>>>>> the new inode's SID. Does that make sense to you?
>>>>
>>>> Yes, that's the approach in our current WIP, except we call it a
>>>> "related" inode since it isn't necessarily connected to the anon inode
>>>> in any vfs sense.
>>>
>>> The other key difference in our WIP approach is that we assumed that we
>>> couldn't mandate allocating a separate anon inode for each of these fds
>>> and we wanted to cover all anonymous inodes (not opt-in), so we are
>>> storing the SID/class pair as additional fields in the
>>> file_security_struct and have modified file_has_perm() and others to
>>> look there for anonymous inodes.
>>
>> A separate inode seems like the simpler approach for now, because it
>> means that we have fewer places to check for security information ---
>> and it's not as if an inode is particularly expensive. We can always
>> switch later.
>
> We'd prefer having a separate inode if possible but didn't think that
> would fly with the vfs folks, especially if we try to apply this to all
> anonymous inodes. It might be ok for userfaultfd usage as a specific
> case but there is a reason why anonymous inodes were introduced and
> creating a separate inode each time defeats that purpose IIUC. It will
> be interesting to see how they respond.
I suppose an optimization of your approach could be to only allocate a
new anon inode if there isn't already one that has the same security
info (SID/class pair in the SELinux case).
Hello everyone,
On Wed, Feb 12, 2020 at 12:14:16PM -0500, Peter Xu wrote:
> Right. AFAICT QEMU uses it far more than disk IOs. A guest page can
> be accessed by any kernel component on the destination host during a
> postcopy procedure. It can be as simple as when a vcpu writes to a
> missing guest page which still resides on the source host, then KVM
> will get a page fault and trap into userfaultfd asking for that page.
> The same thing happens to other modules like vhost, etc., as long as a
> missing guest page is touched by a kernel module.
Correct.
How does the android garbage collection work to make sure there cannot
be kernel faults on the missing memory?
If I understood correctly (I didn't have much time to review sorry)
what's proposed with regard to limiting uffd events from kernel
faults, the only use case I know that could deal with it is the
UFFD_FEATURE_SIGBUS but that's not normal userfaultfd: that's also the
only feature required from uffd to implement a pure malloc library in
userland that never takes the mmap sem for writing to implement
userland mremap/mmap/munmap lib calls (as those will convert to
UFFDIO_ZEROPAGE and MADV_DONTNEED internally to the lib and there will
be always a single vma). We just need to extend UFFDIO_ZEROPAGE to map
the THP zeropage to make this future pure-uffd malloc lib perform
better.
On the other end I'm also planning a mremap_vma_merge userland syscall
that will merge fragmented vmas.
Currently once you have a nice heap all contiguous but with small
objects and you free the fragments you can't build THP anymore even if
you make the memory virtually contiguous again by calling mremap. That
just build up a ton of vmas slowing down the app forever and also
preventing THP collapsing ever again.
mremap_vma_merge will require no new kernel feature, but it
fundamentally must be able to handle kernel faults. If databases
starts to use that, how can you enable this feature without breaking
random apps then?
So it'd be a feature usable only by one user (Android) perhaps? And
only until you start defragging the vmas of small objects?
Thanks,
Andrea
On Wed, Feb 12, 2020 at 11:41 AM Andrea Arcangeli <[email protected]> wrote:
>
> Hello everyone,
>
> On Wed, Feb 12, 2020 at 12:14:16PM -0500, Peter Xu wrote:
> > Right. AFAICT QEMU uses it far more than disk IOs. A guest page can
> > be accessed by any kernel component on the destination host during a
> > postcopy procedure. It can be as simple as when a vcpu writes to a
> > missing guest page which still resides on the source host, then KVM
> > will get a page fault and trap into userfaultfd asking for that page.
> > The same thing happens to other modules like vhost, etc., as long as a
> > missing guest page is touched by a kernel module.
>
> Correct.
>
> How does the android garbage collection work to make sure there cannot
> be kernel faults on the missing memory?
We don't pass pointers to the heap into system calls. (Big primitive
arrays, ByteBuffer, etc. are allocated off the regular heap.)
> If I understood correctly (I didn't have much time to review sorry)
> what's proposed with regard to limiting uffd events from kernel
> faults,
I don't understand what you mean. The purpose of preventing UFFD from
handling kernel faults is exploit mitigation.
> the only use case I know that could deal with it is the
> UFFD_FEATURE_SIGBUS but that's not normal userfaultfd: that's also the
> only feature required from uffd to implement a pure malloc library in
> userland that never takes the mmap sem for writing to implement
> userland mremap/mmap/munmap lib calls (as those will convert to
> UFFDIO_ZEROPAGE and MADV_DONTNEED internally to the lib and there will
> be always a single vma). We just need to extend UFFDIO_ZEROPAGE to map
> the THP zeropage to make this future pure-uffd malloc lib perform
> better.
The key requirement here is the ability to prevent unprivileged
processes from using UFFD to widen kernel exploit windows by
preventing UFFD from taking kernel faults. Forcing unprivileged
processes to use UFFD only with UFFD_FEATURE_SIGBUS would satisfy this
requirement, but it's much less flexible and unnecessarily couples two
features.
> On the other end I'm also planning a mremap_vma_merge userland syscall
> that will merge fragmented vmas.
This is probably a separate discussion, but does that operation really
need to be a system call? Historically, userspace has operated mostly
on page ranges and not VMAs per se, and the kernel has been free to
merge and split VMAs as needed for its internal purposes. This
approach has serious negative side effects (like making munmap
fallible: see [1]), but it is what it is.
[1] https://lore.kernel.org/linux-mm/CAKOZuetOD6MkGPVvYFLj5RXh200FaDyu3sQqZviVRhTFFS3fjA@mail.gmail.com/
> Currently once you have a nice heap all contiguous but with small
> objects and you free the fragments you can't build THP anymore even if
> you make the memory virtually contiguous again by calling mremap. That
> just build up a ton of vmas slowing down the app forever and also
> preventing THP collapsing ever again.
Shouldn't the THP background kthread take care of merging VMAs?
> mremap_vma_merge will require no new kernel feature, but it
> fundamentally must be able to handle kernel faults. If databases
> starts to use that, how can you enable this feature without breaking
> random apps then?
Presumably, those apps wouldn't issue the system call on address
ranges managed with a non-kernel-fault UFFD.
> So it'd be a feature usable only by one user (Android) perhaps? And
> only until you start defragging the vmas of small objects?
We shouldn't be fragmenting at all, either at the memory level or the
VMA level. The GC is a moving collector, and we don't punch holes in
the heap.
Hi Daniel,
On Wed, Feb 12, 2020 at 12:04:39PM -0800, Daniel Colascione wrote:
> We don't pass pointers to the heap into system calls. (Big primitive
> arrays, ByteBuffer, etc. are allocated off the regular heap.)
That sounds pretty restrictive, I wonder what you gain by enforcing
that invariant or if it just happened incidentally for some other
reason? Do you need to copy that memory every time if you need to do
I/O on it? Are you sure this won't need to change any time soon to
increase performance?
> I don't understand what you mean. The purpose of preventing UFFD from
> handling kernel faults is exploit mitigation.
That part was clear. What wasn't clear is what the new feature
does exactly and what it blocks, because it's all about blocking or
how does it make things more secure?
> The key requirement here is the ability to prevent unprivileged
> processes from using UFFD to widen kernel exploit windows by
> preventing UFFD from taking kernel faults. Forcing unprivileged
> processes to use UFFD only with UFFD_FEATURE_SIGBUS would satisfy this
> requirement, but it's much less flexible and unnecessarily couples two
> features.
I mentioned it in case you could use something like that model.
> > On the other end I'm also planning a mremap_vma_merge userland syscall
> > that will merge fragmented vmas.
>
> This is probably a separate discussion, but does that operation really
> need to be a system call? Historically, userspace has operated mostly
mremap_vma_merge was not intended as a system call, if implemented as
a system call it wouldn't use uffd.
> on page ranges and not VMAs per se, and the kernel has been free to
Userland doesn't need to know anything.. unless it wants to optimize.
The userland can know full well if it does certain mremap operations
and puts its ranges virtually contiguous in a non linear way, so that
the kernel cannot merge the vmas.
> merge and split VMAs as needed for its internal purposes. This
> approach has serious negative side effects (like making munmap
> fallible: see [1]), but it is what it is.
>
> [1] https://lore.kernel.org/linux-mm/CAKOZuetOD6MkGPVvYFLj5RXh200FaDyu3sQqZviVRhTFFS3fjA@mail.gmail.com/
The fact it's fallible is a secondary concern here. Even if you make
it unlimited, if it grows it slowdown everything and also prevents THP
to be collapsed. Even the scalability of the mmap_sem worsens.
> > Currently once you have a nice heap all contiguous but with small
> > objects and you free the fragments you can't build THP anymore even if
> > you make the memory virtually contiguous again by calling mremap. That
> > just build up a ton of vmas slowing down the app forever and also
> > preventing THP collapsing ever again.
>
> Shouldn't the THP background kthread take care of merging VMAs?
The solution can't depend on any THP feature, because the buildup of
vmas is a scalability issue and a performance regression over time
even if THP is not configured in the kernel. However once that's
solved THP also gets naturally optimized.
What should happen (in my view) is just the simplest solution that can
defrag and forcefully merge the vmas without having to stop or restart
the app.
> Presumably, those apps wouldn't issue the system call on address
> ranges managed with a non-kernel-fault UFFD.
So the new security feature won't have to block kernel faults on those
apps and they can run side by side with the blocked app?
> We shouldn't be fragmenting at all, either at the memory level or the
> VMA level. The GC is a moving collector, and we don't punch holes in
> the heap.
That sounds good.
Thanks,
Andrea
Userfaultfd in unprivileged contexts could be potentially very
useful. We'd like to harden userfaultfd to make such unprivileged use
less risky. This patch series allows SELinux to manage userfaultfd
file descriptors and in the future, other kinds of
anonymous-inode-based file descriptor. SELinux policy authors can
apply policy types to anonymous inodes by providing name-based
transition rules keyed off the anonymous inode internal name (
"[userfaultfd]" in the case of userfaultfd(2) file descriptors) and
applying policy to the new SIDs thus produced.
Inside the kernel, a pair of new anon_inodes interface,
anon_inode_getfile_secure and anon_inode_getfd_secure, allow callers
to opt into this SELinux management. In this new "secure" mode,
anon_inodes creates new ephemeral inodes for anonymous file objects
instead of reusing the normal anon_inodes singleton dummy inode. A new
LSM hook gives security modules an opportunity to configure and veto
these ephemeral inodes.
This patch series is one of two fork of [1] and is an
alternative to [2].
The primary difference between the two patch series is that this
partch series creates a unique inode for each "secure" anonymous
inode, while the other patch series ([2]) continues using the
singleton dummy anonymous inode and adds a way to attach SELinux
security information directly to file objects.
I prefer the approach in this patch series because 1) it's a smaller
patch than [2], and 2) it produces a more regular security
architecture: in this patch series, secure anonymous inodes aren't
S_PRIVATE and they maintain the SELinux property that the label for a
file is in its inode. We do need an additional inode per anonymous
file, but per-struct-file inode creation doesn't seem to be a problem
for pipes and sockets.
The previous version of this feature ([1]) created a new SELinux
security class for userfaultfd file descriptors. This version adopts
the generic transition-based approach of [2].
This patch series also differs from [2] in that it doesn't affect all
anonymous inodes right away --- instead requiring anon_inodes callers
to opt in --- but this difference isn't one of basic approach. The
important question to resolve is whether we should be creating new
inodes or enhancing per-file data.
[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/linux-fsdevel/[email protected]/
Daniel Colascione (3):
Add a new LSM-supporting anonymous inode interface
Teach SELinux about anonymous inodes
Wire UFFD up to SELinux
fs/anon_inodes.c | 196 ++++++++++++++++++++++++++++--------
fs/userfaultfd.c | 34 +++++--
include/linux/anon_inodes.h | 13 +++
include/linux/lsm_hooks.h | 9 ++
include/linux/security.h | 4 +
security/security.c | 10 ++
security/selinux/hooks.c | 57 +++++++++++
7 files changed, 274 insertions(+), 49 deletions(-)
--
2.25.0.265.gbab2e86ba0-goog
On Tue, 11 Feb 2020, Daniel Colascione wrote:
> > This must be posted to the linux Security Module list
> > <[email protected]>
>
> Added. I thought selinux@ was sufficient.
>
Please cc: me on these patches.
--
James Morris
<[email protected]>
Userfaultfd in unprivileged contexts could be potentially very
useful. We'd like to harden userfaultfd to make such unprivileged use
less risky. This patch series allows SELinux to manage userfaultfd
file descriptors and in the future, other kinds of
anonymous-inode-based file descriptor. SELinux policy authors can
apply policy types to anonymous inodes by providing name-based
transition rules keyed off the anonymous inode internal name (
"[userfaultfd]" in the case of userfaultfd(2) file descriptors) and
applying policy to the new SIDs thus produced.
Inside the kernel, a pair of new anon_inodes interface,
anon_inode_getfile_secure and anon_inode_getfd_secure, allow callers
to opt into this SELinux management. In this new "secure" mode,
anon_inodes creates new ephemeral inodes for anonymous file objects
instead of reusing the normal anon_inodes singleton dummy inode. A new
LSM hook gives security modules an opportunity to configure and veto
these ephemeral inodes.
This patch series is one of two fork of [1] and is an
alternative to [2].
The primary difference between the two patch series is that this
partch series creates a unique inode for each "secure" anonymous
inode, while the other patch series ([2]) continues using the
singleton dummy anonymous inode and adds a way to attach SELinux
security information directly to file objects.
I prefer the approach in this patch series because 1) it's a smaller
patch than [2], and 2) it produces a more regular security
architecture: in this patch series, secure anonymous inodes aren't
S_PRIVATE and they maintain the SELinux property that the label for a
file is in its inode. We do need an additional inode per anonymous
file, but per-struct-file inode creation doesn't seem to be a problem
for pipes and sockets.
The previous version of this feature ([1]) created a new SELinux
security class for userfaultfd file descriptors. This version adopts
the generic transition-based approach of [2].
This patch series also differs from [2] in that it doesn't affect all
anonymous inodes right away --- instead requiring anon_inodes callers
to opt in --- but this difference isn't one of basic approach. The
important question to resolve is whether we should be creating new
inodes or enhancing per-file data.
Changes from the first version of the patch:
- Removed some error checks
- Defined a new anon_inode SELinux class to resolve the
ambiguity in [3]
- Inherit sclass as well as descriptor from context inode
[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):
Add a new LSM-supporting anonymous inode interface
Teach SELinux about anonymous inodes
Wire UFFD up to SELinux
fs/anon_inodes.c | 196 ++++++++++++++++++++++------
fs/userfaultfd.c | 30 ++++-
include/linux/anon_inodes.h | 13 ++
include/linux/lsm_hooks.h | 9 ++
include/linux/security.h | 4 +
security/security.c | 10 ++
security/selinux/hooks.c | 54 ++++++++
security/selinux/include/classmap.h | 2 +
8 files changed, 272 insertions(+), 46 deletions(-)
--
2.25.1.696.g5e7596f4ac-goog
This change adds two new functions, anon_inode_getfile_secure and
anon_inode_getfd_secure, that create anonymous-node files with
individual non-S_PRIVATE inodes to which security modules can apply
policy. Existing callers continue using the original singleton-inode
kind of anonymous-inode file. We can transition anonymous inode users
to the new kind of anonymous inode in individual patches for the sake
of bisection and review.
The new functions accept an optional context_inode parameter that
callers can use to provide additional contextual information to
security modules, e.g., indicating that one anonymous struct file is a
logical child of another, allowing a security model to propagate
security information from one to the other.
Signed-off-by: Daniel Colascione <[email protected]>
---
fs/anon_inodes.c | 196 ++++++++++++++++++++++++++++--------
fs/userfaultfd.c | 4 +-
include/linux/anon_inodes.h | 13 +++
include/linux/lsm_hooks.h | 9 ++
include/linux/security.h | 4 +
security/security.c | 10 ++
6 files changed, 191 insertions(+), 45 deletions(-)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b..114a04fc1db4 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -55,75 +55,135 @@ static struct file_system_type anon_inode_fs_type = {
.kill_sb = kill_anon_super,
};
-/**
- * anon_inode_getfile - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
- *
- * @name: [in] name of the "class" of the new file
- * @fops: [in] file operations for the new file
- * @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
- *
- * Creates a new file by hooking it on a single inode. This is useful for files
- * that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfile() will share a single inode,
- * hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns the newly created file* or an error pointer.
- */
-struct file *anon_inode_getfile(const char *name,
- const struct file_operations *fops,
- void *priv, int flags)
+static struct inode *anon_inode_make_secure_inode(
+ const char *name,
+ const struct inode *context_inode,
+ const struct file_operations *fops)
+{
+ struct inode *inode;
+ const struct qstr qname = QSTR_INIT(name, strlen(name));
+ int error;
+
+ inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
+ if (IS_ERR(inode))
+ return ERR_PTR(PTR_ERR(inode));
+ inode->i_flags &= ~S_PRIVATE;
+ error = security_inode_init_security_anon(
+ inode, &qname, fops, context_inode);
+ if (error) {
+ iput(inode);
+ return ERR_PTR(error);
+ }
+ return inode;
+}
+
+struct file *_anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
+ struct inode *inode;
struct file *file;
- if (IS_ERR(anon_inode_inode))
- return ERR_PTR(-ENODEV);
+ if (secure) {
+ inode = anon_inode_make_secure_inode(
+ name, context_inode, fops);
+ if (IS_ERR(inode))
+ return ERR_PTR(PTR_ERR(inode));
+ } else {
+ inode = anon_inode_inode;
+ if (IS_ERR(inode))
+ return ERR_PTR(-ENODEV);
+ /*
+ * We know the anon_inode inode count is always
+ * greater than zero, so ihold() is safe.
+ */
+ ihold(inode);
+ }
- if (fops->owner && !try_module_get(fops->owner))
- return ERR_PTR(-ENOENT);
+ if (fops->owner && !try_module_get(fops->owner)) {
+ file = ERR_PTR(-ENOENT);
+ goto err;
+ }
- /*
- * We know the anon_inode inode count is always greater than zero,
- * so ihold() is safe.
- */
- ihold(anon_inode_inode);
- file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name,
+ file = alloc_file_pseudo(inode, anon_inode_mnt, name,
flags & (O_ACCMODE | O_NONBLOCK), fops);
if (IS_ERR(file))
goto err;
- file->f_mapping = anon_inode_inode->i_mapping;
+ file->f_mapping = inode->i_mapping;
file->private_data = priv;
return file;
err:
- iput(anon_inode_inode);
+ iput(inode);
module_put(fops->owner);
return file;
}
-EXPORT_SYMBOL_GPL(anon_inode_getfile);
/**
- * anon_inode_getfd - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
+ * anon_inode_getfile_secure - creates a new file instance by hooking
+ * it up to a new anonymous inode and a
+ * dentry that describe the "class" of the
+ * file. Make it possible to use security
+ * modules to control access to the
+ * new file.
*
* @name: [in] name of the "class" of the new file
* @fops: [in] file operations for the new file
* @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
+ * @flags: [in] flags for the file
+ * @anon_inode_flags: [in] flags for anon_inode*
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns the
+ * newly created file* or an error pointer.
+ */
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfile(
+ name, fops, priv, flags, context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile_secure);
+
+/**
+ * anon_inode_getfile - creates a new file instance by hooking it up
+ * to an anonymous inode and a dentry that
+ * describe the "class" of the file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags for the file
*
- * Creates a new file by hooking it on a single inode. This is useful for files
+ * Creates a new file by hooking it on an unspecified inode. This is useful for files
* that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfd() will share a single inode,
+ * All the files created with anon_inode_getfile() will share a single inode,
* hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns new descriptor or an error code.
+ * setup. Returns the newly created file* or an error pointer.
*/
-int anon_inode_getfd(const char *name, const struct file_operations *fops,
- void *priv, int flags)
+struct file *anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfile(name, fops, priv, flags, NULL, false);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile);
+
+static int _anon_inode_getfd(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
int error, fd;
struct file *file;
@@ -133,7 +193,8 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
return error;
fd = error;
- file = anon_inode_getfile(name, fops, priv, flags);
+ file = _anon_inode_getfile(name, fops, priv, flags, context_inode,
+ secure);
if (IS_ERR(file)) {
error = PTR_ERR(file);
goto err_put_unused_fd;
@@ -146,6 +207,57 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
put_unused_fd(fd);
return error;
}
+
+/**
+ * anon_inode_getfd_secure - creates a new file instance by hooking it
+ * up to a new anonymous inode and a dentry
+ * that describe the "class" of the file.
+ * Make it possible to use security modules
+ * to control access to the new file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns a newly
+ * created file descriptor or an error code.
+ */
+int anon_inode_getfd_secure(const char *name, const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfd(name, fops, priv, flags,
+ context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);
+
+/**
+ * anon_inode_getfd - creates a new file instance by hooking it up to
+ * an anonymous inode and a dentry that describe
+ * the "class" of the file
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile() will use the same singleton inode, reducing
+ * memory use and avoiding code duplication for the file/inode/dentry
+ * setup. Returns a newly created file descriptor or an error code.
+ */
+int anon_inode_getfd(const char *name, const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfd(name, fops, priv, flags, NULL, false);
+}
EXPORT_SYMBOL_GPL(anon_inode_getfd);
static int __init anon_inode_init(void)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 37df7c9eedb1..07b0f6e03849 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1014,8 +1014,6 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
}
}
-static const struct file_operations userfaultfd_fops;
-
static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
struct userfaultfd_ctx *new,
struct uffd_msg *msg)
@@ -1920,7 +1918,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f)
}
#endif
-static const struct file_operations userfaultfd_fops = {
+const struct file_operations userfaultfd_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = userfaultfd_show_fdinfo,
#endif
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261ad..67bd85d92dca 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -10,12 +10,25 @@
#define _LINUX_ANON_INODES_H
struct file_operations;
+struct inode;
+
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+
+int anon_inode_getfd_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
+
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
+
#endif /* _LINUX_ANON_INODES_H */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 20d8cf194fb7..de5d37e388df 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -215,6 +215,10 @@
* Returns 0 if @name and @value have been successfully set,
* -EOPNOTSUPP if no security attribute is needed, or
* -ENOMEM on memory allocation failure.
+ * @inode_init_security_anon:
+ * Set up a secure anonymous inode.
+ * Returns 0 on success. Returns -EPERM if the security module denies
+ * the creation of this inode.
* @inode_create:
* Check permission to create a regular file.
* @dir contains inode structure of the parent of the new file.
@@ -1552,6 +1556,10 @@ union security_list_options {
const struct qstr *qstr,
const char **name, void **value,
size_t *len);
+ int (*inode_init_security_anon)(struct inode *inode,
+ const struct qstr *name,
+ const struct file_operations *fops,
+ const struct inode *context_inode);
int (*inode_create)(struct inode *dir, struct dentry *dentry,
umode_t mode);
int (*inode_link)(struct dentry *old_dentry, struct inode *dir,
@@ -1884,6 +1892,7 @@ struct security_hook_heads {
struct hlist_head inode_alloc_security;
struct hlist_head inode_free_security;
struct hlist_head inode_init_security;
+ struct hlist_head inode_init_security_anon;
struct hlist_head inode_create;
struct hlist_head inode_link;
struct hlist_head inode_unlink;
diff --git a/include/linux/security.h b/include/linux/security.h
index 64b19f050343..8ea76af0be7a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -320,6 +320,10 @@ 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 file_operations *fops,
+ const struct inode *context_inode);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len);
diff --git a/security/security.c b/security/security.c
index 565bc9b67276..d06f3969c030 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1033,6 +1033,16 @@ 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 file_operations *fops,
+ const struct inode *context_inode)
+{
+ return call_int_hook(inode_init_security_anon, 0, inode, name,
+ fops, 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.25.1.696.g5e7596f4ac-goog
This change gives userfaultfd file descriptors a real security
context, allowing policy to act on them.
Signed-off-by: Daniel Colascione <[email protected]>
---
fs/userfaultfd.c | 34 +++++++++++++++++++++++++++++-----
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 07b0f6e03849..78ff5d898733 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -76,6 +76,8 @@ struct userfaultfd_ctx {
bool mmap_changing;
/* mm with one ore more vmas attached to this userfaultfd_ctx */
struct mm_struct *mm;
+ /* The inode that owns this context --- not a strong reference. */
+ const struct inode *owner;
};
struct userfaultfd_fork_ctx {
@@ -1014,14 +1016,18 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
}
}
+static const struct file_operations userfaultfd_fops;
+
static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
struct userfaultfd_ctx *new,
struct uffd_msg *msg)
{
int fd;
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, new,
- O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS));
+ fd = anon_inode_getfd_secure(
+ "[userfaultfd]", &userfaultfd_fops, new,
+ O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS),
+ ctx->owner);
if (fd < 0)
return fd;
@@ -1918,7 +1924,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f)
}
#endif
-const struct file_operations userfaultfd_fops = {
+static const struct file_operations userfaultfd_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = userfaultfd_show_fdinfo,
#endif
@@ -1943,6 +1949,7 @@ static void init_once_userfaultfd_ctx(void *mem)
SYSCALL_DEFINE1(userfaultfd, int, flags)
{
+ struct file *file;
struct userfaultfd_ctx *ctx;
int fd;
@@ -1972,8 +1979,25 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
/* prevent the mm struct to be freed */
mmgrab(ctx->mm);
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, ctx,
- O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS));
+ file = anon_inode_getfile_secure(
+ "[userfaultfd]", &userfaultfd_fops, ctx,
+ O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS),
+ NULL);
+ if (IS_ERR(file)) {
+ fd = PTR_ERR(file);
+ goto out;
+ }
+
+ fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
+ if (fd < 0) {
+ fput(file);
+ goto out;
+ }
+
+ ctx->owner = file_inode(file);
+ fd_install(fd, file);
+
+out:
if (fd < 0) {
mmdrop(ctx->mm);
kmem_cache_free(userfaultfd_ctx_cachep, ctx);
--
2.25.1.696.g5e7596f4ac-goog
This change uses the anon_inodes and LSM infrastructure introduced in
the previous patch to give SELinux the ability to control
anonymous-inode files that are created using the new _secure()
anon_inodes functions.
A SELinux policy author detects and controls these anonymous inodes by
adding a name-based type_transition rule that assigns a new security
type to anonymous-inode files created in some domain. The name used
for the name-based transition is the name associated with the
anonymous inode for file listings --- e.g., "[userfaultfd]" or
"[perf_event]".
Example:
type uffd_t;
type_transition sysadm_t sysadm_t : file uffd_t "[userfaultfd]";
allow sysadm_t uffd_t:file { 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]>
---
security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 ++
2 files changed, 56 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1659b59fb5d7..b9eb45c2e4e5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2915,6 +2915,59 @@ 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 file_operations *fops,
+ const struct inode *context_inode)
+{
+ const struct task_security_struct *tsec = selinux_cred(current_cred());
+ struct common_audit_data ad;
+ struct inode_security_struct *isec;
+ int rc;
+
+ if (unlikely(!selinux_state.initialized))
+ return 0;
+
+ isec = selinux_inode(inode);
+
+ /*
+ * We only get here once per ephemeral inode. The inode has
+ * been initialized via inode_alloc_security but is otherwise
+ * untouched.
+ */
+
+ if (context_inode) {
+ struct inode_security_struct *context_isec =
+ selinux_inode(context_inode);
+ isec->sclass = context_isec->sclass;
+ isec->sid = context_isec->sid;
+ } else {
+ isec->sclass = SECCLASS_ANON_INODE;
+ rc = security_transition_sid(
+ &selinux_state, tsec->sid, tsec->sid,
+ SECCLASS_FILE, name, &isec->sid);
+ if (rc)
+ return rc;
+ }
+
+ isec->initialized = LABEL_INITIALIZED;
+
+ /*
+ * Now that we've initialized security, check whether we're
+ * allowed to actually create this type of anonymous inode.
+ */
+
+ ad.type = LSM_AUDIT_DATA_INODE;
+ ad.u.inode = inode;
+
+ return avc_has_perm(&selinux_state,
+ tsec->sid,
+ isec->sid,
+ isec->sclass,
+ FILE__CREATE,
+ &ad);
+}
+
static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
{
return may_create(dir, dentry, SECCLASS_FILE);
@@ -6923,6 +6976,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 986f3ac14282..263750b6aaac 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -248,6 +248,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.25.1.696.g5e7596f4ac-goog
On 3/25/2020 4:02 PM, Daniel Colascione wrote:
> This change gives userfaultfd file descriptors a real security
> context, allowing policy to act on them.
You should change the title to "Wire UFFD up to secure anon inodes".
This code should support any LSM that wants to control anon inodes.
If it doesn't, it isn't correct.
All references to SELinux behavior (i.e. assigning a "security context")
should be restricted to the SELinux specific bits of the patch set. You
should be describing how any LSM can use this, not just the LSM you've
targeted.
>
> Signed-off-by: Daniel Colascione <[email protected]>
> ---
> fs/userfaultfd.c | 34 +++++++++++++++++++++++++++++-----
> 1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 07b0f6e03849..78ff5d898733 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -76,6 +76,8 @@ struct userfaultfd_ctx {
> bool mmap_changing;
> /* mm with one ore more vmas attached to this userfaultfd_ctx */
> struct mm_struct *mm;
> + /* The inode that owns this context --- not a strong reference. */
> + const struct inode *owner;
> };
>
> struct userfaultfd_fork_ctx {
> @@ -1014,14 +1016,18 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
> }
> }
>
> +static const struct file_operations userfaultfd_fops;
> +
> static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
> struct userfaultfd_ctx *new,
> struct uffd_msg *msg)
> {
> int fd;
>
> - fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, new,
> - O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS));
> + fd = anon_inode_getfd_secure(
> + "[userfaultfd]", &userfaultfd_fops, new,
> + O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS),
> + ctx->owner);
> if (fd < 0)
> return fd;
>
> @@ -1918,7 +1924,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f)
> }
> #endif
>
> -const struct file_operations userfaultfd_fops = {
> +static const struct file_operations userfaultfd_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = userfaultfd_show_fdinfo,
> #endif
> @@ -1943,6 +1949,7 @@ static void init_once_userfaultfd_ctx(void *mem)
>
> SYSCALL_DEFINE1(userfaultfd, int, flags)
> {
> + struct file *file;
> struct userfaultfd_ctx *ctx;
> int fd;
>
> @@ -1972,8 +1979,25 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
> /* prevent the mm struct to be freed */
> mmgrab(ctx->mm);
>
> - fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, ctx,
> - O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS));
> + file = anon_inode_getfile_secure(
> + "[userfaultfd]", &userfaultfd_fops, ctx,
> + O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS),
> + NULL);
> + if (IS_ERR(file)) {
> + fd = PTR_ERR(file);
> + goto out;
> + }
> +
> + fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> + if (fd < 0) {
> + fput(file);
> + goto out;
> + }
> +
> + ctx->owner = file_inode(file);
> + fd_install(fd, file);
> +
> +out:
> if (fd < 0) {
> mmdrop(ctx->mm);
> kmem_cache_free(userfaultfd_ctx_cachep, ctx);
On 3/25/20 7:02 PM, Daniel Colascione wrote:
> This change adds two new functions, anon_inode_getfile_secure and
> anon_inode_getfd_secure, that create anonymous-node files with
> individual non-S_PRIVATE inodes to which security modules can apply
> policy. Existing callers continue using the original singleton-inode
> kind of anonymous-inode file. We can transition anonymous inode users
> to the new kind of anonymous inode in individual patches for the sake
> of bisection and review.
>
> The new functions accept an optional context_inode parameter that
> callers can use to provide additional contextual information to
> security modules, e.g., indicating that one anonymous struct file is a
> logical child of another, allowing a security model to propagate
> security information from one to the other.
>
> Signed-off-by: Daniel Colascione <[email protected]>
> ---
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 89714308c25b..114a04fc1db4 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -55,75 +55,135 @@ static struct file_system_type anon_inode_fs_type = {
> .kill_sb = kill_anon_super,
> };
>
> -/**
> - * anon_inode_getfile - creates a new file instance by hooking it up to an
> - * anonymous inode, and a dentry that describe the "class"
> - * of the file
> - *
> - * @name: [in] name of the "class" of the new file
> - * @fops: [in] file operations for the new file
> - * @priv: [in] private data for the new file (will be file's private_data)
> - * @flags: [in] flags
> - *
> - * Creates a new file by hooking it on a single inode. This is useful for files
> - * that do not need to have a full-fledged inode in order to operate correctly.
> - * All the files created with anon_inode_getfile() will share a single inode,
> - * hence saving memory and avoiding code duplication for the file/inode/dentry
> - * setup. Returns the newly created file* or an error pointer.
> - */
> -struct file *anon_inode_getfile(const char *name,
> - const struct file_operations *fops,
> - void *priv, int flags)
> +static struct inode *anon_inode_make_secure_inode(
> + const char *name,
> + const struct inode *context_inode,
> + const struct file_operations *fops)
> +{
> + struct inode *inode;
> + const struct qstr qname = QSTR_INIT(name, strlen(name));
> + int error;
> +
> + inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
> + if (IS_ERR(inode))
> + return ERR_PTR(PTR_ERR(inode));
Just return inode here?
> + inode->i_flags &= ~S_PRIVATE;
> + error = security_inode_init_security_anon(
> + inode, &qname, fops, context_inode);
Would drop fops argument to the security hook until we have an actual
user of it; one can always add it later.
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 37df7c9eedb1..07b0f6e03849 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1014,8 +1014,6 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
> }
> }
>
> -static const struct file_operations userfaultfd_fops;
> -
> static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
> struct userfaultfd_ctx *new,
> struct uffd_msg *msg)
> @@ -1920,7 +1918,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f)
> }
> #endif
>
> -static const struct file_operations userfaultfd_fops = {
> +const struct file_operations userfaultfd_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = userfaultfd_show_fdinfo,
> #endif
These changes can be dropped entirely AFAICT.
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 20d8cf194fb7..de5d37e388df 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -215,6 +215,10 @@
> * Returns 0 if @name and @value have been successfully set,
> * -EOPNOTSUPP if no security attribute is needed, or
> * -ENOMEM on memory allocation failure.
> + * @inode_init_security_anon:
> + * Set up a secure anonymous inode.
> + * Returns 0 on success. Returns -EPERM if the security module denies
> + * the creation of this inode.
I'd favor describing the arguments (@name, @context_inode) too.
> * @inode_create:
> * Check permission to create a regular file.
> * @dir contains inode structure of the parent of the new file.
> @@ -1552,6 +1556,10 @@ union security_list_options {
> const struct qstr *qstr,
> const char **name, void **value,
> size_t *len);
> + int (*inode_init_security_anon)(struct inode *inode,
> + const struct qstr *name,
> + const struct file_operations *fops,
> + const struct inode *context_inode);
> int (*inode_create)(struct inode *dir, struct dentry *dentry,
Can drop the fops argument.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 64b19f050343..8ea76af0be7a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -320,6 +320,10 @@ 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 file_operations *fops,
> + const struct inode *context_inode);
Ditto
> diff --git a/security/security.c b/security/security.c
> index 565bc9b67276..d06f3969c030 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1033,6 +1033,16 @@ 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 file_operations *fops,
> + const struct inode *context_inode)
> +{
> + return call_int_hook(inode_init_security_anon, 0, inode, name,
> + fops, context_inode);
> +}
> +
And again.
On 3/25/20 7:02 PM, Daniel Colascione wrote:
> This change uses the anon_inodes and LSM infrastructure introduced in
> the previous patch to give SELinux the ability to control
> anonymous-inode files that are created using the new _secure()
> anon_inodes functions.
>
> A SELinux policy author detects and controls these anonymous inodes by
> adding a name-based type_transition rule that assigns a new security
> type to anonymous-inode files created in some domain. The name used
> for the name-based transition is the name associated with the
> anonymous inode for file listings --- e.g., "[userfaultfd]" or
> "[perf_event]".
>
> Example:
>
> type uffd_t;
> type_transition sysadm_t sysadm_t : file uffd_t "[userfaultfd]";
> allow sysadm_t uffd_t:file { 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]>
> ---
> security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 2 ++
> 2 files changed, 56 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1659b59fb5d7..b9eb45c2e4e5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2915,6 +2915,59 @@ 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 file_operations *fops,
> + const struct inode *context_inode)
> +{
> + const struct task_security_struct *tsec = selinux_cred(current_cred());
> + struct common_audit_data ad;
> + struct inode_security_struct *isec;
> + int rc;
> +
> + if (unlikely(!selinux_state.initialized))
> + return 0;
This leaves secure anon inodes created before first policy load with the
unlabeled SID rather than defaulting to the SID of the creating task
(kernel SID in that situation). Is that what you want? Alternatively
you can just remove this test and let it proceed; nothing should be
break and the anon inodes will get the kernel SID.
> +
> + isec = selinux_inode(inode);
> +
> + /*
> + * We only get here once per ephemeral inode. The inode has
> + * been initialized via inode_alloc_security but is otherwise
> + * untouched.
> + */
> +
> + if (context_inode) {
> + struct inode_security_struct *context_isec =
> + selinux_inode(context_inode);
> + isec->sclass = context_isec->sclass;
> + isec->sid = context_isec->sid;
> + } else {
> + isec->sclass = SECCLASS_ANON_INODE;
> + rc = security_transition_sid(
> + &selinux_state, tsec->sid, tsec->sid,
> + SECCLASS_FILE, name, &isec->sid);
> + if (rc)
> + return rc;
> + }
> +
> + isec->initialized = LABEL_INITIALIZED;
> +
> + /*
> + * Now that we've initialized security, check whether we're
> + * allowed to actually create this type of anonymous inode.
> + */
> +
> + ad.type = LSM_AUDIT_DATA_INODE;
> + ad.u.inode = inode;
> +
> + return avc_has_perm(&selinux_state,
> + tsec->sid,
> + isec->sid,
> + isec->sclass,
> + FILE__CREATE,
> + &ad);
> +}
> +
> static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
> {
> return may_create(dir, dentry, SECCLASS_FILE);
> @@ -6923,6 +6976,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 986f3ac14282..263750b6aaac 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -248,6 +248,8 @@ struct security_class_mapping secclass_map[] = {
> {"open", "cpu", "kernel", "tracepoint", "read", "write"} },
> { "lockdown",
> { "integrity", "confidentiality", NULL } },
> + { "anon_inode",
> + { COMMON_FILE_PERMS, NULL } },
> { NULL }
> };
>
>
On 3/25/20 7:02 PM, Daniel Colascione wrote:
> This change uses the anon_inodes and LSM infrastructure introduced in
> the previous patch to give SELinux the ability to control
> anonymous-inode files that are created using the new _secure()
> anon_inodes functions.
>
> A SELinux policy author detects and controls these anonymous inodes by
> adding a name-based type_transition rule that assigns a new security
> type to anonymous-inode files created in some domain. The name used
> for the name-based transition is the name associated with the
> anonymous inode for file listings --- e.g., "[userfaultfd]" or
> "[perf_event]".
>
> Example:
>
> type uffd_t;
> type_transition sysadm_t sysadm_t : file uffd_t "[userfaultfd]";
> allow sysadm_t uffd_t:file { create };
These should use :anon_inode rather than :file since the class is no
longer file.
>
> (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]>
> ---
> security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 2 ++
> 2 files changed, 56 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1659b59fb5d7..b9eb45c2e4e5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2915,6 +2915,59 @@ 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 file_operations *fops,
> + const struct inode *context_inode)
> +{
> + const struct task_security_struct *tsec = selinux_cred(current_cred());
> + struct common_audit_data ad;
> + struct inode_security_struct *isec;
> + int rc;
> +
> + if (unlikely(!selinux_state.initialized))
> + return 0;
> +
> + isec = selinux_inode(inode);
> +
> + /*
> + * We only get here once per ephemeral inode. The inode has
> + * been initialized via inode_alloc_security but is otherwise
> + * untouched.
> + */
> +
> + if (context_inode) {
> + struct inode_security_struct *context_isec =
> + selinux_inode(context_inode);
> + isec->sclass = context_isec->sclass;
> + isec->sid = context_isec->sid;
> + } else {
> + isec->sclass = SECCLASS_ANON_INODE;
> + rc = security_transition_sid(
> + &selinux_state, tsec->sid, tsec->sid,
> + SECCLASS_FILE, name, &isec->sid);
You should use isec->sclass == SECCLASS_ANON_INODE instead of
SECCLASS_FILE here.
Thanks for taking a look!
On Thu, Mar 26, 2020 at 6:57 AM Stephen Smalley <[email protected]> wrote:
>
> On 3/25/20 7:02 PM, Daniel Colascione wrote:
> > This change uses the anon_inodes and LSM infrastructure introduced in
> > the previous patch to give SELinux the ability to control
> > anonymous-inode files that are created using the new _secure()
> > anon_inodes functions.
> >
> > A SELinux policy author detects and controls these anonymous inodes by
> > adding a name-based type_transition rule that assigns a new security
> > type to anonymous-inode files created in some domain. The name used
> > for the name-based transition is the name associated with the
> > anonymous inode for file listings --- e.g., "[userfaultfd]" or
> > "[perf_event]".
> >
> > Example:
> >
> > type uffd_t;
> > type_transition sysadm_t sysadm_t : file uffd_t "[userfaultfd]";
> > allow sysadm_t uffd_t:file { create };
Oops. Will fix.
> > (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]>
> > ---
> > security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++
> > security/selinux/include/classmap.h | 2 ++
> > 2 files changed, 56 insertions(+)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 1659b59fb5d7..b9eb45c2e4e5 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2915,6 +2915,59 @@ 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 file_operations *fops,
> > + const struct inode *context_inode)
> > +{
> > + const struct task_security_struct *tsec = selinux_cred(current_cred());
> > + struct common_audit_data ad;
> > + struct inode_security_struct *isec;
> > + int rc;
> > +
> > + if (unlikely(!selinux_state.initialized))
> > + return 0;
>
> This leaves secure anon inodes created before first policy load with the
> unlabeled SID rather than defaulting to the SID of the creating task
> (kernel SID in that situation). Is that what you want? Alternatively
> you can just remove this test and let it proceed; nothing should be
> break and the anon inodes will get the kernel SID.
We talked about this decision on the last thread [1], and I think you
mentioned that either the unlabeled or the kernel SID approach would
be defensible. Using the unlabeled SID seems more "honest" to me than
using the kernel SID: the unlabeled SID says "we don't know", while
using kernel SID would be making an affirmative claim that the
anonymous inode belongs to the kernel, and claim wouldn't be true.
That's why I'm leaning toward the unlabeled approach right now.
[1] https://lore.kernel.org/lkml/[email protected]/
Userfaultfd in unprivileged contexts could be potentially very
useful. We'd like to harden userfaultfd to make such unprivileged use
less risky. This patch series allows SELinux to manage userfaultfd
file descriptors and in the future, other kinds of
anonymous-inode-based file descriptor. SELinux policy authors can
apply policy types to anonymous inodes by providing name-based
transition rules keyed off the anonymous inode internal name (
"[userfaultfd]" in the case of userfaultfd(2) file descriptors) and
applying policy to the new SIDs thus produced.
Inside the kernel, a pair of new anon_inodes interface,
anon_inode_getfile_secure and anon_inode_getfd_secure, allow callers
to opt into this SELinux management. In this new "secure" mode,
anon_inodes creates new ephemeral inodes for anonymous file objects
instead of reusing the normal anon_inodes singleton dummy inode. A new
LSM hook gives security modules an opportunity to configure and veto
these ephemeral inodes.
This patch series is one of two fork of [1] and is an
alternative to [2].
The primary difference between the two patch series is that this
partch series creates a unique inode for each "secure" anonymous
inode, while the other patch series ([2]) continues using the
singleton dummy anonymous inode and adds a way to attach SELinux
security information directly to file objects.
I prefer the approach in this patch series because 1) it's a smaller
patch than [2], and 2) it produces a more regular security
architecture: in this patch series, secure anonymous inodes aren't
S_PRIVATE and they maintain the SELinux property that the label for a
file is in its inode. We do need an additional inode per anonymous
file, but per-struct-file inode creation doesn't seem to be a problem
for pipes and sockets.
The previous version of this feature ([1]) created a new SELinux
security class for userfaultfd file descriptors. This version adopts
the generic transition-based approach of [2].
This patch series also differs from [2] in that it doesn't affect all
anonymous inodes right away --- instead requiring anon_inodes callers
to opt in --- but this difference isn't one of basic approach. The
important question to resolve is whether we should be creating new
inodes or enhancing per-file data.
Changes from the first version of the patch:
- Removed some error checks
- Defined a new anon_inode SELinux class to resolve the
ambiguity in [3]
- Inherit sclass as well as descriptor from context inode
Changes from the second version of the patch:
- Fixed example policy in the commit message to reflect the use of
the new anon_inode class.
[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):
Add a new LSM-supporting anonymous inode interface
Teach SELinux about anonymous inodes
Wire UFFD up to SELinux
fs/anon_inodes.c | 196 ++++++++++++++++++++++------
fs/userfaultfd.c | 30 ++++-
include/linux/anon_inodes.h | 13 ++
include/linux/lsm_hooks.h | 9 ++
include/linux/security.h | 4 +
security/security.c | 10 ++
security/selinux/hooks.c | 54 ++++++++
security/selinux/include/classmap.h | 2 +
8 files changed, 272 insertions(+), 46 deletions(-)
--
2.25.1.696.g5e7596f4ac-goog
This change adds two new functions, anon_inode_getfile_secure and
anon_inode_getfd_secure, that create anonymous-node files with
individual non-S_PRIVATE inodes to which security modules can apply
policy. Existing callers continue using the original singleton-inode
kind of anonymous-inode file. We can transition anonymous inode users
to the new kind of anonymous inode in individual patches for the sake
of bisection and review.
The new functions accept an optional context_inode parameter that
callers can use to provide additional contextual information to
security modules, e.g., indicating that one anonymous struct file is a
logical child of another, allowing a security model to propagate
security information from one to the other.
Signed-off-by: Daniel Colascione <[email protected]>
---
fs/anon_inodes.c | 196 ++++++++++++++++++++++++++++--------
fs/userfaultfd.c | 4 +-
include/linux/anon_inodes.h | 13 +++
include/linux/lsm_hooks.h | 9 ++
include/linux/security.h | 4 +
security/security.c | 10 ++
6 files changed, 191 insertions(+), 45 deletions(-)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b..114a04fc1db4 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -55,75 +55,135 @@ static struct file_system_type anon_inode_fs_type = {
.kill_sb = kill_anon_super,
};
-/**
- * anon_inode_getfile - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
- *
- * @name: [in] name of the "class" of the new file
- * @fops: [in] file operations for the new file
- * @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
- *
- * Creates a new file by hooking it on a single inode. This is useful for files
- * that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfile() will share a single inode,
- * hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns the newly created file* or an error pointer.
- */
-struct file *anon_inode_getfile(const char *name,
- const struct file_operations *fops,
- void *priv, int flags)
+static struct inode *anon_inode_make_secure_inode(
+ const char *name,
+ const struct inode *context_inode,
+ const struct file_operations *fops)
+{
+ struct inode *inode;
+ const struct qstr qname = QSTR_INIT(name, strlen(name));
+ int error;
+
+ inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
+ if (IS_ERR(inode))
+ return ERR_PTR(PTR_ERR(inode));
+ inode->i_flags &= ~S_PRIVATE;
+ error = security_inode_init_security_anon(
+ inode, &qname, fops, context_inode);
+ if (error) {
+ iput(inode);
+ return ERR_PTR(error);
+ }
+ return inode;
+}
+
+struct file *_anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
+ struct inode *inode;
struct file *file;
- if (IS_ERR(anon_inode_inode))
- return ERR_PTR(-ENODEV);
+ if (secure) {
+ inode = anon_inode_make_secure_inode(
+ name, context_inode, fops);
+ if (IS_ERR(inode))
+ return ERR_PTR(PTR_ERR(inode));
+ } else {
+ inode = anon_inode_inode;
+ if (IS_ERR(inode))
+ return ERR_PTR(-ENODEV);
+ /*
+ * We know the anon_inode inode count is always
+ * greater than zero, so ihold() is safe.
+ */
+ ihold(inode);
+ }
- if (fops->owner && !try_module_get(fops->owner))
- return ERR_PTR(-ENOENT);
+ if (fops->owner && !try_module_get(fops->owner)) {
+ file = ERR_PTR(-ENOENT);
+ goto err;
+ }
- /*
- * We know the anon_inode inode count is always greater than zero,
- * so ihold() is safe.
- */
- ihold(anon_inode_inode);
- file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name,
+ file = alloc_file_pseudo(inode, anon_inode_mnt, name,
flags & (O_ACCMODE | O_NONBLOCK), fops);
if (IS_ERR(file))
goto err;
- file->f_mapping = anon_inode_inode->i_mapping;
+ file->f_mapping = inode->i_mapping;
file->private_data = priv;
return file;
err:
- iput(anon_inode_inode);
+ iput(inode);
module_put(fops->owner);
return file;
}
-EXPORT_SYMBOL_GPL(anon_inode_getfile);
/**
- * anon_inode_getfd - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
+ * anon_inode_getfile_secure - creates a new file instance by hooking
+ * it up to a new anonymous inode and a
+ * dentry that describe the "class" of the
+ * file. Make it possible to use security
+ * modules to control access to the
+ * new file.
*
* @name: [in] name of the "class" of the new file
* @fops: [in] file operations for the new file
* @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
+ * @flags: [in] flags for the file
+ * @anon_inode_flags: [in] flags for anon_inode*
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns the
+ * newly created file* or an error pointer.
+ */
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfile(
+ name, fops, priv, flags, context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile_secure);
+
+/**
+ * anon_inode_getfile - creates a new file instance by hooking it up
+ * to an anonymous inode and a dentry that
+ * describe the "class" of the file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags for the file
*
- * Creates a new file by hooking it on a single inode. This is useful for files
+ * Creates a new file by hooking it on an unspecified inode. This is useful for files
* that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfd() will share a single inode,
+ * All the files created with anon_inode_getfile() will share a single inode,
* hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns new descriptor or an error code.
+ * setup. Returns the newly created file* or an error pointer.
*/
-int anon_inode_getfd(const char *name, const struct file_operations *fops,
- void *priv, int flags)
+struct file *anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfile(name, fops, priv, flags, NULL, false);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile);
+
+static int _anon_inode_getfd(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
int error, fd;
struct file *file;
@@ -133,7 +193,8 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
return error;
fd = error;
- file = anon_inode_getfile(name, fops, priv, flags);
+ file = _anon_inode_getfile(name, fops, priv, flags, context_inode,
+ secure);
if (IS_ERR(file)) {
error = PTR_ERR(file);
goto err_put_unused_fd;
@@ -146,6 +207,57 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
put_unused_fd(fd);
return error;
}
+
+/**
+ * anon_inode_getfd_secure - creates a new file instance by hooking it
+ * up to a new anonymous inode and a dentry
+ * that describe the "class" of the file.
+ * Make it possible to use security modules
+ * to control access to the new file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns a newly
+ * created file descriptor or an error code.
+ */
+int anon_inode_getfd_secure(const char *name, const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfd(name, fops, priv, flags,
+ context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);
+
+/**
+ * anon_inode_getfd - creates a new file instance by hooking it up to
+ * an anonymous inode and a dentry that describe
+ * the "class" of the file
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile() will use the same singleton inode, reducing
+ * memory use and avoiding code duplication for the file/inode/dentry
+ * setup. Returns a newly created file descriptor or an error code.
+ */
+int anon_inode_getfd(const char *name, const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfd(name, fops, priv, flags, NULL, false);
+}
EXPORT_SYMBOL_GPL(anon_inode_getfd);
static int __init anon_inode_init(void)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 37df7c9eedb1..07b0f6e03849 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1014,8 +1014,6 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
}
}
-static const struct file_operations userfaultfd_fops;
-
static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
struct userfaultfd_ctx *new,
struct uffd_msg *msg)
@@ -1920,7 +1918,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f)
}
#endif
-static const struct file_operations userfaultfd_fops = {
+const struct file_operations userfaultfd_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = userfaultfd_show_fdinfo,
#endif
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261ad..67bd85d92dca 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -10,12 +10,25 @@
#define _LINUX_ANON_INODES_H
struct file_operations;
+struct inode;
+
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+
+int anon_inode_getfd_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
+
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
+
#endif /* _LINUX_ANON_INODES_H */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 20d8cf194fb7..de5d37e388df 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -215,6 +215,10 @@
* Returns 0 if @name and @value have been successfully set,
* -EOPNOTSUPP if no security attribute is needed, or
* -ENOMEM on memory allocation failure.
+ * @inode_init_security_anon:
+ * Set up a secure anonymous inode.
+ * Returns 0 on success. Returns -EPERM if the security module denies
+ * the creation of this inode.
* @inode_create:
* Check permission to create a regular file.
* @dir contains inode structure of the parent of the new file.
@@ -1552,6 +1556,10 @@ union security_list_options {
const struct qstr *qstr,
const char **name, void **value,
size_t *len);
+ int (*inode_init_security_anon)(struct inode *inode,
+ const struct qstr *name,
+ const struct file_operations *fops,
+ const struct inode *context_inode);
int (*inode_create)(struct inode *dir, struct dentry *dentry,
umode_t mode);
int (*inode_link)(struct dentry *old_dentry, struct inode *dir,
@@ -1884,6 +1892,7 @@ struct security_hook_heads {
struct hlist_head inode_alloc_security;
struct hlist_head inode_free_security;
struct hlist_head inode_init_security;
+ struct hlist_head inode_init_security_anon;
struct hlist_head inode_create;
struct hlist_head inode_link;
struct hlist_head inode_unlink;
diff --git a/include/linux/security.h b/include/linux/security.h
index 64b19f050343..8ea76af0be7a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -320,6 +320,10 @@ 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 file_operations *fops,
+ const struct inode *context_inode);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len);
diff --git a/security/security.c b/security/security.c
index 565bc9b67276..d06f3969c030 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1033,6 +1033,16 @@ 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 file_operations *fops,
+ const struct inode *context_inode)
+{
+ return call_int_hook(inode_init_security_anon, 0, inode, name,
+ fops, 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.25.1.696.g5e7596f4ac-goog
This change gives userfaultfd file descriptors a real security
context, allowing policy to act on them.
Signed-off-by: Daniel Colascione <[email protected]>
---
fs/userfaultfd.c | 34 +++++++++++++++++++++++++++++-----
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 07b0f6e03849..78ff5d898733 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -76,6 +76,8 @@ struct userfaultfd_ctx {
bool mmap_changing;
/* mm with one ore more vmas attached to this userfaultfd_ctx */
struct mm_struct *mm;
+ /* The inode that owns this context --- not a strong reference. */
+ const struct inode *owner;
};
struct userfaultfd_fork_ctx {
@@ -1014,14 +1016,18 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
}
}
+static const struct file_operations userfaultfd_fops;
+
static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
struct userfaultfd_ctx *new,
struct uffd_msg *msg)
{
int fd;
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, new,
- O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS));
+ fd = anon_inode_getfd_secure(
+ "[userfaultfd]", &userfaultfd_fops, new,
+ O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS),
+ ctx->owner);
if (fd < 0)
return fd;
@@ -1918,7 +1924,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f)
}
#endif
-const struct file_operations userfaultfd_fops = {
+static const struct file_operations userfaultfd_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = userfaultfd_show_fdinfo,
#endif
@@ -1943,6 +1949,7 @@ static void init_once_userfaultfd_ctx(void *mem)
SYSCALL_DEFINE1(userfaultfd, int, flags)
{
+ struct file *file;
struct userfaultfd_ctx *ctx;
int fd;
@@ -1972,8 +1979,25 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
/* prevent the mm struct to be freed */
mmgrab(ctx->mm);
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, ctx,
- O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS));
+ file = anon_inode_getfile_secure(
+ "[userfaultfd]", &userfaultfd_fops, ctx,
+ O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS),
+ NULL);
+ if (IS_ERR(file)) {
+ fd = PTR_ERR(file);
+ goto out;
+ }
+
+ fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
+ if (fd < 0) {
+ fput(file);
+ goto out;
+ }
+
+ ctx->owner = file_inode(file);
+ fd_install(fd, file);
+
+out:
if (fd < 0) {
mmdrop(ctx->mm);
kmem_cache_free(userfaultfd_ctx_cachep, ctx);
--
2.25.1.696.g5e7596f4ac-goog
This change uses the anon_inodes and LSM infrastructure introduced in
the previous patch to give SELinux the ability to control
anonymous-inode files that are created using the new _secure()
anon_inodes functions.
A SELinux policy author detects and controls these anonymous inodes by
adding a name-based type_transition rule that assigns a new security
type to anonymous-inode files created in some domain. The name used
for the name-based transition is the name associated with the
anonymous inode for file listings --- e.g., "[userfaultfd]" or
"[perf_event]".
Example:
type uffd_t;
type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
allow sysadm_t uffd_t:anon_inode { create };
(The next patch in this series is necessary for making userfaultfd
support this new interface. The example above is just
for exposition.)
Signed-off-by: Daniel Colascione <[email protected]>
---
security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 ++
2 files changed, 56 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1659b59fb5d7..b9eb45c2e4e5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2915,6 +2915,59 @@ 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 file_operations *fops,
+ const struct inode *context_inode)
+{
+ const struct task_security_struct *tsec = selinux_cred(current_cred());
+ struct common_audit_data ad;
+ struct inode_security_struct *isec;
+ int rc;
+
+ if (unlikely(!selinux_state.initialized))
+ return 0;
+
+ isec = selinux_inode(inode);
+
+ /*
+ * We only get here once per ephemeral inode. The inode has
+ * been initialized via inode_alloc_security but is otherwise
+ * untouched.
+ */
+
+ if (context_inode) {
+ struct inode_security_struct *context_isec =
+ selinux_inode(context_inode);
+ isec->sclass = context_isec->sclass;
+ isec->sid = context_isec->sid;
+ } else {
+ isec->sclass = SECCLASS_ANON_INODE;
+ rc = security_transition_sid(
+ &selinux_state, tsec->sid, tsec->sid,
+ SECCLASS_FILE, name, &isec->sid);
+ if (rc)
+ return rc;
+ }
+
+ isec->initialized = LABEL_INITIALIZED;
+
+ /*
+ * Now that we've initialized security, check whether we're
+ * allowed to actually create this type of anonymous inode.
+ */
+
+ ad.type = LSM_AUDIT_DATA_INODE;
+ ad.u.inode = inode;
+
+ return avc_has_perm(&selinux_state,
+ tsec->sid,
+ isec->sid,
+ isec->sclass,
+ FILE__CREATE,
+ &ad);
+}
+
static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
{
return may_create(dir, dentry, SECCLASS_FILE);
@@ -6923,6 +6976,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 986f3ac14282..263750b6aaac 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -248,6 +248,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.25.1.696.g5e7596f4ac-goog
On 3/26/20 2:14 PM, Daniel Colascione wrote:
> This change adds two new functions, anon_inode_getfile_secure and
> anon_inode_getfd_secure, that create anonymous-node files with
> individual non-S_PRIVATE inodes to which security modules can apply
> policy. Existing callers continue using the original singleton-inode
> kind of anonymous-inode file. We can transition anonymous inode users
> to the new kind of anonymous inode in individual patches for the sake
> of bisection and review.
>
> The new functions accept an optional context_inode parameter that
> callers can use to provide additional contextual information to
> security modules, e.g., indicating that one anonymous struct file is a
> logical child of another, allowing a security model to propagate
> security information from one to the other.
>
> Signed-off-by: Daniel Colascione <[email protected]>
> ---
Repeating verbatim what I said on v2 of patch 1/3.
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 89714308c25b..114a04fc1db4 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -55,75 +55,135 @@ static struct file_system_type anon_inode_fs_type = {
> .kill_sb = kill_anon_super,
> };
>
> -/**
> - * anon_inode_getfile - creates a new file instance by hooking it up to an
> - * anonymous inode, and a dentry that describe the "class"
> - * of the file
> - *
> - * @name: [in] name of the "class" of the new file
> - * @fops: [in] file operations for the new file
> - * @priv: [in] private data for the new file (will be file's private_data)
> - * @flags: [in] flags
> - *
> - * Creates a new file by hooking it on a single inode. This is useful for files
> - * that do not need to have a full-fledged inode in order to operate correctly.
> - * All the files created with anon_inode_getfile() will share a single inode,
> - * hence saving memory and avoiding code duplication for the file/inode/dentry
> - * setup. Returns the newly created file* or an error pointer.
> - */
> -struct file *anon_inode_getfile(const char *name,
> - const struct file_operations *fops,
> - void *priv, int flags)
> +static struct inode *anon_inode_make_secure_inode(
> + const char *name,
> + const struct inode *context_inode,
> + const struct file_operations *fops)
> +{
> + struct inode *inode;
> + const struct qstr qname = QSTR_INIT(name, strlen(name));
> + int error;
> +
> + inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
> + if (IS_ERR(inode))
> + return ERR_PTR(PTR_ERR(inode));
Just return inode here ?
> + inode->i_flags &= ~S_PRIVATE;
> + error = security_inode_init_security_anon(
> + inode, &qname, fops, context_inode);
Would drop fops argument until we have a real user for it; we can always
add it later.
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 37df7c9eedb1..07b0f6e03849 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1014,8 +1014,6 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
> }
> }
>
> -static const struct file_operations userfaultfd_fops;
> -
> static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
> struct userfaultfd_ctx *new,
> struct uffd_msg *msg)
> @@ -1920,7 +1918,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f)
> }
> #endif
>
> -static const struct file_operations userfaultfd_fops = {
> +const struct file_operations userfaultfd_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = userfaultfd_show_fdinfo,
> #endif
These changes are unnecessary for this patch and reverted by patch 3, so
drop them here.
On 3/26/20 2:14 PM, Daniel Colascione wrote:
> This change uses the anon_inodes and LSM infrastructure introduced in
> the previous patch to give SELinux the ability to control
> anonymous-inode files that are created using the new _secure()
> anon_inodes functions.
>
> A SELinux policy author detects and controls these anonymous inodes by
> adding a name-based type_transition rule that assigns a new security
> type to anonymous-inode files created in some domain. The name used
> for the name-based transition is the name associated with the
> anonymous inode for file listings --- e.g., "[userfaultfd]" or
> "[perf_event]".
>
> Example:
>
> type uffd_t;
> type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
> allow sysadm_t uffd_t:anon_inode { create };
>
> (The next patch in this series is necessary for making userfaultfd
> support this new interface. The example above is just
> for exposition.)
>
> Signed-off-by: Daniel Colascione <[email protected]>
> ---
> security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 2 ++
> 2 files changed, 56 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1659b59fb5d7..b9eb45c2e4e5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2915,6 +2915,59 @@ 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 file_operations *fops,
> + const struct inode *context_inode)
> +{
> + const struct task_security_struct *tsec = selinux_cred(current_cred());
> + struct common_audit_data ad;
> + struct inode_security_struct *isec;
> + int rc;
> +
> + if (unlikely(!selinux_state.initialized))
> + return 0;
> +
> + isec = selinux_inode(inode);
> +
> + /*
> + * We only get here once per ephemeral inode. The inode has
> + * been initialized via inode_alloc_security but is otherwise
> + * untouched.
> + */
> +
> + if (context_inode) {
> + struct inode_security_struct *context_isec =
> + selinux_inode(context_inode);
> + isec->sclass = context_isec->sclass;
> + isec->sid = context_isec->sid;
> + } else {
> + isec->sclass = SECCLASS_ANON_INODE;
> + rc = security_transition_sid(
> + &selinux_state, tsec->sid, tsec->sid,
> + SECCLASS_FILE, name, &isec->sid);
I guess you missed the 2nd comment in my reply to v2 of this patch:
You should use isec->sclass aka SECCLASS_ANON_INODE instead of
SECCLASS_FILE here.
Userfaultfd in unprivileged contexts could be potentially very
useful. We'd like to harden userfaultfd to make such unprivileged use
less risky. This patch series allows SELinux to manage userfaultfd
file descriptors and in the future, other kinds of
anonymous-inode-based file descriptor. SELinux policy authors can
apply policy types to anonymous inodes by providing name-based
transition rules keyed off the anonymous inode internal name (
"[userfaultfd]" in the case of userfaultfd(2) file descriptors) and
applying policy to the new SIDs thus produced.
Inside the kernel, a pair of new anon_inodes interface,
anon_inode_getfile_secure and anon_inode_getfd_secure, allow callers
to opt into this SELinux management. In this new "secure" mode,
anon_inodes creates new ephemeral inodes for anonymous file objects
instead of reusing the normal anon_inodes singleton dummy inode. A new
LSM hook gives security modules an opportunity to configure and veto
these ephemeral inodes.
This patch series is one of two fork of [1] and is an
alternative to [2].
The primary difference between the two patch series is that this
partch series creates a unique inode for each "secure" anonymous
inode, while the other patch series ([2]) continues using the
singleton dummy anonymous inode and adds a way to attach SELinux
security information directly to file objects.
I prefer the approach in this patch series because 1) it's a smaller
patch than [2], and 2) it produces a more regular security
architecture: in this patch series, secure anonymous inodes aren't
S_PRIVATE and they maintain the SELinux property that the label for a
file is in its inode. We do need an additional inode per anonymous
file, but per-struct-file inode creation doesn't seem to be a problem
for pipes and sockets.
The previous version of this feature ([1]) created a new SELinux
security class for userfaultfd file descriptors. This version adopts
the generic transition-based approach of [2].
This patch series also differs from [2] in that it doesn't affect all
anonymous inodes right away --- instead requiring anon_inodes callers
to opt in --- but this difference isn't one of basic approach. The
important question to resolve is whether we should be creating new
inodes or enhancing per-file data.
Changes from the first version of the patch:
- Removed some error checks
- Defined a new anon_inode SELinux class to resolve the
ambiguity in [3]
- Inherit sclass as well as descriptor from context inode
Changes from the second version of the patch:
- Fixed example policy in the commit message to reflect the use of
the new anon_inode class.
Changes from the third version of the patch:
- Dropped the fops parameter to the LSM hook
- Documented hook parameters
- Fixed incorrect class used for SELinux transition
- Removed stray UFFD changed early in the series
- Removed a redundant ERR_PTR(PTR_ERR())
[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):
Add a new LSM-supporting anonymous inode interface
Teach SELinux about anonymous inodes
Wire UFFD up to SELinux
fs/anon_inodes.c | 196 ++++++++++++++++++++++------
fs/userfaultfd.c | 30 ++++-
include/linux/anon_inodes.h | 13 ++
include/linux/lsm_hooks.h | 11 ++
include/linux/security.h | 3 +
security/security.c | 9 ++
security/selinux/hooks.c | 53 ++++++++
security/selinux/include/classmap.h | 2 +
8 files changed, 271 insertions(+), 46 deletions(-)
--
2.25.1.696.g5e7596f4ac-goog
This change adds two new functions, anon_inode_getfile_secure and
anon_inode_getfd_secure, that create anonymous-node files with
individual non-S_PRIVATE inodes to which security modules can apply
policy. Existing callers continue using the original singleton-inode
kind of anonymous-inode file. We can transition anonymous inode users
to the new kind of anonymous inode in individual patches for the sake
of bisection and review.
The new functions accept an optional context_inode parameter that
callers can use to provide additional contextual information to
security modules, e.g., indicating that one anonymous struct file is a
logical child of another, allowing a security model to propagate
security information from one to the other.
Signed-off-by: Daniel Colascione <[email protected]>
---
fs/anon_inodes.c | 196 ++++++++++++++++++++++++++++--------
include/linux/anon_inodes.h | 13 +++
include/linux/lsm_hooks.h | 11 ++
include/linux/security.h | 3 +
security/security.c | 9 ++
5 files changed, 190 insertions(+), 42 deletions(-)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b..024059e333dc 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -55,75 +55,135 @@ static struct file_system_type anon_inode_fs_type = {
.kill_sb = kill_anon_super,
};
-/**
- * anon_inode_getfile - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
- *
- * @name: [in] name of the "class" of the new file
- * @fops: [in] file operations for the new file
- * @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
- *
- * Creates a new file by hooking it on a single inode. This is useful for files
- * that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfile() will share a single inode,
- * hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns the newly created file* or an error pointer.
- */
-struct file *anon_inode_getfile(const char *name,
- const struct file_operations *fops,
- void *priv, int flags)
+static struct inode *anon_inode_make_secure_inode(
+ const char *name,
+ const struct inode *context_inode,
+ const struct file_operations *fops)
+{
+ struct inode *inode;
+ const struct qstr qname = QSTR_INIT(name, strlen(name));
+ int error;
+
+ inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
+ if (IS_ERR(inode))
+ return inode;
+ inode->i_flags &= ~S_PRIVATE;
+ error = security_inode_init_security_anon(
+ inode, &qname, context_inode);
+ if (error) {
+ iput(inode);
+ return ERR_PTR(error);
+ }
+ return inode;
+}
+
+struct file *_anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
+ struct inode *inode;
struct file *file;
- if (IS_ERR(anon_inode_inode))
- return ERR_PTR(-ENODEV);
+ if (secure) {
+ inode = anon_inode_make_secure_inode(
+ name, context_inode, fops);
+ if (IS_ERR(inode))
+ return ERR_PTR(PTR_ERR(inode));
+ } else {
+ inode = anon_inode_inode;
+ if (IS_ERR(inode))
+ return ERR_PTR(-ENODEV);
+ /*
+ * We know the anon_inode inode count is always
+ * greater than zero, so ihold() is safe.
+ */
+ ihold(inode);
+ }
- if (fops->owner && !try_module_get(fops->owner))
- return ERR_PTR(-ENOENT);
+ if (fops->owner && !try_module_get(fops->owner)) {
+ file = ERR_PTR(-ENOENT);
+ goto err;
+ }
- /*
- * We know the anon_inode inode count is always greater than zero,
- * so ihold() is safe.
- */
- ihold(anon_inode_inode);
- file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name,
+ file = alloc_file_pseudo(inode, anon_inode_mnt, name,
flags & (O_ACCMODE | O_NONBLOCK), fops);
if (IS_ERR(file))
goto err;
- file->f_mapping = anon_inode_inode->i_mapping;
+ file->f_mapping = inode->i_mapping;
file->private_data = priv;
return file;
err:
- iput(anon_inode_inode);
+ iput(inode);
module_put(fops->owner);
return file;
}
-EXPORT_SYMBOL_GPL(anon_inode_getfile);
/**
- * anon_inode_getfd - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
+ * anon_inode_getfile_secure - creates a new file instance by hooking
+ * it up to a new anonymous inode and a
+ * dentry that describe the "class" of the
+ * file. Make it possible to use security
+ * modules to control access to the
+ * new file.
*
* @name: [in] name of the "class" of the new file
* @fops: [in] file operations for the new file
* @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
+ * @flags: [in] flags for the file
+ * @anon_inode_flags: [in] flags for anon_inode*
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns the
+ * newly created file* or an error pointer.
+ */
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfile(
+ name, fops, priv, flags, context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile_secure);
+
+/**
+ * anon_inode_getfile - creates a new file instance by hooking it up
+ * to an anonymous inode and a dentry that
+ * describe the "class" of the file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags for the file
*
- * Creates a new file by hooking it on a single inode. This is useful for files
+ * Creates a new file by hooking it on an unspecified inode. This is useful for files
* that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfd() will share a single inode,
+ * All the files created with anon_inode_getfile() will share a single inode,
* hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns new descriptor or an error code.
+ * setup. Returns the newly created file* or an error pointer.
*/
-int anon_inode_getfd(const char *name, const struct file_operations *fops,
- void *priv, int flags)
+struct file *anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfile(name, fops, priv, flags, NULL, false);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile);
+
+static int _anon_inode_getfd(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
int error, fd;
struct file *file;
@@ -133,7 +193,8 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
return error;
fd = error;
- file = anon_inode_getfile(name, fops, priv, flags);
+ file = _anon_inode_getfile(name, fops, priv, flags, context_inode,
+ secure);
if (IS_ERR(file)) {
error = PTR_ERR(file);
goto err_put_unused_fd;
@@ -146,6 +207,57 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
put_unused_fd(fd);
return error;
}
+
+/**
+ * anon_inode_getfd_secure - creates a new file instance by hooking it
+ * up to a new anonymous inode and a dentry
+ * that describe the "class" of the file.
+ * Make it possible to use security modules
+ * to control access to the new file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns a newly
+ * created file descriptor or an error code.
+ */
+int anon_inode_getfd_secure(const char *name, const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfd(name, fops, priv, flags,
+ context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);
+
+/**
+ * anon_inode_getfd - creates a new file instance by hooking it up to
+ * an anonymous inode and a dentry that describe
+ * the "class" of the file
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile() will use the same singleton inode, reducing
+ * memory use and avoiding code duplication for the file/inode/dentry
+ * setup. Returns a newly created file descriptor or an error code.
+ */
+int anon_inode_getfd(const char *name, const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfd(name, fops, priv, flags, NULL, false);
+}
EXPORT_SYMBOL_GPL(anon_inode_getfd);
static int __init anon_inode_init(void)
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261ad..67bd85d92dca 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -10,12 +10,25 @@
#define _LINUX_ANON_INODES_H
struct file_operations;
+struct inode;
+
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+
+int anon_inode_getfd_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
+
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
+
#endif /* _LINUX_ANON_INODES_H */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 20d8cf194fb7..5434c1d285b2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -215,6 +215,13 @@
* Returns 0 if @name and @value have been successfully set,
* -EOPNOTSUPP if no security attribute is needed, or
* -ENOMEM on memory allocation failure.
+ * @inode_init_security_anon:
+ * Set up a secure anonymous inode.
+ * @inode contains the inode structure
+ * @name name of the anonymous inode class
+ * @context_inode optional related inode
+ * Returns 0 on success. Returns -EPERM if the security module denies
+ * the creation of this inode.
* @inode_create:
* Check permission to create a regular file.
* @dir contains inode structure of the parent of the new file.
@@ -1552,6 +1559,9 @@ union security_list_options {
const struct qstr *qstr,
const char **name, void **value,
size_t *len);
+ int (*inode_init_security_anon)(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode);
int (*inode_create)(struct inode *dir, struct dentry *dentry,
umode_t mode);
int (*inode_link)(struct dentry *old_dentry, struct inode *dir,
@@ -1884,6 +1894,7 @@ struct security_hook_heads {
struct hlist_head inode_alloc_security;
struct hlist_head inode_free_security;
struct hlist_head inode_init_security;
+ struct hlist_head inode_init_security_anon;
struct hlist_head inode_create;
struct hlist_head inode_link;
struct hlist_head inode_unlink;
diff --git a/include/linux/security.h b/include/linux/security.h
index 64b19f050343..2108c3ce0666 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -320,6 +320,9 @@ void security_inode_free(struct inode *inode);
int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
+int security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len);
diff --git a/security/security.c b/security/security.c
index 565bc9b67276..70bfebada024 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1033,6 +1033,15 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
}
EXPORT_SYMBOL(security_inode_init_security);
+int
+security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode)
+{
+ return call_int_hook(inode_init_security_anon, 0, inode, name,
+ context_inode);
+}
+
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len)
--
2.25.1.696.g5e7596f4ac-goog
On 3/26/20 4:06 PM, Daniel Colascione wrote:
> This change adds two new functions, anon_inode_getfile_secure and
> anon_inode_getfd_secure, that create anonymous-node files with
> individual non-S_PRIVATE inodes to which security modules can apply
> policy. Existing callers continue using the original singleton-inode
> kind of anonymous-inode file. We can transition anonymous inode users
> to the new kind of anonymous inode in individual patches for the sake
> of bisection and review.
>
> The new functions accept an optional context_inode parameter that
> callers can use to provide additional contextual information to
> security modules, e.g., indicating that one anonymous struct file is a
> logical child of another, allowing a security model to propagate
> security information from one to the other.
>
> Signed-off-by: Daniel Colascione <[email protected]>
> ---
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 89714308c25b..024059e333dc 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -55,75 +55,135 @@ static struct file_system_type anon_inode_fs_type = {
> +static struct inode *anon_inode_make_secure_inode(
> + const char *name,
> + const struct inode *context_inode,
> + const struct file_operations *fops)
fops argument can be removed here too, unused now by this function.
> /**
> - * anon_inode_getfd - creates a new file instance by hooking it up to an
> - * anonymous inode, and a dentry that describe the "class"
> - * of the file
> + * anon_inode_getfile_secure - creates a new file instance by hooking
> + * it up to a new anonymous inode and a
> + * dentry that describe the "class" of the
> + * file. Make it possible to use security
> + * modules to control access to the
> + * new file.
> *
> * @name: [in] name of the "class" of the new file
> * @fops: [in] file operations for the new file
> * @priv: [in] private data for the new file (will be file's private_data)
> - * @flags: [in] flags
> + * @flags: [in] flags for the file
> + * @anon_inode_flags: [in] flags for anon_inode*
anon_inode_flags leftover from prior version of the patch, not an
argument in the current code. Likewise, the "for the file" addendum to
the @flags argument description is a leftover and not needed.
> + * Creates a new file by hooking it on an unspecified inode. This is
> + * useful for files that do not need to have a full-fledged inode in
> + * order to operate correctly. All the files created with
> + * anon_inode_getfile_secure() will have distinct inodes, avoiding
> + * code duplication for the file/inode/dentry setup.
The two preceding sentences directly contradict each other.
> +/**
> + * anon_inode_getfile - creates a new file instance by hooking it up
> + * to an anonymous inode and a dentry that
> + * describe the "class" of the file.
This would be identical to the original except for different word
wrapping. Probably a leftover from prior version of the patch where you
were modifying the existing interface. Recommend reverting such changes
to minimize unnecessary noise in your diff and meke it easier to tell
what changes are real.
> + *
> + * @name: [in] name of the "class" of the new file
> + * @fops: [in] file operations for the new file
> + * @priv: [in] private data for the new file (will be file's private_data)
> + * @flags: [in] flags for the file
> *
> - * Creates a new file by hooking it on a single inode. This is useful for files
> + * Creates a new file by hooking it on an unspecified inode. This is useful for files
Unnecessary difference here; this interface does use a single inode.
> @@ -146,6 +207,57 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
> put_unused_fd(fd);
> return error;
> }
> +
> +/**
> + * anon_inode_getfd_secure - creates a new file instance by hooking it
> + * up to a new anonymous inode and a dentry
> + * that describe the "class" of the file.
> + * Make it possible to use security modules
> + * to control access to the new file.
> + *
> + * @name: [in] name of the "class" of the new file
> + * @fops: [in] file operations for the new file
> + * @priv: [in] private data for the new file (will be file's private_data)
> + * @flags: [in] flags
> + *
> + * Creates a new file by hooking it on an unspecified inode. This is
> + * useful for files that do not need to have a full-fledged inode in
> + * order to operate correctly. All the files created with
> + * anon_inode_getfile_secure() will have distinct inodes, avoiding
> + * code duplication for the file/inode/dentry setup.
The two preceding sentences contradict each other.
Userfaultfd in unprivileged contexts could be potentially very
useful. We'd like to harden userfaultfd to make such unprivileged use
less risky. This patch series allows SELinux to manage userfaultfd
file descriptors and in the future, other kinds of
anonymous-inode-based file descriptor. SELinux policy authors can
apply policy types to anonymous inodes by providing name-based
transition rules keyed off the anonymous inode internal name (
"[userfaultfd]" in the case of userfaultfd(2) file descriptors) and
applying policy to the new SIDs thus produced.
Inside the kernel, a pair of new anon_inodes interface,
anon_inode_getfile_secure and anon_inode_getfd_secure, allow callers
to opt into this SELinux management. In this new "secure" mode,
anon_inodes creates new ephemeral inodes for anonymous file objects
instead of reusing the normal anon_inodes singleton dummy inode. A new
LSM hook gives security modules an opportunity to configure and veto
these ephemeral inodes.
This patch series is one of two fork of [1] and is an
alternative to [2].
The primary difference between the two patch series is that this
partch series creates a unique inode for each "secure" anonymous
inode, while the other patch series ([2]) continues using the
singleton dummy anonymous inode and adds a way to attach SELinux
security information directly to file objects.
I prefer the approach in this patch series because 1) it's a smaller
patch than [2], and 2) it produces a more regular security
architecture: in this patch series, secure anonymous inodes aren't
S_PRIVATE and they maintain the SELinux property that the label for a
file is in its inode. We do need an additional inode per anonymous
file, but per-struct-file inode creation doesn't seem to be a problem
for pipes and sockets.
The previous version of this feature ([1]) created a new SELinux
security class for userfaultfd file descriptors. This version adopts
the generic transition-based approach of [2].
This patch series also differs from [2] in that it doesn't affect all
anonymous inodes right away --- instead requiring anon_inodes callers
to opt in --- but this difference isn't one of basic approach. The
important question to resolve is whether we should be creating new
inodes or enhancing per-file data.
Changes from the first version of the patch:
- Removed some error checks
- Defined a new anon_inode SELinux class to resolve the
ambiguity in [3]
- Inherit sclass as well as descriptor from context inode
Changes from the second version of the patch:
- Fixed example policy in the commit message to reflect the use of
the new anon_inode class.
Changes from the third version of the patch:
- Dropped the fops parameter to the LSM hook
- Documented hook parameters
- Fixed incorrect class used for SELinux transition
- Removed stray UFFD changed early in the series
- Removed a redundant ERR_PTR(PTR_ERR())
Changes from the fourth version of the patch:
- Removed an unused parameter from an internal function
- Fixed function documentation
[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):
Add a new LSM-supporting anonymous inode interface
Teach SELinux about anonymous inodes
Wire UFFD up to SELinux
fs/anon_inodes.c | 191 ++++++++++++++++++++++------
fs/userfaultfd.c | 30 ++++-
include/linux/anon_inodes.h | 13 ++
include/linux/lsm_hooks.h | 11 ++
include/linux/security.h | 3 +
security/security.c | 9 ++
security/selinux/hooks.c | 53 ++++++++
security/selinux/include/classmap.h | 2 +
8 files changed, 267 insertions(+), 45 deletions(-)
--
2.26.0.rc2.310.g2932bb562d-goog
This change gives userfaultfd file descriptors a real security
context, allowing policy to act on them.
Signed-off-by: Daniel Colascione <[email protected]>
---
fs/userfaultfd.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 37df7c9eedb1..78ff5d898733 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -76,6 +76,8 @@ struct userfaultfd_ctx {
bool mmap_changing;
/* mm with one ore more vmas attached to this userfaultfd_ctx */
struct mm_struct *mm;
+ /* The inode that owns this context --- not a strong reference. */
+ const struct inode *owner;
};
struct userfaultfd_fork_ctx {
@@ -1022,8 +1024,10 @@ static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
{
int fd;
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, new,
- O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS));
+ fd = anon_inode_getfd_secure(
+ "[userfaultfd]", &userfaultfd_fops, new,
+ O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS),
+ ctx->owner);
if (fd < 0)
return fd;
@@ -1945,6 +1949,7 @@ static void init_once_userfaultfd_ctx(void *mem)
SYSCALL_DEFINE1(userfaultfd, int, flags)
{
+ struct file *file;
struct userfaultfd_ctx *ctx;
int fd;
@@ -1974,8 +1979,25 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
/* prevent the mm struct to be freed */
mmgrab(ctx->mm);
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, ctx,
- O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS));
+ file = anon_inode_getfile_secure(
+ "[userfaultfd]", &userfaultfd_fops, ctx,
+ O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS),
+ NULL);
+ if (IS_ERR(file)) {
+ fd = PTR_ERR(file);
+ goto out;
+ }
+
+ fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
+ if (fd < 0) {
+ fput(file);
+ goto out;
+ }
+
+ ctx->owner = file_inode(file);
+ fd_install(fd, file);
+
+out:
if (fd < 0) {
mmdrop(ctx->mm);
kmem_cache_free(userfaultfd_ctx_cachep, ctx);
--
2.26.0.rc2.310.g2932bb562d-goog
This change uses the anon_inodes and LSM infrastructure introduced in
the previous patch to give SELinux the ability to control
anonymous-inode files that are created using the new _secure()
anon_inodes functions.
A SELinux policy author detects and controls these anonymous inodes by
adding a name-based type_transition rule that assigns a new security
type to anonymous-inode files created in some domain. The name used
for the name-based transition is the name associated with the
anonymous inode for file listings --- e.g., "[userfaultfd]" or
"[perf_event]".
Example:
type uffd_t;
type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
allow sysadm_t uffd_t:anon_inode { create };
(The next patch in this series is necessary for making userfaultfd
support this new interface. The example above is just
for exposition.)
Signed-off-by: Daniel Colascione <[email protected]>
---
security/selinux/hooks.c | 53 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 ++
2 files changed, 55 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1659b59fb5d7..6f7222d2e404 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2915,6 +2915,58 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
return 0;
}
+static int selinux_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode)
+{
+ const struct task_security_struct *tsec = selinux_cred(current_cred());
+ struct common_audit_data ad;
+ struct inode_security_struct *isec;
+ int rc;
+
+ if (unlikely(!selinux_state.initialized))
+ return 0;
+
+ isec = selinux_inode(inode);
+
+ /*
+ * We only get here once per ephemeral inode. The inode has
+ * been initialized via inode_alloc_security but is otherwise
+ * untouched.
+ */
+
+ if (context_inode) {
+ struct inode_security_struct *context_isec =
+ selinux_inode(context_inode);
+ isec->sclass = context_isec->sclass;
+ isec->sid = context_isec->sid;
+ } else {
+ isec->sclass = SECCLASS_ANON_INODE;
+ rc = security_transition_sid(
+ &selinux_state, tsec->sid, tsec->sid,
+ isec->sclass, name, &isec->sid);
+ if (rc)
+ return rc;
+ }
+
+ isec->initialized = LABEL_INITIALIZED;
+
+ /*
+ * Now that we've initialized security, check whether we're
+ * allowed to actually create this type of anonymous inode.
+ */
+
+ ad.type = LSM_AUDIT_DATA_INODE;
+ ad.u.inode = inode;
+
+ return avc_has_perm(&selinux_state,
+ tsec->sid,
+ isec->sid,
+ isec->sclass,
+ FILE__CREATE,
+ &ad);
+}
+
static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
{
return may_create(dir, dentry, SECCLASS_FILE);
@@ -6923,6 +6975,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 986f3ac14282..263750b6aaac 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -248,6 +248,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.26.0.rc2.310.g2932bb562d-goog
This change adds two new functions, anon_inode_getfile_secure and
anon_inode_getfd_secure, that create anonymous-node files with
individual non-S_PRIVATE inodes to which security modules can apply
policy. Existing callers continue using the original singleton-inode
kind of anonymous-inode file. We can transition anonymous inode users
to the new kind of anonymous inode in individual patches for the sake
of bisection and review.
The new functions accept an optional context_inode parameter that
callers can use to provide additional contextual information to
security modules, e.g., indicating that one anonymous struct file is a
logical child of another, allowing a security model to propagate
security information from one to the other.
Signed-off-by: Daniel Colascione <[email protected]>
---
fs/anon_inodes.c | 191 ++++++++++++++++++++++++++++--------
include/linux/anon_inodes.h | 13 +++
include/linux/lsm_hooks.h | 11 +++
include/linux/security.h | 3 +
security/security.c | 9 ++
5 files changed, 186 insertions(+), 41 deletions(-)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b..f87f221167cf 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -55,61 +55,108 @@ static struct file_system_type anon_inode_fs_type = {
.kill_sb = kill_anon_super,
};
-/**
- * anon_inode_getfile - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
- *
- * @name: [in] name of the "class" of the new file
- * @fops: [in] file operations for the new file
- * @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
- *
- * Creates a new file by hooking it on a single inode. This is useful for files
- * that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfile() will share a single inode,
- * hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns the newly created file* or an error pointer.
- */
-struct file *anon_inode_getfile(const char *name,
- const struct file_operations *fops,
- void *priv, int flags)
+static struct inode *anon_inode_make_secure_inode(
+ const char *name,
+ const struct inode *context_inode)
+{
+ struct inode *inode;
+ const struct qstr qname = QSTR_INIT(name, strlen(name));
+ int error;
+
+ inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
+ if (IS_ERR(inode))
+ return inode;
+ inode->i_flags &= ~S_PRIVATE;
+ error = security_inode_init_security_anon(
+ inode, &qname, context_inode);
+ if (error) {
+ iput(inode);
+ return ERR_PTR(error);
+ }
+ return inode;
+}
+
+struct file *_anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
+ struct inode *inode;
struct file *file;
- if (IS_ERR(anon_inode_inode))
- return ERR_PTR(-ENODEV);
+ if (secure) {
+ inode = anon_inode_make_secure_inode(
+ name, context_inode);
+ if (IS_ERR(inode))
+ return ERR_PTR(PTR_ERR(inode));
+ } else {
+ inode = anon_inode_inode;
+ if (IS_ERR(inode))
+ return ERR_PTR(-ENODEV);
+ /*
+ * We know the anon_inode inode count is always
+ * greater than zero, so ihold() is safe.
+ */
+ ihold(inode);
+ }
- if (fops->owner && !try_module_get(fops->owner))
- return ERR_PTR(-ENOENT);
+ if (fops->owner && !try_module_get(fops->owner)) {
+ file = ERR_PTR(-ENOENT);
+ goto err;
+ }
- /*
- * We know the anon_inode inode count is always greater than zero,
- * so ihold() is safe.
- */
- ihold(anon_inode_inode);
- file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name,
+ file = alloc_file_pseudo(inode, anon_inode_mnt, name,
flags & (O_ACCMODE | O_NONBLOCK), fops);
if (IS_ERR(file))
goto err;
- file->f_mapping = anon_inode_inode->i_mapping;
+ file->f_mapping = inode->i_mapping;
file->private_data = priv;
return file;
err:
- iput(anon_inode_inode);
+ iput(inode);
module_put(fops->owner);
return file;
}
-EXPORT_SYMBOL_GPL(anon_inode_getfile);
/**
- * anon_inode_getfd - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
+ * anon_inode_getfile_secure - creates a new file instance by hooking
+ * it up to a new anonymous inode and a
+ * dentry that describe the "class" of the
+ * file. Make it possible to use security
+ * modules to control access to the
+ * new file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged filesystem
+ * to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns the
+ * newly created file* or an error pointer.
+ */
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfile(
+ name, fops, priv, flags, context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile_secure);
+
+/**
+ * anon_inode_getfile - creates a new file instance by hooking it up to an
+ * anonymous inode, and a dentry that describe the "class"
+ * of the file
*
* @name: [in] name of the "class" of the new file
* @fops: [in] file operations for the new file
@@ -118,12 +165,23 @@ EXPORT_SYMBOL_GPL(anon_inode_getfile);
*
* Creates a new file by hooking it on a single inode. This is useful for files
* that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfd() will share a single inode,
+ * All the files created with anon_inode_getfile() will share a single inode,
* hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns new descriptor or an error code.
+ * setup. Returns the newly created file* or an error pointer.
*/
-int anon_inode_getfd(const char *name, const struct file_operations *fops,
- void *priv, int flags)
+struct file *anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfile(name, fops, priv, flags, NULL, false);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile);
+
+static int _anon_inode_getfd(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
int error, fd;
struct file *file;
@@ -133,7 +191,8 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
return error;
fd = error;
- file = anon_inode_getfile(name, fops, priv, flags);
+ file = _anon_inode_getfile(name, fops, priv, flags, context_inode,
+ secure);
if (IS_ERR(file)) {
error = PTR_ERR(file);
goto err_put_unused_fd;
@@ -146,6 +205,57 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
put_unused_fd(fd);
return error;
}
+
+/**
+ * anon_inode_getfd_secure - creates a new file instance by hooking it
+ * up to a new anonymous inode and a dentry
+ * that describe the "class" of the file.
+ * Make it possible to use security modules
+ * to control access to the new file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged filesystem
+ * to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns a newly
+ * created file descriptor or an error code.
+ */
+int anon_inode_getfd_secure(const char *name, const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfd(name, fops, priv, flags,
+ context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);
+
+/**
+ * anon_inode_getfd - creates a new file instance by hooking it up to
+ * an anonymous inode and a dentry that describe
+ * the "class" of the file
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on a single inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile() will use the same singleton inode, reducing
+ * memory use and avoiding code duplication for the file/inode/dentry
+ * setup. Returns a newly created file descriptor or an error code.
+ */
+int anon_inode_getfd(const char *name, const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfd(name, fops, priv, flags, NULL, false);
+}
EXPORT_SYMBOL_GPL(anon_inode_getfd);
static int __init anon_inode_init(void)
@@ -162,4 +272,3 @@ static int __init anon_inode_init(void)
}
fs_initcall(anon_inode_init);
-
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261ad..67bd85d92dca 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -10,12 +10,25 @@
#define _LINUX_ANON_INODES_H
struct file_operations;
+struct inode;
+
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+
+int anon_inode_getfd_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
+
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
+
#endif /* _LINUX_ANON_INODES_H */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 20d8cf194fb7..5434c1d285b2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -215,6 +215,13 @@
* Returns 0 if @name and @value have been successfully set,
* -EOPNOTSUPP if no security attribute is needed, or
* -ENOMEM on memory allocation failure.
+ * @inode_init_security_anon:
+ * Set up a secure anonymous inode.
+ * @inode contains the inode structure
+ * @name name of the anonymous inode class
+ * @context_inode optional related inode
+ * Returns 0 on success. Returns -EPERM if the security module denies
+ * the creation of this inode.
* @inode_create:
* Check permission to create a regular file.
* @dir contains inode structure of the parent of the new file.
@@ -1552,6 +1559,9 @@ union security_list_options {
const struct qstr *qstr,
const char **name, void **value,
size_t *len);
+ int (*inode_init_security_anon)(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode);
int (*inode_create)(struct inode *dir, struct dentry *dentry,
umode_t mode);
int (*inode_link)(struct dentry *old_dentry, struct inode *dir,
@@ -1884,6 +1894,7 @@ struct security_hook_heads {
struct hlist_head inode_alloc_security;
struct hlist_head inode_free_security;
struct hlist_head inode_init_security;
+ struct hlist_head inode_init_security_anon;
struct hlist_head inode_create;
struct hlist_head inode_link;
struct hlist_head inode_unlink;
diff --git a/include/linux/security.h b/include/linux/security.h
index 64b19f050343..2108c3ce0666 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -320,6 +320,9 @@ void security_inode_free(struct inode *inode);
int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
+int security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len);
diff --git a/security/security.c b/security/security.c
index 565bc9b67276..70bfebada024 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1033,6 +1033,15 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
}
EXPORT_SYMBOL(security_inode_init_security);
+int
+security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct inode *context_inode)
+{
+ return call_int_hook(inode_init_security_anon, 0, inode, name,
+ context_inode);
+}
+
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len)
--
2.26.0.rc2.310.g2932bb562d-goog
On Wed, Apr 1, 2020 at 2:39 PM Daniel Colascione <[email protected]> wrote:
>
> Changes from the fourth version of the patch:
Is there anything else that needs to be done before merging this patch series?
On Mon, 13 Apr 2020, Daniel Colascione wrote:
> On Wed, Apr 1, 2020 at 2:39 PM Daniel Colascione <[email protected]> wrote:
> >
> > Changes from the fourth version of the patch:
>
>
> Is there anything else that needs to be done before merging this patch series?
The vfs changes need review and signoff from the vfs folk, the SELinux
changes by either Paul or Stephen, and we also need signoff on the LSM
hooks from other major LSM authors (Casey and John, at a minimum).
--
James Morris
<[email protected]>
On 4/22/2020 9:55 AM, James Morris wrote:
> On Mon, 13 Apr 2020, Daniel Colascione wrote:
>
>> On Wed, Apr 1, 2020 at 2:39 PM Daniel Colascione <[email protected]> wrote:
>>> Changes from the fourth version of the patch:
>>
>> Is there anything else that needs to be done before merging this patch series?
> The vfs changes need review and signoff from the vfs folk, the SELinux
> changes by either Paul or Stephen, and we also need signoff on the LSM
> hooks from other major LSM authors (Casey and John, at a minimum).
I haven't had the opportunity to test this relative to Smack.
It's unclear whether the change would impact security modules that
don't provide hooks for it. I will bump my priority on this, but it's
still going to be a bit before I can get to it.
On 4/22/2020 10:12 AM, Casey Schaufler wrote:
> On 4/22/2020 9:55 AM, James Morris wrote:
>> On Mon, 13 Apr 2020, Daniel Colascione wrote:
>>
>>> On Wed, Apr 1, 2020 at 2:39 PM Daniel Colascione <[email protected]> wrote:
>>>> Changes from the fourth version of the patch:
>>> Is there anything else that needs to be done before merging this patch series?
Do you have a test case that exercises this feature?
On 4/23/2020 3:24 PM, Casey Schaufler wrote:
> On 4/22/2020 10:12 AM, Casey Schaufler wrote:
>> On 4/22/2020 9:55 AM, James Morris wrote:
>>> On Mon, 13 Apr 2020, Daniel Colascione wrote:
>>>
>>>> On Wed, Apr 1, 2020 at 2:39 PM Daniel Colascione <[email protected]> wrote:
>>>>> Changes from the fourth version of the patch:
>>>> Is there anything else that needs to be done before merging this patch series?
> Do you have a test case that exercises this feature?
I haven't heard anything back. What would cause this code to be executed?
On Mon, Apr 27, 2020 at 12:19 PM Casey Schaufler <[email protected]> wrote:
>
> On 4/23/2020 3:24 PM, Casey Schaufler wrote:
> > On 4/22/2020 10:12 AM, Casey Schaufler wrote:
> >> On 4/22/2020 9:55 AM, James Morris wrote:
> >>> On Mon, 13 Apr 2020, Daniel Colascione wrote:
> >>>
> >>>> On Wed, Apr 1, 2020 at 2:39 PM Daniel Colascione <[email protected]> wrote:
> >>>>> Changes from the fourth version of the patch:
> >>>> Is there anything else that needs to be done before merging this patch series?
> > Do you have a test case that exercises this feature?
>
> I haven't heard anything back. What would cause this code to be executed?
See https://lore.kernel.org/selinux/[email protected]/
for example.
On 4/27/2020 9:48 AM, Stephen Smalley wrote:
> On Mon, Apr 27, 2020 at 12:19 PM Casey Schaufler <[email protected]> wrote:
>> On 4/23/2020 3:24 PM, Casey Schaufler wrote:
>>> On 4/22/2020 10:12 AM, Casey Schaufler wrote:
>>>> On 4/22/2020 9:55 AM, James Morris wrote:
>>>>> On Mon, 13 Apr 2020, Daniel Colascione wrote:
>>>>>
>>>>>> On Wed, Apr 1, 2020 at 2:39 PM Daniel Colascione <[email protected]> wrote:
>>>>>>> Changes from the fourth version of the patch:
>>>>>> Is there anything else that needs to be done before merging this patch series?
>>> Do you have a test case that exercises this feature?
>> I haven't heard anything back. What would cause this code to be executed?
> See https://lore.kernel.org/selinux/[email protected]/
> for example.
Great. Thanks, that's what I needed. I'll Ack the patch set.
On 4/22/2020 9:55 AM, James Morris wrote:
> On Mon, 13 Apr 2020, Daniel Colascione wrote:
>
>> On Wed, Apr 1, 2020 at 2:39 PM Daniel Colascione <[email protected]> wrote:
>>> Changes from the fourth version of the patch:
>>
>> Is there anything else that needs to be done before merging this patch series?
> The vfs changes need review and signoff from the vfs folk, the SELinux
> changes by either Paul or Stephen, and we also need signoff on the LSM
> hooks from other major LSM authors (Casey and John, at a minimum).
You can add my
Acked-by: Casey Schaufler <[email protected]>
for this patchset.
On Mon, Apr 27, 2020 at 1:17 PM Casey Schaufler <[email protected]> wrote:
>
> On 4/22/2020 9:55 AM, James Morris wrote:
> > On Mon, 13 Apr 2020, Daniel Colascione wrote:
> >
> >> On Wed, Apr 1, 2020 at 2:39 PM Daniel Colascione <[email protected]> wrote:
> >>> Changes from the fourth version of the patch:
> >>
> >> Is there anything else that needs to be done before merging this patch series?
> > The vfs changes need review and signoff from the vfs folk, the SELinux
> > changes by either Paul or Stephen, and we also need signoff on the LSM
> > hooks from other major LSM authors (Casey and John, at a minimum).
>
> You can add my
>
> Acked-by: Casey Schaufler <[email protected]>
>
> for this patchset.
This version of the series addresses all of my comments, so you can add my
Acked-by: Stephen Smalley <[email protected]>
I don't know though how to get a response from the vfs folks; the
series has been posted repeatedly without any
response by them.
On Mon, Apr 27, 2020 at 12:48 PM Stephen Smalley
<[email protected]> wrote:
>
> On Mon, Apr 27, 2020 at 12:19 PM Casey Schaufler <[email protected]> wrote:
> >
> > On 4/23/2020 3:24 PM, Casey Schaufler wrote:
> > > On 4/22/2020 10:12 AM, Casey Schaufler wrote:
> > >> On 4/22/2020 9:55 AM, James Morris wrote:
> > >>> On Mon, 13 Apr 2020, Daniel Colascione wrote:
> > >>>
> > >>>> On Wed, Apr 1, 2020 at 2:39 PM Daniel Colascione <[email protected]> wrote:
> > >>>>> Changes from the fourth version of the patch:
> > >>>> Is there anything else that needs to be done before merging this patch series?
> > > Do you have a test case that exercises this feature?
> >
> > I haven't heard anything back. What would cause this code to be executed?
>
> See https://lore.kernel.org/selinux/[email protected]/
> for example.
NB The example cited above needs to be tweaked for changes in the
logic from the original RFC patch on which the example was
based. In particular, the userfaultfd CIL policy needs to be updated
to define and use the new anon_inode class and to allow create
permission as follows.
$ cat userfaultfd.cil
(class anon_inode ())
(classcommon anon_inode file)
(classorder (unordered anon_inode))
(type uffd_t)
; Label the UFFD with uffd_t; this can be specialized per domain
(typetransition unconfined_t unconfined_t anon_inode "[userfaultfd]" uffd_t)
(allow unconfined_t uffd_t (anon_inode (create)))
; Permit read() and ioctl() on the UFFD.
; Comment out if you want to test read or basic ioctl enforcement.
(allow unconfined_t uffd_t (anon_inode (read)))
(allow unconfined_t uffd_t (anon_inode (ioctl)))
; Uncomment one of the allowx lines below to test ioctl whitelisting.
; Currently the 1st one is uncommented; comment that out if trying another.
; None
(allowx unconfined_t uffd_t (ioctl anon_inode ((0x00))))
; UFFDIO_API
;(allowx unconfined_t uffd_t (ioctl anon_inode ((0xaa3f))))
On Wed, 1 Apr 2020, Daniel Colascione wrote:
> This change adds two new functions, anon_inode_getfile_secure and
> anon_inode_getfd_secure, that create anonymous-node files with
> individual non-S_PRIVATE inodes to which security modules can apply
> policy. Existing callers continue using the original singleton-inode
> kind of anonymous-inode file. We can transition anonymous inode users
> to the new kind of anonymous inode in individual patches for the sake
> of bisection and review.
>
> The new functions accept an optional context_inode parameter that
> callers can use to provide additional contextual information to
> security modules, e.g., indicating that one anonymous struct file is a
> logical child of another, allowing a security model to propagate
> security information from one to the other.
>
> Signed-off-by: Daniel Colascione <[email protected]>
Al, Andrew, wondering if you could look at these anon inode changes
before we merge this?
> ---
> fs/anon_inodes.c | 191 ++++++++++++++++++++++++++++--------
> include/linux/anon_inodes.h | 13 +++
> include/linux/lsm_hooks.h | 11 +++
> include/linux/security.h | 3 +
> security/security.c | 9 ++
> 5 files changed, 186 insertions(+), 41 deletions(-)
>
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 89714308c25b..f87f221167cf 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -55,61 +55,108 @@ static struct file_system_type anon_inode_fs_type = {
> .kill_sb = kill_anon_super,
> };
>
> -/**
> - * anon_inode_getfile - creates a new file instance by hooking it up to an
> - * anonymous inode, and a dentry that describe the "class"
> - * of the file
> - *
> - * @name: [in] name of the "class" of the new file
> - * @fops: [in] file operations for the new file
> - * @priv: [in] private data for the new file (will be file's private_data)
> - * @flags: [in] flags
> - *
> - * Creates a new file by hooking it on a single inode. This is useful for files
> - * that do not need to have a full-fledged inode in order to operate correctly.
> - * All the files created with anon_inode_getfile() will share a single inode,
> - * hence saving memory and avoiding code duplication for the file/inode/dentry
> - * setup. Returns the newly created file* or an error pointer.
> - */
> -struct file *anon_inode_getfile(const char *name,
> - const struct file_operations *fops,
> - void *priv, int flags)
> +static struct inode *anon_inode_make_secure_inode(
> + const char *name,
> + const struct inode *context_inode)
> +{
> + struct inode *inode;
> + const struct qstr qname = QSTR_INIT(name, strlen(name));
> + int error;
> +
> + inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
> + if (IS_ERR(inode))
> + return inode;
> + inode->i_flags &= ~S_PRIVATE;
> + error = security_inode_init_security_anon(
> + inode, &qname, context_inode);
> + if (error) {
> + iput(inode);
> + return ERR_PTR(error);
> + }
> + return inode;
> +}
> +
> +struct file *_anon_inode_getfile(const char *name,
> + const struct file_operations *fops,
> + void *priv, int flags,
> + const struct inode *context_inode,
> + bool secure)
> {
> + struct inode *inode;
> struct file *file;
>
> - if (IS_ERR(anon_inode_inode))
> - return ERR_PTR(-ENODEV);
> + if (secure) {
> + inode = anon_inode_make_secure_inode(
> + name, context_inode);
> + if (IS_ERR(inode))
> + return ERR_PTR(PTR_ERR(inode));
> + } else {
> + inode = anon_inode_inode;
> + if (IS_ERR(inode))
> + return ERR_PTR(-ENODEV);
> + /*
> + * We know the anon_inode inode count is always
> + * greater than zero, so ihold() is safe.
> + */
> + ihold(inode);
> + }
>
> - if (fops->owner && !try_module_get(fops->owner))
> - return ERR_PTR(-ENOENT);
> + if (fops->owner && !try_module_get(fops->owner)) {
> + file = ERR_PTR(-ENOENT);
> + goto err;
> + }
>
> - /*
> - * We know the anon_inode inode count is always greater than zero,
> - * so ihold() is safe.
> - */
> - ihold(anon_inode_inode);
> - file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name,
> + file = alloc_file_pseudo(inode, anon_inode_mnt, name,
> flags & (O_ACCMODE | O_NONBLOCK), fops);
> if (IS_ERR(file))
> goto err;
>
> - file->f_mapping = anon_inode_inode->i_mapping;
> + file->f_mapping = inode->i_mapping;
>
> file->private_data = priv;
>
> return file;
>
> err:
> - iput(anon_inode_inode);
> + iput(inode);
> module_put(fops->owner);
> return file;
> }
> -EXPORT_SYMBOL_GPL(anon_inode_getfile);
>
> /**
> - * anon_inode_getfd - creates a new file instance by hooking it up to an
> - * anonymous inode, and a dentry that describe the "class"
> - * of the file
> + * anon_inode_getfile_secure - creates a new file instance by hooking
> + * it up to a new anonymous inode and a
> + * dentry that describe the "class" of the
> + * file. Make it possible to use security
> + * modules to control access to the
> + * new file.
> + *
> + * @name: [in] name of the "class" of the new file
> + * @fops: [in] file operations for the new file
> + * @priv: [in] private data for the new file (will be file's private_data)
> + * @flags: [in] flags
> + *
> + * Creates a new file by hooking it on an unspecified inode. This is
> + * useful for files that do not need to have a full-fledged filesystem
> + * to operate correctly. All the files created with
> + * anon_inode_getfile_secure() will have distinct inodes, avoiding
> + * code duplication for the file/inode/dentry setup. Returns the
> + * newly created file* or an error pointer.
> + */
> +struct file *anon_inode_getfile_secure(const char *name,
> + const struct file_operations *fops,
> + void *priv, int flags,
> + const struct inode *context_inode)
> +{
> + return _anon_inode_getfile(
> + name, fops, priv, flags, context_inode, true);
> +}
> +EXPORT_SYMBOL_GPL(anon_inode_getfile_secure);
> +
> +/**
> + * anon_inode_getfile - creates a new file instance by hooking it up to an
> + * anonymous inode, and a dentry that describe the "class"
> + * of the file
> *
> * @name: [in] name of the "class" of the new file
> * @fops: [in] file operations for the new file
> @@ -118,12 +165,23 @@ EXPORT_SYMBOL_GPL(anon_inode_getfile);
> *
> * Creates a new file by hooking it on a single inode. This is useful for files
> * that do not need to have a full-fledged inode in order to operate correctly.
> - * All the files created with anon_inode_getfd() will share a single inode,
> + * All the files created with anon_inode_getfile() will share a single inode,
> * hence saving memory and avoiding code duplication for the file/inode/dentry
> - * setup. Returns new descriptor or an error code.
> + * setup. Returns the newly created file* or an error pointer.
> */
> -int anon_inode_getfd(const char *name, const struct file_operations *fops,
> - void *priv, int flags)
> +struct file *anon_inode_getfile(const char *name,
> + const struct file_operations *fops,
> + void *priv, int flags)
> +{
> + return _anon_inode_getfile(name, fops, priv, flags, NULL, false);
> +}
> +EXPORT_SYMBOL_GPL(anon_inode_getfile);
> +
> +static int _anon_inode_getfd(const char *name,
> + const struct file_operations *fops,
> + void *priv, int flags,
> + const struct inode *context_inode,
> + bool secure)
> {
> int error, fd;
> struct file *file;
> @@ -133,7 +191,8 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
> return error;
> fd = error;
>
> - file = anon_inode_getfile(name, fops, priv, flags);
> + file = _anon_inode_getfile(name, fops, priv, flags, context_inode,
> + secure);
> if (IS_ERR(file)) {
> error = PTR_ERR(file);
> goto err_put_unused_fd;
> @@ -146,6 +205,57 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
> put_unused_fd(fd);
> return error;
> }
> +
> +/**
> + * anon_inode_getfd_secure - creates a new file instance by hooking it
> + * up to a new anonymous inode and a dentry
> + * that describe the "class" of the file.
> + * Make it possible to use security modules
> + * to control access to the new file.
> + *
> + * @name: [in] name of the "class" of the new file
> + * @fops: [in] file operations for the new file
> + * @priv: [in] private data for the new file (will be file's private_data)
> + * @flags: [in] flags
> + *
> + * Creates a new file by hooking it on an unspecified inode. This is
> + * useful for files that do not need to have a full-fledged filesystem
> + * to operate correctly. All the files created with
> + * anon_inode_getfile_secure() will have distinct inodes, avoiding
> + * code duplication for the file/inode/dentry setup. Returns a newly
> + * created file descriptor or an error code.
> + */
> +int anon_inode_getfd_secure(const char *name, const struct file_operations *fops,
> + void *priv, int flags,
> + const struct inode *context_inode)
> +{
> + return _anon_inode_getfd(name, fops, priv, flags,
> + context_inode, true);
> +}
> +EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);
> +
> +/**
> + * anon_inode_getfd - creates a new file instance by hooking it up to
> + * an anonymous inode and a dentry that describe
> + * the "class" of the file
> + *
> + * @name: [in] name of the "class" of the new file
> + * @fops: [in] file operations for the new file
> + * @priv: [in] private data for the new file (will be file's private_data)
> + * @flags: [in] flags
> + *
> + * Creates a new file by hooking it on a single inode. This is
> + * useful for files that do not need to have a full-fledged inode in
> + * order to operate correctly. All the files created with
> + * anon_inode_getfile() will use the same singleton inode, reducing
> + * memory use and avoiding code duplication for the file/inode/dentry
> + * setup. Returns a newly created file descriptor or an error code.
> + */
> +int anon_inode_getfd(const char *name, const struct file_operations *fops,
> + void *priv, int flags)
> +{
> + return _anon_inode_getfd(name, fops, priv, flags, NULL, false);
> +}
> EXPORT_SYMBOL_GPL(anon_inode_getfd);
>
> static int __init anon_inode_init(void)
> @@ -162,4 +272,3 @@ static int __init anon_inode_init(void)
> }
>
> fs_initcall(anon_inode_init);
> -
> diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
> index d0d7d96261ad..67bd85d92dca 100644
> --- a/include/linux/anon_inodes.h
> +++ b/include/linux/anon_inodes.h
> @@ -10,12 +10,25 @@
> #define _LINUX_ANON_INODES_H
>
> struct file_operations;
> +struct inode;
> +
> +struct file *anon_inode_getfile_secure(const char *name,
> + const struct file_operations *fops,
> + void *priv, int flags,
> + const struct inode *context_inode);
>
> struct file *anon_inode_getfile(const char *name,
> const struct file_operations *fops,
> void *priv, int flags);
> +
> +int anon_inode_getfd_secure(const char *name,
> + const struct file_operations *fops,
> + void *priv, int flags,
> + const struct inode *context_inode);
> +
> int anon_inode_getfd(const char *name, const struct file_operations *fops,
> void *priv, int flags);
>
> +
> #endif /* _LINUX_ANON_INODES_H */
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 20d8cf194fb7..5434c1d285b2 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -215,6 +215,13 @@
> * Returns 0 if @name and @value have been successfully set,
> * -EOPNOTSUPP if no security attribute is needed, or
> * -ENOMEM on memory allocation failure.
> + * @inode_init_security_anon:
> + * Set up a secure anonymous inode.
> + * @inode contains the inode structure
> + * @name name of the anonymous inode class
> + * @context_inode optional related inode
> + * Returns 0 on success. Returns -EPERM if the security module denies
> + * the creation of this inode.
> * @inode_create:
> * Check permission to create a regular file.
> * @dir contains inode structure of the parent of the new file.
> @@ -1552,6 +1559,9 @@ union security_list_options {
> const struct qstr *qstr,
> const char **name, void **value,
> size_t *len);
> + int (*inode_init_security_anon)(struct inode *inode,
> + const struct qstr *name,
> + const struct inode *context_inode);
> int (*inode_create)(struct inode *dir, struct dentry *dentry,
> umode_t mode);
> int (*inode_link)(struct dentry *old_dentry, struct inode *dir,
> @@ -1884,6 +1894,7 @@ struct security_hook_heads {
> struct hlist_head inode_alloc_security;
> struct hlist_head inode_free_security;
> struct hlist_head inode_init_security;
> + struct hlist_head inode_init_security_anon;
> struct hlist_head inode_create;
> struct hlist_head inode_link;
> struct hlist_head inode_unlink;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 64b19f050343..2108c3ce0666 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -320,6 +320,9 @@ void security_inode_free(struct inode *inode);
> int security_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> initxattrs initxattrs, void *fs_data);
> +int security_inode_init_security_anon(struct inode *inode,
> + const struct qstr *name,
> + const struct inode *context_inode);
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> diff --git a/security/security.c b/security/security.c
> index 565bc9b67276..70bfebada024 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1033,6 +1033,15 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> +int
> +security_inode_init_security_anon(struct inode *inode,
> + const struct qstr *name,
> + const struct inode *context_inode)
> +{
> + return call_int_hook(inode_init_security_anon, 0, inode, name,
> + context_inode);
> +}
> +
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr, const char **name,
> void **value, size_t *len)
>
--
James Morris
<[email protected]>
On Wed, 1 Apr 2020, Daniel Colascione wrote:
> Daniel Colascione (3):
> Add a new LSM-supporting anonymous inode interface
> Teach SELinux about anonymous inodes
> Wire UFFD up to SELinux
>
> fs/anon_inodes.c | 191 ++++++++++++++++++++++------
> fs/userfaultfd.c | 30 ++++-
> include/linux/anon_inodes.h | 13 ++
> include/linux/lsm_hooks.h | 11 ++
> include/linux/security.h | 3 +
> security/security.c | 9 ++
> security/selinux/hooks.c | 53 ++++++++
> security/selinux/include/classmap.h | 2 +
> 8 files changed, 267 insertions(+), 45 deletions(-)
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git secure_uffd_v5.9
and next-testing.
This will provide test coverage in linux-next, as we aim to get this
upstream for v5.9.
I had to make some minor fixups, please review.
--
James Morris
<[email protected]>
On Wed, Jun 3, 2020 at 11:59 PM James Morris <[email protected]> wrote:
>
> On Wed, 1 Apr 2020, Daniel Colascione wrote:
>
> > Daniel Colascione (3):
> > Add a new LSM-supporting anonymous inode interface
> > Teach SELinux about anonymous inodes
> > Wire UFFD up to SELinux
> >
> > fs/anon_inodes.c | 191 ++++++++++++++++++++++------
> > fs/userfaultfd.c | 30 ++++-
> > include/linux/anon_inodes.h | 13 ++
> > include/linux/lsm_hooks.h | 11 ++
> > include/linux/security.h | 3 +
> > security/security.c | 9 ++
> > security/selinux/hooks.c | 53 ++++++++
> > security/selinux/include/classmap.h | 2 +
> > 8 files changed, 267 insertions(+), 45 deletions(-)
>
> Applied to
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git secure_uffd_v5.9
> and next-testing.
>
> This will provide test coverage in linux-next, as we aim to get this
> upstream for v5.9.
>
> I had to make some minor fixups, please review.
LGTM and my userfaultfd test case worked.
Adding a colleague from the Android kernel team.
On Thu, Jun 4, 2020 at 11:52 AM Stephen Smalley
<[email protected]> wrote:
>
> On Wed, Jun 3, 2020 at 11:59 PM James Morris <[email protected]> wrote:
> >
> > On Wed, 1 Apr 2020, Daniel Colascione wrote:
> >
> > > Daniel Colascione (3):
> > > Add a new LSM-supporting anonymous inode interface
> > > Teach SELinux about anonymous inodes
> > > Wire UFFD up to SELinux
> > >
> > > fs/anon_inodes.c | 191 ++++++++++++++++++++++------
> > > fs/userfaultfd.c | 30 ++++-
> > > include/linux/anon_inodes.h | 13 ++
> > > include/linux/lsm_hooks.h | 11 ++
> > > include/linux/security.h | 3 +
> > > security/security.c | 9 ++
> > > security/selinux/hooks.c | 53 ++++++++
> > > security/selinux/include/classmap.h | 2 +
> > > 8 files changed, 267 insertions(+), 45 deletions(-)
> >
> > Applied to
> > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git secure_uffd_v5.9
> > and next-testing.
> >
> > This will provide test coverage in linux-next, as we aim to get this
> > upstream for v5.9.
> >
> > I had to make some minor fixups, please review.
>
> LGTM and my userfaultfd test case worked.
On Wed, Apr 01, 2020 at 02:39:03PM -0700, Daniel Colascione wrote:
> This change gives userfaultfd file descriptors a real security
> context, allowing policy to act on them.
>
> Signed-off-by: Daniel Colascione <[email protected]>
> ---
> fs/userfaultfd.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 37df7c9eedb1..78ff5d898733 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -76,6 +76,8 @@ struct userfaultfd_ctx {
> bool mmap_changing;
> /* mm with one ore more vmas attached to this userfaultfd_ctx */
> struct mm_struct *mm;
> + /* The inode that owns this context --- not a strong reference. */
> + const struct inode *owner;
> };
Adding this field seems wrong. There's no reference held to it, so it can only
be used if the caller holds a reference to the inode anyway. The only user of
this field is via userfafultfd_read(), so why not just use the inode of the
struct file passed to userfaultfd_read()?
> SYSCALL_DEFINE1(userfaultfd, int, flags)
> {
> + struct file *file;
> struct userfaultfd_ctx *ctx;
> int fd;
>
> @@ -1974,8 +1979,25 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
> /* prevent the mm struct to be freed */
> mmgrab(ctx->mm);
>
> - fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, ctx,
> - O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS));
> + file = anon_inode_getfile_secure(
> + "[userfaultfd]", &userfaultfd_fops, ctx,
> + O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS),
> + NULL);
> + if (IS_ERR(file)) {
> + fd = PTR_ERR(file);
> + goto out;
> + }
> +
> + fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> + if (fd < 0) {
> + fput(file);
> + goto out;
> + }
> +
> + ctx->owner = file_inode(file);
> + fd_install(fd, file);
> +
> +out:
> if (fd < 0) {
> mmdrop(ctx->mm);
> kmem_cache_free(userfaultfd_ctx_cachep, ctx);
What's the point of anon_inode_getfile_secure()? anon_inode_getfd_secure()
would work here too.
- Eric
On Wed, Apr 01, 2020 at 02:39:01PM -0700, Daniel Colascione wrote:
> This change adds two new functions, anon_inode_getfile_secure and
> anon_inode_getfd_secure, that create anonymous-node files with
> individual non-S_PRIVATE inodes to which security modules can apply
> policy. Existing callers continue using the original singleton-inode
> kind of anonymous-inode file. We can transition anonymous inode users
> to the new kind of anonymous inode in individual patches for the sake
> of bisection and review.
>
> The new functions accept an optional context_inode parameter that
> callers can use to provide additional contextual information to
> security modules, e.g., indicating that one anonymous struct file is a
> logical child of another, allowing a security model to propagate
> security information from one to the other.
>
> Signed-off-by: Daniel Colascione <[email protected]>
> ---
> fs/anon_inodes.c | 191 ++++++++++++++++++++++++++++--------
> include/linux/anon_inodes.h | 13 +++
> include/linux/lsm_hooks.h | 11 +++
> include/linux/security.h | 3 +
> security/security.c | 9 ++
> 5 files changed, 186 insertions(+), 41 deletions(-)
>
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 89714308c25b..f87f221167cf 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -55,61 +55,108 @@ static struct file_system_type anon_inode_fs_type = {
> .kill_sb = kill_anon_super,
> };
>
> -/**
> - * anon_inode_getfile - creates a new file instance by hooking it up to an
> - * anonymous inode, and a dentry that describe the "class"
> - * of the file
> - *
> - * @name: [in] name of the "class" of the new file
> - * @fops: [in] file operations for the new file
> - * @priv: [in] private data for the new file (will be file's private_data)
> - * @flags: [in] flags
> - *
> - * Creates a new file by hooking it on a single inode. This is useful for files
> - * that do not need to have a full-fledged inode in order to operate correctly.
> - * All the files created with anon_inode_getfile() will share a single inode,
> - * hence saving memory and avoiding code duplication for the file/inode/dentry
> - * setup. Returns the newly created file* or an error pointer.
> - */
> -struct file *anon_inode_getfile(const char *name,
> - const struct file_operations *fops,
> - void *priv, int flags)
> +static struct inode *anon_inode_make_secure_inode(
> + const char *name,
> + const struct inode *context_inode)
> +{
> + struct inode *inode;
> + const struct qstr qname = QSTR_INIT(name, strlen(name));
> + int error;
> +
> + inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
> + if (IS_ERR(inode))
> + return inode;
> + inode->i_flags &= ~S_PRIVATE;
> + error = security_inode_init_security_anon(
> + inode, &qname, context_inode);
> + if (error) {
> + iput(inode);
> + return ERR_PTR(error);
> + }
> + return inode;
> +}
> +
> +struct file *_anon_inode_getfile(const char *name,
> + const struct file_operations *fops,
> + void *priv, int flags,
> + const struct inode *context_inode,
> + bool secure)
Unnecessarily global function.
> {
> + struct inode *inode;
> struct file *file;
>
> - if (IS_ERR(anon_inode_inode))
> - return ERR_PTR(-ENODEV);
> + if (secure) {
> + inode = anon_inode_make_secure_inode(
> + name, context_inode);
> + if (IS_ERR(inode))
> + return ERR_PTR(PTR_ERR(inode));
Use ERR_CAST(), not ERR_PTR(PTR_ERR()).
> /**
> - * anon_inode_getfd - creates a new file instance by hooking it up to an
> - * anonymous inode, and a dentry that describe the "class"
> - * of the file
> + * anon_inode_getfile_secure - creates a new file instance by hooking
> + * it up to a new anonymous inode and a
> + * dentry that describe the "class" of the
> + * file. Make it possible to use security
> + * modules to control access to the
> + * new file.
> + *
> + * @name: [in] name of the "class" of the new file
> + * @fops: [in] file operations for the new file
> + * @priv: [in] private data for the new file (will be file's private_data)
> + * @flags: [in] flags
> + *
> + * Creates a new file by hooking it on an unspecified inode. This is
> + * useful for files that do not need to have a full-fledged filesystem
> + * to operate correctly. All the files created with
> + * anon_inode_getfile_secure() will have distinct inodes, avoiding
> + * code duplication for the file/inode/dentry setup. Returns the
> + * newly created file* or an error pointer.
> + */
> +struct file *anon_inode_getfile_secure(const char *name,
> + const struct file_operations *fops,
> + void *priv, int flags,
> + const struct inode *context_inode)
Why copy-and-paste this long comment if it's not even updated to document the
new argument?
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 20d8cf194fb7..5434c1d285b2 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -215,6 +215,13 @@
> * Returns 0 if @name and @value have been successfully set,
> * -EOPNOTSUPP if no security attribute is needed, or
> * -ENOMEM on memory allocation failure.
> + * @inode_init_security_anon:
> + * Set up a secure anonymous inode.
> + * @inode contains the inode structure
> + * @name name of the anonymous inode class
> + * @context_inode optional related inode
> + * Returns 0 on success. Returns -EPERM if the security module denies
> + * the creation of this inode.
Shouldn't it be EACCES?