Hi Ondrej,
On Fri, Nov 27, 2020 at 6:09 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> Hi Lokesh,
>
> On Wed, Nov 25, 2020 at 1:54 PM Lokesh Gidra <lokeshgidra@xxxxxxxxxx> wrote:
> > Confirm SELinux policies are enforced on userfaultfd operations
> > via secure anon-inode interface.
> >
> > Signed-off-by: Lokesh Gidra <lokeshgidra@xxxxxxxxxx>
> > ---
> > policy/Makefile | 2 +-
> > policy/userfaultfd.cil | 17 ++++++++++++
> > tests/Makefile | 2 +-
> > tests/userfaultfd/Makefile | 3 ++
> > tests/userfaultfd/test | 15 ++++++++++
> > tests/userfaultfd/userfaultfd.c | 49 +++++++++++++++++++++++++++++++++
> > 6 files changed, 86 insertions(+), 2 deletions(-)
> > create mode 100644 policy/userfaultfd.cil
> > create mode 100644 tests/userfaultfd/Makefile
> > create mode 100755 tests/userfaultfd/test
> > create mode 100644 tests/userfaultfd/userfaultfd.c
> >
> > diff --git a/policy/Makefile b/policy/Makefile
> > index 6c49091..02e7568 100644
> > --- a/policy/Makefile
> > +++ b/policy/Makefile
> > @@ -36,7 +36,7 @@ SUPPORTS_CIL = n
> > endif
> >
> > ifeq ($(SUPPORTS_CIL),y)
> > -CIL_TARGETS = test_mlsconstrain.cil test_overlay_defaultrange.cil
> > +CIL_TARGETS = test_mlsconstrain.cil test_overlay_defaultrange.cil userfaultfd.cil
> > ifeq ($(shell [ $(MAX_KERNEL_POLICY) -ge 32 ] && echo true),true)
> > ifeq ($(shell [ $(POL_VERS) -ge 32 ] && echo true),true)
> > # If other MLS tests get written this can be moved outside of the glblub test
> > diff --git a/policy/userfaultfd.cil b/policy/userfaultfd.cil
> > new file mode 100644
> > index 0000000..0743fff
> > --- /dev/null
> > +++ b/policy/userfaultfd.cil
> > @@ -0,0 +1,17 @@
> > +(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 first 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))))
> > diff --git a/tests/Makefile b/tests/Makefile
> > index 4c00b5f..3871570 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -27,7 +27,7 @@ SUBDIRS:= domain_trans entrypoint execshare exectrace execute_no_trans \
> > task_setnice task_setscheduler task_getscheduler task_getsid \
> > task_getpgid task_setpgid file ioctl capable_file capable_net \
> > capable_sys dyntrans dyntrace bounds nnp_nosuid mmap unix_socket \
> > - inet_socket overlay checkreqprot mqueue mac_admin atsecure
> > + inet_socket overlay checkreqprot mqueue mac_admin atsecure userfaultfd
> >
> > ifeq ($(shell grep -q cap_userns $(POLDEV)/include/support/all_perms.spt && echo true),true)
> > ifneq ($(shell ./kvercmp $$(uname -r) 4.7),-1)
> > diff --git a/tests/userfaultfd/Makefile b/tests/userfaultfd/Makefile
> > new file mode 100644
> > index 0000000..66d02a1
> > --- /dev/null
> > +++ b/tests/userfaultfd/Makefile
> > @@ -0,0 +1,3 @@
> > +all: userfaultfd
> > +clean:
> > + rm -f userfaultfd
> > diff --git a/tests/userfaultfd/test b/tests/userfaultfd/test
> > new file mode 100755
> > index 0000000..dd42aa8
> > --- /dev/null
> > +++ b/tests/userfaultfd/test
> > @@ -0,0 +1,15 @@
> > +#!/usr/bin/perl
> > +
> > +use Test;
> > +
> > +BEGIN {
> > + plan tests => 1;
> > +}
> > +
> > +$basedir = $0;
> > +$basedir =~ s|(.*)/[^/]*|$1|;
> > +
> > +$result = system("$basedir/userfaultfd");
> > +ok( $result, 0 );
>
> This is a good start, but it is only a very basic test :)
>
> I would expect at least these tests here:
> 1. that an userfaultfd node created by a domain *without* the
> transition rule gets the context of the creating process (should be
> possible by calling fgetxattr(2) on the fd),
> 2. that an userfaultfd node created by a domain *with* the transition
> rule gets the context specified by the rule,
> 3. that a process with context that is *allowed* to { create read
> write [...] } a given userfaultfd type by the policy *can* indeed
> successfully perform these operations (or at least that the error
> returned is not -EACCESS),
> 4. that a process with context that is *NOT allowed* to { create read
> write [...] } a given userfaultfd type by the policy *cannot* perform
> these operations and the error returned is -EACCESS.
>
> (The more permissions you can test the better, but I'd like to see at
> least the three (create, read, write).)
>
> For this, you will need to define several process types in the test
> policy and run the test program(s) with a specific type using runcon
> (see e.g. policy/test_link.te and tests/link/test for a simple
> example). I suggest using the CIL module only for adding the class and
> permissions and a separate .te file to define the process and
> userfaultfd types (so that you can use the unconfined_runs_test()
> interface for common permissions needed for the process types).
>
I'm having a bit of a hard time managing the test policy in a cil
module and a .te file. My inexperience with SELinux could be a reason
:)
I defined 'anon_inode' class, like in this patch in the CIL module as follows:
(class anon_inode ())
(classcommon anon_inode file)
(classorder (unordered anon_inode))
But then when I used anon_inode in the .te file as follows:
...
type uffd_t;
type_transition test_userfaultfd_t test_userfaultfd_t : anon_inode
uffd_t "[userfaultfd]";
allow test_userfaultfd_t uffd_t:anon_inode { create ioctl read };
I get a compile error:
test_policy.te:<line#>:ERROR 'unknown class anon_inode' at token ';' on line ...
Any advice on where I am making mistakes?
> You will likely also need to communicate different locations of
> failures / error codes from the test program(s) to the script via exit
> code. See tests/keys for an example of such pattern (though you can
> use a different approach as long as it gets the job done :)
>
> Sorry for requiring all the effort, but we're trying to keep
> reasonably high standards for tests here. It's sometimes a PITA, but
> it pays off in the long run ;)
>
> Thanks,
>
> > +
> > +exit;
> > diff --git a/tests/userfaultfd/userfaultfd.c b/tests/userfaultfd/userfaultfd.c
> > new file mode 100644
> > index 0000000..9baebd9
> > --- /dev/null
> > +++ b/tests/userfaultfd/userfaultfd.c
> > @@ -0,0 +1,49 @@
> > +#include <stdio.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +
> > +#include <sys/types.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/syscall.h>
> > +
> > +#include <linux/userfaultfd.h>
> > +
> > +void print_api(const struct uffdio_api *api)
> > +{
> > + printf("api: %llu\n", api->api);
> > + printf("features: %llu\n", api->features);
> > + printf("ioctls: %llu\n", api->ioctls);
> > +
> > + printf("\n");
> > +}
> > +
> > +int main (void)
> > +{
> > + long uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
> > + if (uffd < 0) {
> > + perror("syscall(userfaultfd)");
> > + return -1;
> > + }
> > +
> > + struct uffdio_api api = {0};
> > + api.api = UFFD_API;
> > + if (ioctl(uffd, UFFDIO_API, &api) < 0) {
> > + perror("UFFDIO_API");
> > + return -1;
> > + }
> > +
> > + print_api(&api);
> > +
> > + struct uffd_msg msg = {0};
> > + ssize_t count = read(uffd, &msg, sizeof(msg));
> > + if (count < 0) {
> > + // Expected to fail as no memory range registered.
> > + return 0;
> > + } else if (count == 0) {
> > + printf("read EOF\n\n");
> > + }
> > +
> > + printf("read uffd\n\n");
> > +
> > + return 0;
> > +}
> > --
> > 2.28.0
> >
>
> --
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.
>
