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

[Paul Moore <paul@paul-moore.com>]

On Wed, Apr 10, 2019 at 1:04 PM Richard Haines
<richard_c_haines@btinternet.com> wrote:
> On Wed, 2019-04-10 at 11:35 -0400, Paul Moore wrote:
> > 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?
>
> There are tests for the OS in the 'test' script already. I guess you
> need to check if these are okay, as I check if < 4.16 and if so print
> message saying if fail check for the patch.

Ah ha, yes you did, and I missed it.  Sorry about that.  I checked the
Makefiles, didn't see any checks, and wrongly assumed they were not
there.

Looking quickly at the check it seems reasonable, if I notice any
problems when testing I'll let you know.

> I'm not sure this will get to the list as I appear to be black-balled.
> I did send a cover letter with this patch + another one regarding
> running SCTP on < 4.20.17. I sent email to
> owner-selinux@vger.kernel.org but not heard anything yet.

Hmm, that's not good.  FWIW, the original patch obviously made it, but
yes I'm not seeing your response in the list archives.  Did you get
any sort of majordomo hate mail back on your posts, or is is just
silently dropping your messages?

> Cover Letter:
> Subject: [PATCH 0/1] selinux-testsuite: Update binder test applications
>
> The Binder tests have been rewritten to support the new Dynamically
> Allocated Binder Devices and passing the sender SELinux security
> context on binder transactions.
>
> They have been tested on f29 and rawhide with the following kernels
> from kernel.org:
> mainline: 5.1-rc3
> longterm: 4.19.32
> longterm: 4.14.109
>
> As noted in the main patch, the tests require a minimum kernel of 4.16,
> else some tests may fail (tests 4 & 7). To run successfully the
> 4.14.109 kernel was patched with: "binder: Add thread->process_todo
> flag" available from:
> https://lore.kernel.org/patchwork/patch/851324/
>
> I found that on slow systems using 4.14.109, all tests would pass,
> however when testing on a faster system they would fail. Once patched,
> worked fine.
>
> Any testing feedback gratefully received.
>
> Richard

-- 
paul moore
www.paul-moore.com