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

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

On Thu, Apr 11, 2019 at 7:48 AM Richard Haines
<richard_c_haines@btinternet.com> wrote:
> On Wed, 2019-04-10 at 19:43 -0400, Paul Moore wrote:
> > 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?
>
> Just silently dropping messages. I had thought to remove from list then
> add again, however it might smell a rat so left until owner-selinux
> returns. I can only repeat what Julius Caesar muttered "Infamy! Infamy!
> They've all got it in for me!" (well in Carry On Cleo any way).

I got the brexit jump label gag, but you lost me on the "Carry on
Cleo"; thankfully good ol' Google came to the rescue on that one ;)

On the plus side, your posts seem to be coming through to the list now.

On the negative side I realized when playing with your test changes
that I wasn't building BINDERFS in my test kernels - oops.  I'm fixing
that now, but I might not get a chance to do another test until
tomorrow; at least I can verify that your BINDERFS testing logic works
:)

-- 
paul moore
www.paul-moore.com