Re: [PATCH v1 1/1] selinux-testsuite: Add userfaultfd test

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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).

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.




[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux