Re: v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite

[Todd Kjos <tkjos@google.com>]

On Thu, Mar 21, 2019 at 2:50 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Thu, Mar 21, 2019 at 12:26 AM Todd Kjos <tkjos@google.com> wrote:
> > I can send you a patch tomorrow (I won't be able to test it though).
>
> So, I was a bit quicker than you and I think I managed to fix the test myself :)
>
> See:
> https://github.com/SELinuxProject/selinux-testsuite/pull/50/commits/b559c3f54eae6130cb9e79c295b0f94db26e09e4

Looks good. Thanks!

>
> It seems the problem was indeed in the offsets array handling as you
> discovered. With the above commit included the PR#50 version of the
> binder test no longer fails for me.
>
> Thanks,
>
> >
> > On Wed, Mar 20, 2019 at 4:23 PM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > On Wed, Mar 20, 2019 at 3:50 PM Todd Kjos <tkjos@google.com> wrote:
> > > >
> > > > Paul,
> > > >
> > > > Looking at main() in test_binder.c...
> > > >
> > > > int main(int argc, char **argv)
> > > > {
> > > >
> > > > [...]
> > > >
> > > >   // Line 493
> > > >   struct binder_write_read bwr;
> > > >   struct flat_binder_object obj;
> > > >   struct {
> > > >     uint32_t cmd;
> > > >     struct binder_transaction_data txn;
> > > >   } __attribute__((packed)) writebuf;
> > > >   unsigned int readbuf[32];
> > > >
> > > > [...]
> > > >   // Line 630
> > > >   writebuf.txn.data.ptr.buffer = (uintptr_t)&obj;
> > > >   writebuf.txn.data.ptr.offsets = (uintptr_t)&obj +   // [A]
> > > >                                                  sizeof(struct
> > > > flat_binder_object);
> > > >
> > > >   bwr.write_buffer = (uintptr_t)&writebuf;
> > > >   bwr.write_size = sizeof(writebuf);
> > > >
> > > > It looks like bwr.txn.data.ptr.offsets points off the end of obj (see
> > > > [A] above), which means the binder driver will read compiler-dependent
> > > > stack data as the offset for the object. If it happens to be 0, then
> > > > the test will work (read the object from offset 0). If it's not 0,
> > > > then most likely offset > data_size (which is what found that BUG_ON
> > > > case). With my patch applied, this will just cause an error to be
> > > > returned (what you are seeing now).
> > > >
> > > > Same thing when you test with v5.0 -- if the offset happens to be 0,
> > > > then the test will succeed. If not, then the test will fail because
> > > > the transaction fails in an unexpected way.
> > >
> > > That might explain why the test used to work, but now fails - a
> > > different compiler (I rebuild the test before each test run).
> > >
> > > Keeping in mind I'm really quite ignorant when it comes to binder, how
> > > would you suggest fixing the test?
> > >
> > > --
> > > paul moore
> > > www.paul-moore.com
>
>
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Software Engineer, Security Technologies
> Red Hat, Inc.