Re: [PATCH v4 00/23] LSM: Module stacking for AppArmor

[Kees Cook <keescook@chromium.org>]

On Thu, Jun 27, 2019 at 12:46:01PM +1000, James Morris wrote:
> On Thu, 27 Jun 2019, James Morris wrote:
> 
> > On Wed, 26 Jun 2019, Casey Schaufler wrote:
> > 
> > > This patchset provides the changes required for
> > > the AppArmor security module to stack safely with any other.
> > 
> > I get a kernel oops with this patchset when running the SELinux testsuite 
> > (binder test) with:
> > 
> > $ cat /sys/kernel/security/lsm 
> > capability,yama,loadpin,safesetid,selinux,tomoyo
> > 
> > 
> > [  485.357377] binder: 4224 RLIMIT_NICE not set
> > [  485.360727] binder: 4224 RLIMIT_NICE not set
> > [  485.361480] binder: 4224 RLIMIT_NICE not set
> > [  485.362164] BUG: unable to handle kernel paging request at 0000000000001080
> > [  485.362927] #PF error: [normal kernel read fault]
> > [  485.363143] ------------[ cut here ]------------
> > [  485.363581] PGD 800000044e17b067 P4D 800000044e17b067 PUD 44b796067 PMD 0 
> > [  485.364226] kernel BUG at drivers/android/binder_alloc.c:1139!
> 
> It's this BUG_ON:
> 
> static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc,
>                                         bool to_buffer,
>                                         struct binder_buffer *buffer,
>                                         binder_size_t buffer_offset,
>                                         void *ptr,
>                                         size_t bytes)
> {
>         /* All copies must be 32-bit aligned and 32-bit size */
>         BUG_ON(!check_buffer(alloc, buffer, buffer_offset, bytes));

Before:

        if (secctx) {
                size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
                                    ALIGN(tr->offsets_size, sizeof(void *)) +
                                    ALIGN(extra_buffers_size, sizeof(void *)) -
                                    ALIGN(secctx_sz, sizeof(u64));

                t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
                binder_alloc_copy_to_buffer(&target_proc->alloc,
                                            t->buffer, buf_offset,
                                            secctx, secctx_sz);
                security_release_secctx(secctx, secctx_sz);
                secctx = NULL;
        }

After:

        if (lsmctx.context) {
                size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
                                    ALIGN(tr->offsets_size, sizeof(void *)) +
                                    ALIGN(extra_buffers_size, sizeof(void *)) -
                                    ALIGN(lsmctx.len, sizeof(u64));

                t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
                binder_alloc_copy_to_buffer(&target_proc->alloc,
                                            t->buffer, buf_offset,
                                            lsmctx.context, lsmctx.len);
                security_release_secctx(&lsmctx);
        }

Which changes the "src" and "bytes" argument, assuming the size
calculation for buf_offset is the same. But, a quick shows:

-       char *secctx = NULL;
-       u32 secctx_sz = 0;
+       struct lsmcontext lsmctx;
...
-       if (secctx) {
+       if (lsmctx.context) {

lsmctx.context isn't being initialized, and it was passed by reference,
so compiler won't complain. I'll find the patch and comment. Totally
missed it in review!

-- 
Kees Cook