Re: [PATCH 1/1] selinux-testsuite: Update binder test applications

From: Paul Moore <paul@paul-moore.com>
To: Richard Haines <richard_c_haines@btinternet.com>
Cc: selinux@vger.kernel.org, tkjos@google.com
Subject: Re: [PATCH 1/1] selinux-testsuite: Update binder test applications
Date: Wed, 10 Apr 2019 11:35:23 -0400	[thread overview]
Message-ID: <CAHC9VhTTeetqR03+zaV0d_2rdHr4r6eKnqKNrXO6k9O7+tz=gQ@mail.gmail.com> (raw)
In-Reply-To: <20190403122611.6543-1-richard_c_haines@btinternet.com>

On Wed, Apr 3, 2019 at 8:43 AM Richard Haines
<richard_c_haines@btinternet.com> wrote:
> Replace binder_test.c with separate manager, client and service provider.
> This works in the same way as a service provider/client interacts
> with a service manager in the Android world. It passes the service
> providers binder file descriptor to the client for the impersonate
> permission check.
>
> Also added tests for Dynamically Allocated Binder Devices and passing
> the sender SELinux security context on binder transactions.
>
> Note that the tests require a minimum kernel of 4.16, else some tests may
> fail. To run successfully the "binder: Add thread->process_todo flag"
> patch may be required that is available from:
> https://lore.kernel.org/patchwork/patch/851324/
> This patch has been backported to some earlier kernels.
>
> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
> ---
>  defconfig                       |   3 +
>  policy/test_binder.te           | 176 ++++----
>  tests/binder/.gitignore         |   6 +-
>  tests/binder/Makefile           |  13 +-
>  tests/binder/binder_common.c    | 155 +++++++
>  tests/binder/binder_common.h    |  37 ++
>  tests/binder/check_binder.c     |  27 +-
>  tests/binder/check_binderfs.c   |  53 +++
>  tests/binder/client.c           | 450 ++++++++++++++++++++
>  tests/binder/manager.c          | 362 ++++++++++++++++
>  tests/binder/service_provider.c | 404 ++++++++++++++++++
>  tests/binder/test               | 257 ++++++++++--
>  tests/binder/test_binder.c      | 705 --------------------------------
>  13 files changed, 1785 insertions(+), 863 deletions(-)
>  create mode 100644 tests/binder/binder_common.c
>  create mode 100644 tests/binder/binder_common.h
>  create mode 100644 tests/binder/check_binderfs.c
>  create mode 100644 tests/binder/client.c
>  create mode 100644 tests/binder/manager.c
>  create mode 100644 tests/binder/service_provider.c
>  delete mode 100644 tests/binder/test_binder.c

Hi Richard,

Welcome back :)

I had hoped to spend some time reading up on Binder so I could give
this a proper review, but that hasn't happened so I'm inclined to
merge it, assuming it works on my test system.  However, considering
your comment about this not working on kernel's older than 4.16, I
think we should probably add some checks to only run this test on
systems with the appropriate kernel support.

If you look at tests/Makefile you will see a number of distro specific
test list modifications, and there is even an example of checking the
kernel version (search for "kvercmp" in the Makefile).  I would
suggest a simple check to make sure the kernel is at least v4.16, and
if we find distro specific support (e.g. a particular distro
backported the listed patch) we can always add an exception for that
distro.

How does that sound?

-- 
paul moore
www.paul-moore.com