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
