On Thu, Nov 12, 2020 at 4:13 PM Paul Moore [off-list ref] wrote:

On Tue, Nov 10, 2020 at 10:30 PM Lokesh Gidra [off-list ref] wrote:

quoted

On Tue, Nov 10, 2020 at 6:13 PM Paul Moore [off-list ref] wrote:

quoted

On Tue, Nov 10, 2020 at 1:24 PM Lokesh Gidra [off-list ref] wrote:

quoted

On Mon, Nov 9, 2020 at 7:12 PM Paul Moore [off-list ref] wrote:

quoted

On Fri, Nov 6, 2020 at 10:56 AM Lokesh Gidra [off-list ref] wrote:

quoted

From: Daniel Colascione <redacted>

This change uses the anon_inodes and LSM infrastructure introduced in
the previous patches to give SELinux the ability to control
anonymous-inode files that are created using the new
anon_inode_getfd_secure() function.

A SELinux policy author detects and controls these anonymous inodes by
adding a name-based type_transition rule that assigns a new security
type to anonymous-inode files created in some domain. The name used
for the name-based transition is the name associated with the
anonymous inode for file listings --- e.g., "[userfaultfd]" or
"[perf_event]".

Example:

type uffd_t;
type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
allow sysadm_t uffd_t:anon_inode { create };

(The next patch in this series is necessary for making userfaultfd
support this new interface.  The example above is just
for exposition.)

Signed-off-by: Daniel Colascione <redacted>
Signed-off-by: Lokesh Gidra <redacted>
---
 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 6b1826fc3658..1c0adcdce7a8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c

@@ -2927,6 +2927,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_initialized(&selinux_state)))
+               return 0;
+
+       isec = selinux_inode(inode);
+
+       /*
+        * We only get here once per ephemeral inode.  The inode has
+        * been initialized via inode_alloc_security but is otherwise
+        * untouched.
+        */
+
+       if (context_inode) {
+               struct inode_security_struct *context_isec =
+                       selinux_inode(context_inode);
+               isec->sclass = context_isec->sclass;
+               isec->sid = context_isec->sid;

I suppose this isn't a major concern given the limited usage at the
moment, but I wonder if it would be a good idea to make sure the
context_inode's SELinux label is valid before we assign it to the
anonymous inode?  If it is invalid, what should we do?  Do we attempt
to (re)validate it?  Do we simply fallback to the transition approach?

Frankly, I'm not too familiar with SELinux. Originally this patch
series was developed by Daniel, in consultation with Stephen Smalley.
In my (probably naive) opinion we should fallback to transition
approach. But I'd request you to tell me if this needs to be addressed
now, and if so then what's the right approach.

If the decision is to address this now, then what's the best way to
check the SELinux label validity?

You can check to see if an inode's label is valid by looking at the
isec->initialized field; if it is LABEL_INITIALIZED then it is all
set, if it is any other value then the label isn't entirely correct.
It may have not have ever been fully initialized (and has a default
value) or it may have live on a remote filesystem where the host has
signaled that the label has changed (and the label is now outdated).

This patchset includes support for userfaultfd, which means we don't
really have to worry about the remote fs problem, but the
never-fully-initialized problem could be real in this case.  Normally
we would revalidate an inode in SELinux by calling
__inode_security_revalidate() which requires either a valid dentry or
one that can be found via the inode; does d_find_alias() work on
userfaultfd inodes?

If all else fails, it seems like the safest approach would be to
simply fail the selinux_inode_init_security_anon() call if a
context_inode was supplied and the label wasn't valid.  If we later
decide to change it to falling back to the transition approach we can
do that, we can't go the other way (from transition to error).

I'm not sure about d_find_alias() on userfaultfd inodes. But it seems
ok to fail selinux_inode_init_security_anon() to begin with.

I'm okay with simply failing here, but I'm growing a bit concerned
that this patchset hasn't been well tested.  That is a problem.

quoted

quoted

quoted

quoted

This brings up another question, and requirement - what testing are
you doing for this patchset?  We require that new SELinux kernel
functionality includes additions to the SELinux test suite to help
verify the functionality.  I'm also *strongly* encouraging that new
contributions come with updates to The SELinux Notebook.  If you are
unsure about what to do for either, let us know and we can help get
you started.

* https://github.com/SELinuxProject/selinux-testsuite
* https://github.com/SELinuxProject/selinux-notebook

I'd definitely need help with both of these. Kindly guide how to proceed.

Well, perhaps the best way to start is to explain how you have been
testing this so far and then using that information to draft a test
for the testsuite.

As I said in my previous reply, Daniel worked on this patch along with
Stephan Smalley. Here's the conversation regarding testing from back
then:
https://lore.kernel.org/lkml/CAEjxPJ4iquFSBfEj+UEFLUFHPsezuQ-Bzv09n+WgOWk38Nyw3w@mail.gmail.com/ (local)

There have been only minor changes (fixing comments/coding-style),
except for addressing a double free issue with userfaultfd_ctx since
last time it was tested as per the link above.

I should probably be more clear.  I honestly don't care who originally
wrote the patch, the simple fact is that you are the one who is
posting it *now* for inclusion in the kernel; at the very least I
expect you to be able to demonstrate that you are able to reliably
test this functionality and prove it is working.  While being able to
test this submission initially is important, it is far more important
to have the tests and docs necessary to maintain this functionality
long term.  Perhaps you and/or Google will continue to contribute and
support this functionality long term, but it would be irresponsible of
me to assume that to be true; both people and companies come and go
but code has a tendency to live forever.

Let's start again; how have you been testing this code?

I have created a cuttlefish build and have tested with the attached
userfaultfd program:

1) Without these kernel patches the program executes without any restrictions

vsoc_x86_64:/ $ ./system/bin/userfaultfdSimple
api: 170
features: 511
ioctls: 9223372036854775811

read: Try again


2) With these patches applied but without any policy the 'permission
denied' is thrown

vsoc_x86_64:/ $ ./system/bin/userfaultfdSimple
syscall(userfaultfd): Permission denied

with the following logcat message:
11-18 14:21:44.041  3130  3130 W userfaultfdSimp: type=1400
audit(0.0:107): avc: denied { create } for dev="anon_inodefs"
ino=45031 scontext=u:r:shell:s0 tcontext=u:object_r:shell:s0
tclass=anon_inode permissive=0


3) With the attached .te policy file in place the following output is
observed, confirming that the patch is working as intended.
vsoc_x86_64:/ $ ./vendor/bin/userfaultfdSimple
UFFDIO_API: Permission denied

with the following logcat message:
11-18 14:33:29.142  2028  2028 W userfaultfdSimp: type=1400
audit(0.0:104): avc: denied { ioctl } for
path="anon_inode:[userfaultfd]" dev="anon_inodefs" ino=41169
ioctlcmd=0xaa3f scontext=u:r:userfaultfdSimple:s0
tcontext=u:object_r:uffd_t:s0 tclass=anon_inode permissive=0

--
paul moore
www.paul-moore.com