| Message ID | cdd42a1b-ce15-df8c-6bd1-b0943275986f@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 24, 2017 at 2:17 PM, Alexander Popov <alex.popov@linux.com> wrote: > From 86f4f1f6deb76849e00c761fa30eeb479f789c35 Mon Sep 17 00:00:00 2001 > From: Alexander Popov <alex.popov@linux.com> > Date: Mon, 24 Jul 2017 23:16:28 +0300 > Subject: [PATCH 2/2] mm/slub.c: add a naive detection of double free or > corruption > > On 06.07.2017 03:27, Kees Cook wrote: >> This SLUB free list pointer obfuscation code is modified from Brad >> Spengler/PaX Team's code in the last public patch of grsecurity/PaX based >> on my understanding of the code. Changes or omissions from the original >> code are mine and don't reflect the original grsecurity/PaX code. >> >> This adds a per-cache random value to SLUB caches that is XORed with >> their freelist pointer address and value. This adds nearly zero overhead >> and frustrates the very common heap overflow exploitation method of >> overwriting freelist pointers. A recent example of the attack is written >> up here: http://cyseclabs.com/blog/cve-2016-6187-heap-off-by-one-exploit >> >> This is based on patches by Daniel Micay, and refactored to minimize the >> use of #ifdef. > > Hello! > > This is an addition to the SLAB_FREELIST_HARDENED feature. I'm sending it > according the discussion here: > http://www.openwall.com/lists/kernel-hardening/2017/07/17/9 > > -- >8 -- > > Add an assertion similar to "fasttop" check in GNU C Library allocator > as a part of SLAB_FREELIST_HARDENED feature. An object added to a singly > linked freelist should not point to itself. That helps to detect some > double free errors (e.g. CVE-2017-2636) without slub_debug and KASAN. > > Signed-off-by: Alexander Popov <alex.popov@linux.com> > --- > mm/slub.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/mm/slub.c b/mm/slub.c > index c92d636..f39d06e 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -290,6 +290,10 @@ static inline void set_freepointer(struct kmem_cache *s, > void *object, void *fp) > { > unsigned long freeptr_addr = (unsigned long)object + s->offset; > > +#ifdef CONFIG_SLAB_FREELIST_HARDENED > + BUG_ON(object == fp); /* naive detection of double free or corruption */ > +#endif > + > *(void **)freeptr_addr = freelist_ptr(s, fp, freeptr_addr); What happens if, instead of BUG_ON, we do: if (unlikely(WARN_RATELIMIT(object == fp, "double-free detected")) return; That would ignore adding it back to the list, since it's already there, yes? Or would this make SLUB go crazy? I can't tell from the accounting details around callers to set_freepointer(). I assume it's correct, since it's close to the same effect as BUG (i.e. we don't do the update, but the cache remains visible to the system) -Kees
On Tue, 25 Jul 2017, Kees Cook wrote: > > @@ -290,6 +290,10 @@ static inline void set_freepointer(struct kmem_cache *s, > > void *object, void *fp) > > { > > unsigned long freeptr_addr = (unsigned long)object + s->offset; > > > > +#ifdef CONFIG_SLAB_FREELIST_HARDENED > > + BUG_ON(object == fp); /* naive detection of double free or corruption */ > > +#endif > > + > > *(void **)freeptr_addr = freelist_ptr(s, fp, freeptr_addr); > > What happens if, instead of BUG_ON, we do: > > if (unlikely(WARN_RATELIMIT(object == fp, "double-free detected")) > return; This may work for the free fastpath but the set_freepointer function is use in multiple other locations. Maybe just add this to the fastpath instead of to this fucnction?
On Wed, Jul 26, 2017 at 7:08 AM, Christopher Lameter <cl@linux.com> wrote: > On Tue, 25 Jul 2017, Kees Cook wrote: > >> > @@ -290,6 +290,10 @@ static inline void set_freepointer(struct kmem_cache *s, >> > void *object, void *fp) >> > { >> > unsigned long freeptr_addr = (unsigned long)object + s->offset; >> > >> > +#ifdef CONFIG_SLAB_FREELIST_HARDENED >> > + BUG_ON(object == fp); /* naive detection of double free or corruption */ >> > +#endif >> > + >> > *(void **)freeptr_addr = freelist_ptr(s, fp, freeptr_addr); >> >> What happens if, instead of BUG_ON, we do: >> >> if (unlikely(WARN_RATELIMIT(object == fp, "double-free detected")) >> return; > > This may work for the free fastpath but the set_freepointer function is > use in multiple other locations. Maybe just add this to the fastpath > instead of to this fucnction? Do you mean do_slab_free()? -Kees
On Wed, 26 Jul 2017, Kees Cook wrote: > >> What happens if, instead of BUG_ON, we do: > >> > >> if (unlikely(WARN_RATELIMIT(object == fp, "double-free detected")) > >> return; > > > > This may work for the free fastpath but the set_freepointer function is > > use in multiple other locations. Maybe just add this to the fastpath > > instead of to this fucnction? > > Do you mean do_slab_free()? Yes inserting these lines into do_slab_free() would simple ignore the double free operation in the fast path and that would be safe. Although in either case we are adding code to the fastpath...
On Wed, Jul 26, 2017 at 9:55 AM, Christopher Lameter <cl@linux.com> wrote: > On Wed, 26 Jul 2017, Kees Cook wrote: > >> >> What happens if, instead of BUG_ON, we do: >> >> >> >> if (unlikely(WARN_RATELIMIT(object == fp, "double-free detected")) >> >> return; >> > >> > This may work for the free fastpath but the set_freepointer function is >> > use in multiple other locations. Maybe just add this to the fastpath >> > instead of to this fucnction? >> >> Do you mean do_slab_free()? > > Yes inserting these lines into do_slab_free() would simple ignore the > double free operation in the fast path and that would be safe. > > Although in either case we are adding code to the fastpath... While I'd like it unconditionally, I think Alexander's proposal was to put it behind CONFIG_SLAB_FREELIST_HARDENED. BTW, while I've got your attention, can you Ack the other patch? I sent a v4 for the pointer obfuscation, which we really need: https://lkml.org/lkml/2017/7/26/4 Thanks! -Kees
On Wed, 26 Jul 2017, Kees Cook wrote: > > Although in either case we are adding code to the fastpath... > > While I'd like it unconditionally, I think Alexander's proposal was to > put it behind CONFIG_SLAB_FREELIST_HARDENED. Sounds good. > BTW, while I've got your attention, can you Ack the other patch? I > sent a v4 for the pointer obfuscation, which we really need: > https://lkml.org/lkml/2017/7/26/4 Just looked through it.
Hello Christopher and Kees, On 26.07.2017 19:55, Christopher Lameter wrote: > On Wed, 26 Jul 2017, Kees Cook wrote: > >>>> What happens if, instead of BUG_ON, we do: >>>> >>>> if (unlikely(WARN_RATELIMIT(object == fp, "double-free detected")) >>>> return; >>> >>> This may work for the free fastpath but the set_freepointer function is >>> use in multiple other locations. Maybe just add this to the fastpath >>> instead of to this fucnction? >> >> Do you mean do_slab_free()? > > Yes inserting these lines into do_slab_free() would simple ignore the > double free operation in the fast path and that would be safe. I don't really like ignoring double-free. I think, that: - it will hide dangerous bugs in the kernel, - it can make some kernel exploits more stable. I would rather add BUG_ON to set_freepointer() behind SLAB_FREELIST_HARDENED. Is it fine? At the same time avoiding the consequences of some double-free errors is better than not doing that. It may be considered as kernel "self-healing", I don't know. I can prepare a second patch for do_slab_free(), as you described. Would you like it? Best regards, Alexander
On Fri, 28 Jul 2017, Alexander Popov wrote: > I don't really like ignoring double-free. I think, that: > - it will hide dangerous bugs in the kernel, > - it can make some kernel exploits more stable. > I would rather add BUG_ON to set_freepointer() behind SLAB_FREELIST_HARDENED. Is > it fine? I think Kees already added some logging output. > At the same time avoiding the consequences of some double-free errors is better > than not doing that. It may be considered as kernel "self-healing", I don't > know. I can prepare a second patch for do_slab_free(), as you described. Would > you like it? The SLUB allocator is already self healing if you enable the option to do so on bootup (covers more than just the double free case). What you propose here is no different than that and just another way of having similar functionality. In the best case it would work the same way.
Hello Christopher and Kees, Excuse me for the delayed reply. On 28.07.2017 02:53, Christopher Lameter wrote: > On Fri, 28 Jul 2017, Alexander Popov wrote: > >> I don't really like ignoring double-free. I think, that: >> - it will hide dangerous bugs in the kernel, >> - it can make some kernel exploits more stable. >> I would rather add BUG_ON to set_freepointer() behind SLAB_FREELIST_HARDENED. Is >> it fine? > > I think Kees already added some logging output. Hm, I don't see anything like that in v4 of "SLUB free list pointer obfuscation": https://patchwork.kernel.org/patch/9864165/ >> At the same time avoiding the consequences of some double-free errors is better >> than not doing that. It may be considered as kernel "self-healing", I don't >> know. I can prepare a second patch for do_slab_free(), as you described. Would >> you like it? > > The SLUB allocator is already self healing if you enable the option to do > so on bootup (covers more than just the double free case). What you > propose here is no different than that and just another way of having > similar functionality. In the best case it would work the same way. Ok, I see. Thanks. Best regards, Alexander
diff --git a/mm/slub.c b/mm/slub.c index c92d636..f39d06e 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -290,6 +290,10 @@ static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp) { unsigned long freeptr_addr = (unsigned long)object + s->offset; +#ifdef CONFIG_SLAB_FREELIST_HARDENED + BUG_ON(object == fp); /* naive detection of double free or corruption */ +#endif + *(void **)freeptr_addr = freelist_ptr(s, fp, freeptr_addr); }