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

[Ondrej Mosnacek]

On Thu, Mar 21, 2019 at 12:26 AM Todd Kjos <tkjos at 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

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 at paul-moore.com> wrote:
> >
> > On Wed, Mar 20, 2019 at 3:50 PM Todd Kjos <tkjos at 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.