| Message ID | alpine.DEB.2.21.1904151101100.1729@nanos.tec.linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V4,01/32] mm/slab: Fix broken stack trace storage | expand |
On Mon, Apr 15, 2019 at 11:02:58AM +0200, Thomas Gleixner wrote: > kstack_end() is broken on interrupt stacks as they are not guaranteed to be > sized THREAD_SIZE and THREAD_SIZE aligned. > > Use the stack tracer instead. Remove the pointless pointer increment at the > end of the function while at it. > > Fixes: 98eb235b7feb ("[PATCH] page unmapping debug") - History tree > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Pekka Enberg <penberg@kernel.org> > Cc: linux-mm@kvack.org > --- > V4: Made the code simpler to understand (Andy) and make it actually compile > --- > mm/slab.c | 30 ++++++++++++++---------------- > 1 file changed, 14 insertions(+), 16 deletions(-) > > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -1470,33 +1470,31 @@ static bool is_debug_pagealloc_cache(str > static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr, > unsigned long caller) > { > - int size = cachep->object_size; > + int size = cachep->object_size / sizeof(unsigned long); > > 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) Who actually reads this stack trace? I couldn't find a consumer.
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. > 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. Thanks, tglx
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?
On Mon, Apr 15, 2019 at 06:07:44PM +0200, Thomas Gleixner wrote: > On Mon, 15 Apr 2019, Josh Poimboeuf wrote: > > > + struct stack_trace trace = { > > > + /* Leave one for the end marker below */ > > > + .max_entries = size - 1, > > > + .entries = addr, > > > + .skip = 3, > > > + }; > > 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. Correct.
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
On Mon, 15 Apr 2019, Josh Poimboeuf wrote: > On Mon, Apr 15, 2019 at 06:07:44PM +0200, Thomas Gleixner wrote: > > > > > > 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? Indeed. I didn't check but I know that I saw that info printed at least a decade ago. Looks like that debug magic in slab.c has seen major changes since then. Thanks, tglx
On Mon, 15 Apr 2019, Andy Lutomirski wrote: > 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: > > > > 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 Anymore. There was something 10y+ ago... > 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. No objections from my side, but the mm people might have opinions. Thanks, tglx
On 4/15/19 11:22 PM, Thomas Gleixner wrote: > On Mon, 15 Apr 2019, Andy Lutomirski wrote: >> 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: >>>>> 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 > > Anymore. There was something 10y+ ago. In theory it can be useful in a crash dump. But I don't see any related debugging check that would trigger a panic, in order to get one. >> 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. > > No objections from my side, but the mm people might have opinions. Anyone who wants to debug wrong slab usage probably uses SLUB anyway, so I don't think it's a problem to remove broken SLAB debugging. Perhaps even SLAB itself will be removed soon if there's performance data supporting it [1]. [1] https://lore.kernel.org/linux-mm/20190412112816.GD18914@techsingularity.net/T/#u > Thanks, > > tglx >
--- a/mm/slab.c +++ b/mm/slab.c @@ -1470,33 +1470,31 @@ static bool is_debug_pagealloc_cache(str static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr, unsigned long caller) { - int size = cachep->object_size; + int size = cachep->object_size / sizeof(unsigned long); 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; } static void slab_kernel_map(struct kmem_cache *cachep, void *objp,
kstack_end() is broken on interrupt stacks as they are not guaranteed to be sized THREAD_SIZE and THREAD_SIZE aligned. Use the stack tracer instead. Remove the pointless pointer increment at the end of the function while at it. Fixes: 98eb235b7feb ("[PATCH] page unmapping debug") - History tree Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Pekka Enberg <penberg@kernel.org> Cc: linux-mm@kvack.org --- V4: Made the code simpler to understand (Andy) and make it actually compile --- mm/slab.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-)