Re: [PATCH v9 0/3] SELinux support for anonymous inodes and UFFD

From: Lokesh Gidra
Date: Wed Oct 07 2020 - 16:28:35 EST


On Wed, Sep 23, 2020 at 12:33 PM Lokesh Gidra <lokeshgidra@xxxxxxxxxx> 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 in the future, other kinds of
> anonymous-inode-based file descriptor. SELinux policy authors can
> apply policy types to anonymous inodes by providing name-based
> transition rules keyed off the anonymous inode internal name (
> "[userfaultfd]" in the case of userfaultfd(2) file descriptors) and
> applying policy to the new SIDs thus produced.
>
> With SELinux managed userfaultfd, an admin can control creation and
> movement of the file descriptors. In particular, handling of
> a userfaultfd descriptor by a different process is essentially a
> ptrace access into the process, without any of the corresponding
> security_ptrace_access_check() checks. For privacy, the admin may
> want to deny such accesses, which is possible with SELinux support.
>
> Inside the kernel, a new anon_inode interface, anon_inode_getfd_secure,
> allows callers to opt into this SELinux management. In this new "secure"
> mode, anon_inodes create new ephemeral inodes for anonymous file objects
> instead of reusing the normal anon_inodes singleton dummy inode. A new
> LSM hook gives security modules an opportunity to configure and veto
> these ephemeral inodes.
>
> This patch series is one of two fork of [1] and is an
> alternative to [2].
>
> The primary difference between the two patch series is that this
> partch series creates a unique inode for each "secure" anonymous
> inode, while the other patch series ([2]) continues using the
> singleton dummy anonymous inode and adds a way to attach SELinux
> security information directly to file objects.
>
> I prefer the approach in this patch series because 1) it's a smaller
> patch than [2], and 2) it produces a more regular security
> architecture: in this patch series, secure anonymous inodes aren't
> S_PRIVATE and they maintain the SELinux property that the label for a
> file is in its inode. We do need an additional inode per anonymous
> file, but per-struct-file inode creation doesn't seem to be a problem
> for pipes and sockets.
>
> The previous version of this feature ([1]) created a new SELinux
> security class for userfaultfd file descriptors. This version adopts
> the generic transition-based approach of [2].
>
> This patch series also differs from [2] in that it doesn't affect all
> anonymous inodes right away --- instead requiring anon_inodes callers
> to opt in --- but this difference isn't one of basic approach. The
> important question to resolve is whether we should be creating new
> inodes or enhancing per-file data.
>
> Changes from the first version of the patch:
>
> - Removed some error checks
> - Defined a new anon_inode SELinux class to resolve the
> ambiguity in [3]
> - Inherit sclass as well as descriptor from context inode
>
> Changes from the second version of the patch:
>
> - Fixed example policy in the commit message to reflect the use of
> the new anon_inode class.
>
> Changes from the third version of the patch:
>
> - Dropped the fops parameter to the LSM hook
> - Documented hook parameters
> - Fixed incorrect class used for SELinux transition
> - Removed stray UFFD changed early in the series
> - Removed a redundant ERR_PTR(PTR_ERR())
>
> Changes from the fourth version of the patch:
>
> - Removed an unused parameter from an internal function
> - Fixed function documentation
>
> Changes from the fifth version of the patch:
>
> - Fixed function documentation in fs/anon_inodes.c and
> include/linux/lsm_hooks.h
> - Used anon_inode_getfd_secure() in userfaultfd() syscall and removed
> owner from userfaultfd_ctx.
>
> Changes from the sixth version of the patch:
>
> - Removed definition of anon_inode_getfile_secure() as there are no
> callers.
> - Simplified function description of anon_inode_getfd_secure().
> - Elaborated more on the purpose of 'context_inode' in commit message.
>
> Changes from the seventh version of the patch:
>
> - Fixed error handling in _anon_inode_getfile().
> - Fixed minor comment and indentation related issues.
>
> Changes from the eighth version of the patch:
>
> - Replaced selinux_state.initialized with selinux_state.initialized
>

Is there anything else that needs to be done before merging this
patch series? I urge the reviewers to please take a look.

>
> [1] https://lore.kernel.org/lkml/20200211225547.235083-1-dancol@xxxxxxxxxx/
> [2] https://lore.kernel.org/linux-fsdevel/20200213194157.5877-1-sds@xxxxxxxxxxxxx/
> [3] https://lore.kernel.org/lkml/23f725ca-5b5a-5938-fcc8-5bbbfc9ba9bc@xxxxxxxxxxxxx/
>
> 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 | 147 ++++++++++++++++++++--------
> fs/userfaultfd.c | 19 ++--
> include/linux/anon_inodes.h | 8 ++
> include/linux/lsm_hook_defs.h | 2 +
> include/linux/lsm_hooks.h | 9 ++
> include/linux/security.h | 10 ++
> security/security.c | 8 ++
> security/selinux/hooks.c | 53 ++++++++++
> security/selinux/include/classmap.h | 2 +
> 9 files changed, 209 insertions(+), 49 deletions(-)
>
> --
> 2.28.0.681.g6f77f65b4e-goog
>