Message-ID: <CA+EESO4eS4KsqgjmDWdwMozK36GbgTvJsKsC_5NO-pQA3huDWg@mail.gmail.com>
Date:   Wed, 7 Oct 2020 13:28:22 -0700
From:   Lokesh Gidra <lokeshgidra@...gle.com>
To:     Alexander Viro <viro@...iv.linux.org.uk>,
        James Morris <jmorris@...ei.org>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Eric Biggers <ebiggers@...nel.org>
Cc:     "Serge E. Hallyn" <serge@...lyn.com>,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...isplace.org>,
        Daniel Colascione <dancol@...col.org>,
        Kees Cook <keescook@...omium.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        KP Singh <kpsingh@...gle.com>,
        David Howells <dhowells@...hat.com>,
        Thomas Cedeno <thomascedeno@...gle.com>,
        Anders Roxell <anders.roxell@...aro.org>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Matthew Garrett <matthewgarrett@...gle.com>,
        Aaron Goidel <acgoide@...ho.nsa.gov>,
        Randy Dunlap <rdunlap@...radead.org>,
        "Joel Fernandes (Google)" <joel@...lfernandes.org>,
        YueHaibing <yuehaibing@...wei.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Alexey Budankov <alexey.budankov@...ux.intel.com>,
        Adrian Reber <areber@...hat.com>,
        Aleksa Sarai <cyphar@...har.com>,
        Linux FS Devel <linux-fsdevel@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        LSM List <linux-security-module@...r.kernel.org>,
        SElinux list <selinux@...r.kernel.org>,
        Kalesh Singh <kaleshsingh@...gle.com>,
        Calin Juravle <calin@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Nick Kralevich <nnk@...gle.com>,
        Jeffrey Vander Stoep <jeffv@...gle.com>,
        "Cc: Android Kernel" <kernel-team@...roid.com>
Subject: Re: [PATCH v9 0/3] SELinux support for anonymous inodes and UFFD

On Wed, Sep 23, 2020 at 12:33 PM Lokesh Gidra <lokeshgidra@...gle.com> 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@google.com/
> [2] https://lore.kernel.org/linux-fsdevel/20200213194157.5877-1-sds@tycho.nsa.gov/
> [3] https://lore.kernel.org/lkml/23f725ca-5b5a-5938-fcc8-5bbbfc9ba9bc@tycho.nsa.gov/
>
> 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
>

Powered by blists - more mailing lists