mm: Add SLUB free list pointer obfuscation
Subsystems (2)
PATCH v3 1/1 mm: Add SLUB free list pointer obfuscation
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.
Under 200-count cycles of "hackbench -g 20 -l 1000" I saw the following run times:
before:
mean 10.11882499999999999995 variance .03320378329145728642 stdev .18221905304181911048
after:
mean 10.12654000000000000014 variance .04700556623115577889 stdev .21680767106160192064
The difference gets lost in the noise, but if the above is to be taken literally, using CONFIG_FREELIST_HARDENED is 0.07% slower.
---
v3:
- use static inlines instead of macros (akpm).
v2:
- rename CONFIG_SLAB_HARDENED to CONFIG_FREELIST_HARDENED (labbott).
--- include/linux/slub_def.h | 4 ++++ init/Kconfig | 9 +++++++++ mm/slub.c | 42 +++++++++++++++++++++++++++++++++++++----- 3 files changed, 50 insertions(+), 5 deletions(-)
include/linux/slub_def.h +4 -0
@@ -93,6 +93,10 @@ struct kmem_cache {#endif #endif +#ifdef CONFIG_SLAB_FREELIST_HARDENED +unsigned long random; +#endif +#ifdef CONFIG_NUMA /* * Defragmentation by allocating from a remote node.
init/Kconfig +9 -0
@@ -1900,6 +1900,15 @@ config SLAB_FREELIST_RANDOM security feature reduces the predictability of the kernel slab allocator against heap overflows. +config SLAB_FREELIST_HARDENED + bool "Harden slab freelist metadata" + depends on SLUB + help + Many kernel heap attacks try to target slab cache metadata and + other infrastructure. This options makes minor performance + sacrifies to harden the kernel slab allocator against common + freelist exploit methods. + config SLUB_CPU_PARTIAL default y depends on SLUB && SMP
mm/slub.c +37 -5
79 lines not on this page · raw message
Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
mm/slub.c static int kmem_cache_open(struct kmem_cache *s, unsigned long flags) · line 3573
Quoted 11 lines · Kees Cook
mm/slub.c +3
-0@@ -3536,6 +3565,9 @@ static int kmem_cache_open(struct kmem_cache *s, unsigned long flags) { s->flags =kmem_cache_flags (s->size, flags, s->name, s->ctor); s->reserved =0 ; +#ifdef CONFIG_SLAB_FREELIST_HARDENED + s->random =get_random_long (); +#endif if (need_reserve_slab_rcu && (s->flags &SLAB_TYPESAFE_BY_RCU )) s->reserved =sizeof (struct rcu_head );
So if an attacker knows the internal structure of data then he can simply dereference page->kmem_cache->random to decode the freepointer.
Assuming someone is already targeting a freelist pointer (which indicates detailed knowledge of the internal structure) then I would think that someone like that will also figure out how to follow the pointer links to get to the random value.
Not seeing the point of all of this.
Quoted 10 lines · Christoph Lameter
Quoted 8 lines · Kees Cook
mm/slub.c +3
-0@@ -3536,6 +3565,9 @@ static int kmem_cache_open(struct kmem_cache *s, unsigned long flags) { s->flags =kmem_cache_flags (s->size, flags, s->name, s->ctor); s->reserved =0 ; +#ifdef CONFIG_SLAB_FREELIST_HARDENED + s->random =get_random_long (); +#endif
Quoted 6 lines
mm/slub.c static int kmem_cache_open(struct kmem_cache *s, unsigned long flags) · line 3573
Quoted 3 lines
mm/slub.c +0
-0if (need_reserve_slab_rcu && (s->flags &SLAB_TYPESAFE_BY_RCU )) s->reserved =sizeof (struct rcu_head );So if an attacker knows the internal structure of data then he can simply dereference
page->kmem_cache->randomto decode the freepointer.
That requires a series of arbitrary reads. This is protecting against attacks that use an adjacent slab object write overflow to write the freelist pointer. This internal structure is very reliable, and has been the basis of freelist attacks against the kernel for a decade.
Quoted 4 lines
Assuming someone is already targeting a freelist pointer (which indicates detailed knowledge of the internal structure) then I would think that someone like that will also figure out how to follow the pointer links to get to the random value.
The kind of hardening this provides is to frustrate the expansion of an attacker's capabilities. Most attacks are a chain of exploits that slowly builds up the ability to perform arbitrary writes. For example, a slab object overflow isn't an arbitrary write on its own, but when combined with heap allocation layout control and an adjacent free object, this can be upgraded to an arbitrary write.
Quoted 1 line
Not seeing the point of all of this.
It is a probabilistic defense, but then so is the stack protector. This is a similar defense; while not perfect it makes the class of attack much more difficult to mount.
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 18 lines · Kees Cook
Quoted 10 lines · Christoph Lameter
Quoted 8 lines · Kees Cook
mm/slub.c +3
-0@@ -3536,6 +3565,9 @@ static int kmem_cache_open(struct kmem_cache *s, unsigned long flags) { s->flags =kmem_cache_flags (s->size, flags, s->name, s->ctor); s->reserved =0 ; +#ifdef CONFIG_SLAB_FREELIST_HARDENED + s->random =get_random_long (); +#endif Quoted 6 lines
mm/slub.c static int kmem_cache_open(struct kmem_cache *s, unsigned long flags) · line 3573
Quoted 3 lines
mm/slub.c +0
-0if (need_reserve_slab_rcu && (s->flags &SLAB_TYPESAFE_BY_RCU )) s->reserved =sizeof (struct rcu_head );So if an attacker knows the internal structure of data then he can simply dereference
page->kmem_cache->randomto decode the freepointer.
Quoted 4 lines
That requires a series of arbitrary reads. This is protecting against attacks that use an adjacent slab object write overflow to write the freelist pointer. This internal structure is very reliable, and has been the basis of freelist attacks against the kernel for a decade.
These reads are not arbitrary. You can usually calculate the page struct address easily from the address and then do a couple of loads to get there.
Ok so you get rid of the old attacks because we did not have that hardening in effect when they designed their approaches?
Quoted 3 lines
It is a probabilistic defense, but then so is the stack protector. This is a similar defense; while not perfect it makes the class of attack much more difficult to mount.
Na I am not convinced of the "much more difficult". Maybe they will just have to upgrade their approaches to fetch the proper values to decode.
Quoted 31 lines · Christoph Lameter
Quoted 23 lines · Kees Cook
Quoted 12 lines · Christoph Lameter
Quoted 10 lines · Kees Cook
mm/slub.c +3
-0@@ -3536,6 +3565,9 @@ static int kmem_cache_open(struct kmem_cache *s, unsigned long flags) { s->flags =kmem_cache_flags (s->size, flags, s->name, s- >ctor); s->reserved =0 ; +#ifdef CONFIG_SLAB_FREELIST_HARDENED + s->random =get_random_long (); +#endif Quoted 8 lines
Quoted 4 lines
if (need_reserve_slab_rcu && (s->flags & SLAB_TYPESAFE_BY_RCU)) s->reserved = sizeof(struct rcu_head);So if an attacker knows the internal structure of data then he can simply
dereferencepage->kmem_cache->randomto decode the freepointer.Quoted 5 lines
That requires a series of arbitrary reads. This is protecting against
attacks that use an adjacent slab object write overflow to write the freelist pointer. This internal structure is very reliable, and has been the basis of freelist attacks against the kernel for a decade.
Quoted 4 lines
These reads are not arbitrary. You can usually calculate the page struct
address easily from the address and then do a couple of loads to get there.
You're describing an arbitrary read vulnerability: an attacker able to read the value of an address of their choosing. Requiring a powerful additional primitive rather than only a small fixed size overflow or a weak use-after-free vulnerability to use a common exploit vector is useful.
A deterministic mitigation would be better, but I don't think an extra slab allocator for hardened kernels would be welcomed. Since there isn't a separate allocator for that niche, SLAB or SLUB are used. The ideal would be bitmaps in struct page but that implies another allocator, using single pages for the smallest size classes and potentially needing to bloat struct page even with that.
There's definitely a limit to the hardening that can be done for SLUB, but unless forking it into a different allocator is welcome that's what will be suggested. Similarly, the slab freelist randomization feature is a much weaker mitigation than it could be without these constraints placed on it. This is much lower complexity than that and higher value though...
Quoted 10 lines
Ok so you get rid of the old attacks because we did not have that hardening in effect when they designed their approaches?
Quoted 3 lines
It is a probabilistic defense, but then so is the stack protector. This is a similar defense; while not perfect it makes the class of attack much more difficult to mount.
Na I am not convinced of the "much more difficult". Maybe they will just
have to upgrade their approaches to fetch the proper values to decode.
To fetch the values they would need an arbitrary read vulnerability or the ability to dump them via uninitialized slab allocations as an extra requirement.
An attacker can similarly bypass the stack canary by reading them from stack frames via a stack buffer read overflow or uninitialized variable usage leaking stack data. On non-x86, at least with SMP, the stack canary is just a global variable that remains the same after initialization too. That doesn't make it useless, although the kernel doesn't have many linear overflows on the stack which is the real issue with it as a mitigation. Despite that, most people are using kernels with stack canaries, and that has a significant performance cost unlike these kinds of changes.
Quoted 32 lines · Christoph Lameter
Quoted 23 lines · Kees Cook
Quoted 12 lines · Christoph Lameter
Quoted 10 lines · Kees Cook
(quoted diff) +0
-0@@ -3536,6 +3565,9 @@ static int kmem_cache_open(struct kmem_cache *s, unsigned long flags) {s->flags = kmem_cache_flags(s->size, flags,
s->name, s-Quoted 1 line
ctor);
s->reserved = 0;
mm/slub.c +3
-0+#ifdef CONFIG_SLAB_FREELIST_HARDENED + s->random = get_random_long(); +#endif Quoted 8 lines
Quoted 4 lines
if (
need_reserve_slab_rcu&& (s->flags&SLAB_TYPESAFE_BY_RCU))
s->reserved =sizeof(struct rcu_head);So if an attacker knows the internal structure of data then he can simply
dereferencepage->kmem_cache->randomto decode the freepointer.Quoted 6 lines
That requires a series of arbitrary reads. This is protecting against
attacks that use an adjacent slab object write overflow to write the
freelist pointer. This internal structure is very reliable, and has been the basis of freelist attacks against the kernel for a decade.
Quoted 7 lines
These reads are not arbitrary. You can usually calculate the page struct
address easily from the address and then do a couple of loads to get there.Ok so you get rid of the old attacks because we did not have that hardening in effect when they designed their approaches?
The hardening protects against situations where people do not have arbitrary code execution and memory read access in the kernel, with the goal of protecting people from acquiring those abilities.
Quoted 8 lines
Quoted 3 lines
It is a probabilistic defense, but then so is the stack protector. This is a similar defense; while not perfect it makes the class of attack much more difficult to mount.
Na I am not convinced of the "much more difficult". Maybe they will just
have to upgrade their approaches to fetch the proper values to decode.
Easier said than done. Most of the time there is an unpatched vulnerability outstanding, there is only one known issue, before the kernel is updated by the user, to a version that does not have that issue.
Bypassing kernel hardening typically requires the use of multiple vulnerabilities, and the absence of roadblocks (like hardening) that make a type of vulnerability exploitable.
Between usercopy hardening, and these slub freelist canaries (which is what they effectively are), several classes of exploits are no longer usable.
Signature · 1 line
All rights reversed
Quoted 13 lines · Rik van Riel
Quoted 12 lines · Christoph Lameter
Quoted 6 lines · Kees Cook
That requires a series of arbitrary reads. This is protecting against
attacks that use an adjacent slab object write overflow to write the
freelist pointer. This internal structure is very reliable, and has been the basis of freelist attacks against the kernel for a decade.These reads are not arbitrary. You can usually calculate the page struct address easily from the address and then do a couple of loads to get there.
Quoted 7 lines
Quoted 2 lines
Ok so you get rid of the old attacks because we did not have that hardening in effect when they designed their approaches?
The hardening protects against situations where people do not have arbitrary code execution and memory read access in the kernel, with the goal of protecting people from acquiring those abilities.
Right. This is about blocking the escalation of attack capability. For slab object overflow flaws, there are mainly two exploitation methods:
adjacent allocated object overwrite and adjacent freed object overwrite (i.e. a freelist pointer overwrite). The first attack depends heavily on which slab cache (and therefore which structures) has been exposed by the bug. It's a very narrow and specific attack method. The freelist attack is entirely general purpose since it provides a reliable way to gain arbitrary write capabilities. Protecting against that attack greatly narrows the options for an attacker which makes attacks more expensive to create and possibly less reliable (and reliability is crucial to successful attacks).
Quoted 13 lines
Quoted 7 lines
Quoted 3 lines
It is a probabilistic defense, but then so is the stack protector. This is a similar defense; while not perfect it makes the class of attack much more difficult to mount.
Na I am not convinced of the "much more difficult". Maybe they will just have to upgrade their approaches to fetch the proper values to decode.
Easier said than done. Most of the time there is an unpatched vulnerability outstanding, there is only one known issue, before the kernel is updated by the user, to a version that does not have that issue.
Quoted 8 lines
Bypassing kernel hardening typically requires the use of multiple vulnerabilities, and the absence of roadblocks (like hardening) that make a type of vulnerability exploitable.
Between usercopy hardening, and these slub freelist canaries (which is what they effectively are), several classes of exploits are no longer usable.
Yup. I've been thinking of this patch kind of like glibc's PTR_MANGLE feature.
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 11 lines · Kees Cook
Right. This is about blocking the escalation of attack capability. For slab object overflow flaws, there are mainly two exploitation methods:
adjacent allocated object overwrite and adjacent freed object overwrite (i.e. a freelist pointer overwrite). The first attack depends heavily on which slab cache (and therefore which structures) has been exposed by the bug. It's a very narrow and specific attack method. The freelist attack is entirely general purpose since it provides a reliable way to gain arbitrary write capabilities. Protecting against that attack greatly narrows the options for an attacker which makes attacks more expensive to create and possibly less reliable (and reliability is crucial to successful attacks).
The simplest thing here is to vary the location of the freelist pointer. That way you cannot hit the freepointer in a deterministic way
The freepointer is put at offset 0 right now. But you could put it anywhere in the object.
mm/slub.c +2 -1
@@ -3467,7 +3467,8 @@ static int calculate_sizes(struct kmem_c*/ s->offset = size; size +=sizeof (void *); - } + }else + s->offset = s->size / sizeof(void *) * <insert random chance logic here>#ifdef CONFIG_SLUB_DEBUG if (flags &SLAB_STORE_USER )
Quoted 26 lines · Christoph Lameter
Quoted 11 lines · Kees Cook
Right. This is about blocking the escalation of attack capability. For slab object overflow flaws, there are mainly two exploitation methods:
adjacent allocated object overwrite and adjacent freed object overwrite (i.e. a freelist pointer overwrite). The first attack depends heavily on which slab cache (and therefore which structures) has been exposed by the bug. It's a very narrow and specific attack method. The freelist attack is entirely general purpose since it provides a reliable way to gain arbitrary write capabilities. Protecting against that attack greatly narrows the options for an attacker which makes attacks more expensive to create and possibly less reliable (and reliability is crucial to successful attacks).The simplest thing here is to vary the location of the freelist pointer. That way you cannot hit the freepointer in a deterministic way
The freepointer is put at offset 0 right now. But you could put it anywhere in the object.
Index:
linux/mm/slub.c
===================================================================
mm/slub.c static int calculate_sizes(struct kmem_c · line 3474
Quoted 9 lines
mm/slub.c +2
-1@@ -3467,7 +3467,8 @@ static int calculate_sizes(struct kmem_c*/ s->offset = size; size +=sizeof (void *); - } + }else + s->offset = s->size / sizeof(void *) * <insert random chance logic here>#ifdef CONFIG_SLUB_DEBUG if (flags &SLAB_STORE_USER )
I wouldn't mind having both mitigations, but this alone is still open to spraying attacks. As long as an attacker's overflow can span an entire object, they can still hit the freelist pointer (which is especially true with small objects). With the XOR obfuscation they have to know where the pointer is stored (usually not available since they have only been able to arrange "next object is unallocated" without knowing _where_ it is allocated) and the random number (stored separately in the cache).
If we also added a >0 offset, that would make things even less deterministic. Though I wonder if it would make the performance impact higher. The XOR patch right now is very light.
Yet another option would be to moving the freelist pointer over by sizeof(void *) and adding a canary to be checked at offset 0, but that involves additional memory fetches and doesn't protect against a bad array index attack (rather than a linear overflow). So, I still think the XOR patch is the right first step. Would could further harden it, but I think it's the place to start.
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 3 lines · Kees Cook
If we also added a >0 offset, that would make things even less deterministic. Though I wonder if it would make the performance impact higher. The XOR patch right now is very light.
There would be barely any performance impact if you keep the offset within a cacheline since most objects start on a cacheline boundary. The processor has to fetch the cacheline anyways.
Quoted 9 lines · Christoph Lameter
Quoted 3 lines · Kees Cook
If we also added a >0 offset, that would make things even less deterministic. Though I wonder if it would make the performance impact higher. The XOR patch right now is very light.
There would be barely any performance impact if you keep the offset within a cacheline since most objects start on a cacheline boundary. The processor has to fetch the cacheline anyways.
Sure, this seems like a nice additional bit of hardening, even if we're limited to a cacheline. I'd still want to protect the spray and index attacks though (which the XOR method covers), but we can do both. We should keep them distinct patches, though. If you'll Ack the XOR patch, I can poke at adding offset randomization?
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 1 line
From
86f4f1f6deb76849e00c761fa30eeb479f789c35Mon Sep 17 00:00:00 2001
From: Alexander Popov <[email protected]>
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
Quoted 5 lines · Kees Cook
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.
Quoted 8 lines
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.
--- mm/slub.c | 4 ++++ 1 file changed, 4 insertions(+)
mm/slub.c +0 -0
@@ -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);
}
Signature · 1 line
2.7.4
Quoted 1 line
From
86f4f1f6deb76849e00c761fa30eeb479f789c35Mon Sep 17 00:00:00 2001
From: Alexander Popov <[email protected]>
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
Quoted 12 lines · Alexander Popov
Quoted 11 lines · Kees Cook
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
Quoted 8 lines
Quoted 2 lines
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_HARDENEDfeature. I'm sending it according the discussion here:
http://www.openwall.com/lists/kernel-hardening/2017/07/17/9
In my previous message my email client wrapped one line and corrupted the patch. Excuse me for that. See the fixed patch below.
-- >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.
--- mm/slub.c | 4 ++++ 1 file changed, 4 insertions(+)
mm/slub.c +4 -0
@@ -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); }
Signature · 1 line
2.7.4
Quoted 44 lines · Alexander Popov
From
86f4f1f6deb76849e00c761fa30eeb479f789c35Mon Sep 17 00:00:00 2001
From: Alexander Popov <[email protected]>
Date: Mon, 24 Jul 2017 23:16:28 +0300
Subject: [PATCH 2/2]mm/slub.c: add a naive detection of double free or
corruptionQuoted 5 lines · Kees Cook
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.
Quoted 8 lines
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_HARDENEDfeature. 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_HARDENEDfeature. 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) withoutslub_debugand KASAN.Signed-off-by Alexander Popov <[email protected]>--- mm/slub.c | 4 ++++ 1 file changed, 4 insertions(+)mm/slub.c +0
-0
mm/slub.c static inline void set_freepointer(struct kmem_cache *s, · line 298
Quoted 9 lines
mm/slub.c +4
-0@@ -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
Signature · 2 lines
Kees Cook Pixel Security
Quoted 13 lines · Kees Cook
Review static inline void set_freepointer(struct kmem_cache *s, · line 298
Quoted 10 lines
(quoted diff) +4
-0@@ -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:
Quoted 2 lines
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?
Quoted 15 lines · Christopher Lameter
Quoted 13 lines · Kees Cook
Review static inline void set_freepointer(struct kmem_cache *s, · line 298
Quoted 10 lines
(quoted diff) +4
-0@@ -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:
Quoted 6 lines
Quoted 2 lines
if (
unlikely(WARN_RATELIMIT(object == fp, "double-free detected"))return;This may work for the free fastpath but the
set_freepointerfunction 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
Signature · 2 lines
Kees Cook Pixel Security
Quoted 10 lines · Kees Cook
Quoted 8 lines
Quoted 4 lines
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_freepointerfunction 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...
Quoted 11 lines · Christopher Lameter
Quoted 9 lines · Kees Cook
Quoted 8 lines
Quoted 4 lines
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_freepointerfunction is use in multiple other locations. Maybe just add this to the fastpath instead of to this fucnction?
Quoted 6 lines
Quoted 1 line
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
Signature · 2 lines
Kees Cook Pixel Security
Quoted 4 lines · Kees Cook
Quoted 1 line
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.
Quoted 3 lines
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,
Quoted 7 lines · Christopher Lameter
Quoted 5 lines · Kees Cook
Quoted 5 lines
Quoted 4 lines
What happens if, instead of
BUG_ON, we do:if (
unlikely(WARN_RATELIMIT(object == fp, "double-free detected"))return;
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
This may work for the free fastpath but the
set_freepointerfunction 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
Quoted 5 lines · Alexander Popov
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 addBUG_ONtoset_freepointer()behindSLAB_FREELIST_HARDENED. Is it fine?
I think Kees already added some logging output.
Quoted 4 lines
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.
Quoted 9 lines · Christopher Lameter
Quoted 5 lines · Alexander Popov
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 addBUG_ONtoset_freepointer()behindSLAB_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/
Quoted 9 lines
Quoted 4 lines
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