Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage

[Andy Lutomirski <luto@kernel.org>]

On Mon, Apr 15, 2019 at 9:17 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Mon, Apr 15, 2019 at 06:07:44PM +0200, Thomas Gleixner wrote:
> > On Mon, 15 Apr 2019, Josh Poimboeuf wrote:
> > > On Mon, Apr 15, 2019 at 11:02:58AM +0200, Thomas Gleixner wrote:
> > > >   addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
> > > >
> > > > - if (size < 5 * sizeof(unsigned long))
> > > > + if (size < 5)
> > > >           return;
> > > >
> > > >   *addr++ = 0x12345678;
> > > >   *addr++ = caller;
> > > >   *addr++ = smp_processor_id();
> > > > - size -= 3 * sizeof(unsigned long);
> > > > + size -= 3;
> > > > +#ifdef CONFIG_STACKTRACE
> > > >   {
> > > > -         unsigned long *sptr = &caller;
> > > > -         unsigned long svalue;
> > > > -
> > > > -         while (!kstack_end(sptr)) {
> > > > -                 svalue = *sptr++;
> > > > -                 if (kernel_text_address(svalue)) {
> > > > -                         *addr++ = svalue;
> > > > -                         size -= sizeof(unsigned long);
> > > > -                         if (size <= sizeof(unsigned long))
> > > > -                                 break;
> > > > -                 }
> > > > -         }
> > > > +         struct stack_trace trace = {
> > > > +                 /* Leave one for the end marker below */
> > > > +                 .max_entries    = size - 1,
> > > > +                 .entries        = addr,
> > > > +                 .skip           = 3,
> > > > +         };
> > > >
> > > > +         save_stack_trace(&trace);
> > > > +         addr += trace.nr_entries;
> > > >   }
> > > > - *addr++ = 0x87654321;
> > > > +#endif
> > > > + *addr = 0x87654321;
> > >
> > > Looks like stack_trace.nr_entries isn't initialized?  (though this code
> > > gets eventually replaced by a later patch)
> >
> > struct initializer initialized the non mentioned fields to 0, if I'm not
> > totally mistaken.
>
> Hm, it seems you are correct.  And I thought I knew C.
>
> > > Who actually reads this stack trace?  I couldn't find a consumer.
> >
> > It's stored directly in the memory pointed to by @addr and that's the freed
> > cache memory. If that is used later (UAF) then the stack trace can be
> > printed to see where it was freed.
>
> Right... but who reads it?

That seems like a reasonable question.  After some grepping and some
git searching, it looks like there might not be any users.  I found
SLAB_STORE_USER, but that seems to be independent.

So maybe the whole mess should just be deleted.  If anyone ever
notices, they can re-add it better.

--Andy