mm: Add additional consistency check

PATCH In review Kees Cook 6 participants 41 messages Mar 31, 2017 – Apr 28, 2017

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Kees Cook

author

2017-03-31 16:40 UTC #0 lore raw

PATCH 1/1 mm: Add additional consistency check

As found in PaX, this adds a cheap check on heap consistency, just to notice if things have gotten corrupted in the page lookup.

Signed-off-by Kees Cook <[email protected]>

---
 mm/slab.h | 1 +
 1 file changed, 1 insertion(+)

mm/slab.h +1 -0

index 65e7c3fcac72..64447640b70c 100644
@@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
 		return s;
 
 	page = virt_to_head_page(x);
+	BUG_ON(!PageSlab(page));
 	cachep = page->slab_cache;
 	if (slab_equal_or_root(cachep, s))
 		return cachep;

Signature · 12 lines

2.7.4


-- 
Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2017-03-31 21:33 UTC #1 lore raw

Quoted 2 lines · On Fri, 31 Mar 2017 09:40:28 -0700 Kees Cook <[email protected]> wrote:

As found in PaX, this adds a cheap check on heap consistency, just to notice if things have gotten corrupted in the page lookup.

"As found in PaX" isn't a very illuminating justification for such a change. Was there a real kernel bug which this would have exposed, or what?

mm/slab.h static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) · line 390

Quoted 10 lines

mm/slab.h +1 -0

@@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
 		return s;
 
 	page = virt_to_head_page(x);
+	BUG_ON(!PageSlab(page));
 	cachep = page->slab_cache;
 	if (slab_equal_or_root(cachep, s))
 		return cachep;

BUG_ON might be too severe. I expect the kindest VM_WARN_ON_ONCE() would suffice here, but without more details it is hard to say.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kees Cook

author

2017-04-01 00:04 UTC #2 lore raw

Quoted 8 lines · Andrew Morton

Quoted 2 lines · On Fri, 31 Mar 2017 09:40:28 -0700 Kees Cook <[email protected]> wrote:
As found in PaX, this adds a cheap check on heap consistency, just to notice if things have gotten corrupted in the page lookup.
"As found in PaX" isn't a very illuminating justification for such a change. Was there a real kernel bug which this would have exposed, or what?

I don't know off the top of my head, but given the kinds of heap attacks I've been seeing, I think this added consistency check is worth it given how inexpensive it is. When heap metadata gets corrupted, we can get into nasty side-effects that can be attacker-controlled, so better to catch obviously bad states as early as possible.

Quoted 5 lines

mm/slab.h +0 -0

@@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
              return s;

Quoted 8 lines

mm/slab.h static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) · line 390
Quoted 5 lines
mm/slab.h +1 -0
      page = virt_to_head_page(x);
+     BUG_ON(!PageSlab(page));
      cachep = page->slab_cache;
      if (slab_equal_or_root(cachep, s))
              return cachep;
BUG_ON might be too severe. I expect the kindest VM_WARN_ON_ONCE() would suffice here, but without more details it is hard to say.

So, WARN isn't enough to protect the kernel (execution continues and the memory is still dereferenced for malicious purposes, etc). Perhaps use CHECK_DATA_CORRUPTION() here, which can either WARN and take a "safe" path, or BUG (depending on config paranoia of the builder). I've got a series adding it in a number of other places, so I could add this patch to that series?

-Kees

Signature · 8 lines

Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michael Ellerman

2017-04-03 03:40 UTC #3 lore raw

Quoted 11 lines · Kees Cook

Quoted 8 lines · Andrew Morton
Quoted 2 lines · On Fri, 31 Mar 2017 09:40:28 -0700 Kees Cook <[email protected]> wrote:
As found in PaX, this adds a cheap check on heap consistency, just to notice if things have gotten corrupted in the page lookup.
"As found in PaX" isn't a very illuminating justification for such a change. Was there a real kernel bug which this would have exposed, or what?

Quoted 6 lines

I don't know off the top of my head, but given the kinds of heap attacks I've been seeing, I think this added consistency check is worth it given how inexpensive it is. When heap metadata gets corrupted, we can get into nasty side-effects that can be attacker-controlled, so better to catch obviously bad states as early as possible.

There's your changelog :)

Quoted 14 lines

Quoted 5 lines
Quoted 5 lines
mm/slab.h +0 -0
@@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
              return s;
 
Quoted 8 lines
mm/slab.h static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) · line 390
Quoted 5 lines
mm/slab.h +1 -0
      page = virt_to_head_page(x);
+     BUG_ON(!PageSlab(page));
      cachep = page->slab_cache;
      if (slab_equal_or_root(cachep, s))
              return cachep;
BUG_ON might be too severe. I expect the kindest VM_WARN_ON_ONCE() would suffice here, but without more details it is hard to say.

Quoted 2 lines

So, WARN isn't enough to protect the kernel (execution continues and the memory is still dereferenced for malicious purposes, etc).

You could do:

	if (WARN_ON(!PageSlab(page)))
        	return NULL.

Though I see at least two callers that don't check for a NULL return.

Looking at the context, the tail of the function already contains:

	pr_err("%s: Wrong slab cache. %s but object is from %s\n",
	       __func__, s->name, cachep->name);
	WARN_ON_ONCE(1);
	return s;
}

At least in slab.c it seems that would allow you to "free" an object from one kmem_cache onto the array_cache of another kmem_cache, which seems fishy. But maybe there's a check somewhere I'm missing?

cheers

Christoph Lameter

2017-04-03 14:03 UTC #4 lore raw

Quoted 3 lines · Michael Ellerman

At least in slab.c it seems that would allow you to "free" an object from one kmem_cache onto the array_cache of another kmem_cache, which seems fishy. But maybe there's a check somewhere I'm missing?

kfree can be used to free any object from any slab cache.

kmem_cache_free() checks if the object belongs to the cache given.

Matthew Wilcox

2017-04-03 14:53 UTC #5 lore raw

Quoted 7 lines · Christoph Lameter

Quoted 3 lines · Michael Ellerman
At least in slab.c it seems that would allow you to "free" an object from one kmem_cache onto the array_cache of another kmem_cache, which seems fishy. But maybe there's a check somewhere I'm missing?
kfree can be used to free any object from any slab cache.

Is that a guarantee? There's some wording in the RCU free code that seems to indicate we can't rely on that being true.

Michal Hocko

2017-04-04 11:30 UTC #6 lore raw

In reply to Kees Cook

Quoted 4 lines · Kees Cook

As found in PaX, this adds a cheap check on heap consistency, just to notice if things have gotten corrupted in the page lookup.
Signed-off-by Kees Cook <[email protected]>

NAK without a proper changelog. Seriously, we do not blindly apply changes from other projects without a deep understanding of all consequences.

Quoted 29 lines

---
 mm/slab.h | 1 +
 1 file changed, 1 insertion(+)

mm/slab.h +1 -0

index 65e7c3fcac72..64447640b70c 100644
@@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
 		return s;
 
 	page = virt_to_head_page(x);
+	BUG_ON(!PageSlab(page));
 	cachep = page->slab_cache;
 	if (slab_equal_or_root(cachep, s))
 		return cachep;

Signature · 12 lines

2.7.4


-- 
Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-04 15:07 UTC #7 lore raw

Quoted 3 lines · Michal Hocko

NAK without a proper changelog. Seriously, we do not blindly apply changes from other projects without a deep understanding of all consequences.

Functionalitywise this is trivial. A page must be a slab page in order to be able to determine the slab cache of an object. Its definitely not ok if the page is not a slab page.

The main issue that may exist here is the adding of overhead to a critical code path like kfree().

Michal Hocko

2017-04-04 15:16 UTC #8 lore raw

Quoted 9 lines · Cristopher Lameter

Quoted 3 lines · Michal Hocko
NAK without a proper changelog. Seriously, we do not blindly apply changes from other projects without a deep understanding of all consequences.
Functionalitywise this is trivial. A page must be a slab page in order to be able to determine the slab cache of an object. Its definitely not ok if the page is not a slab page.

Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?

Quoted 2 lines

The main issue that may exist here is the adding of overhead to a critical code path like kfree().

Yes, nothing is for free. But if the attack space is real then we probably want to sacrifice few cycles (to simply return ASAP without further further processing). This all should be in the changelog ideally with some numbers. I suspect this would be hard to measure in most workloads.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kees Cook

author

2017-04-04 15:46 UTC #9 lore raw

Quoted 7 lines · Michal Hocko

Quoted 6 lines · Cristopher Lameter
Quoted 3 lines · Michal Hocko
NAK without a proper changelog. Seriously, we do not blindly apply changes from other projects without a deep understanding of all consequences.

Quoted 6 lines

Quoted 3 lines
Functionalitywise this is trivial. A page must be a slab page in order to be able to determine the slab cache of an object. Its definitely not ok if the page is not a slab page.
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?

I can put this behind CHECK_DATA_CORRUPTION() instead of BUG(), which allows the system builder to choose between WARN and BUG. Some people absolutely want the kernel to BUG on data corruption as it could be an attack.

Quoted 8 lines

Quoted 2 lines
The main issue that may exist here is the adding of overhead to a critical code path like kfree().
Yes, nothing is for free. But if the attack space is real then we probably want to sacrifice few cycles (to simply return ASAP without further further processing). This all should be in the changelog ideally with some numbers. I suspect this would be hard to measure in most workloads.

Given the trivial nature of the check, yeah, it seemed impossible to actually show performance changes.

-Kees

Signature · 8 lines

Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michal Hocko

2017-04-04 15:59 UTC #10 lore raw

Quoted 12 lines · Kees Cook

Quoted 11 lines · Michal Hocko
Quoted 9 lines · Cristopher Lameter
Quoted 3 lines · Michal Hocko
NAK without a proper changelog. Seriously, we do not blindly apply changes from other projects without a deep understanding of all consequences.
Functionalitywise this is trivial. A page must be a slab page in order to be able to determine the slab cache of an object. Its definitely not ok if the page is not a slab page.

Quoted 7 lines

Quoted 2 lines
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?
I can put this behind CHECK_DATA_CORRUPTION() instead of BUG(), which allows the system builder to choose between WARN and BUG. Some people absolutely want the kernel to BUG on data corruption as it could be an attack.

CHECK_DATA_CORRUPTION sounds as better fit to me. This would, however require to handle the potenial corruption by returning and leaking the memory.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kees Cook

author

2017-04-04 16:02 UTC #11 lore raw

Quoted 16 lines · Michal Hocko

Quoted 15 lines · Kees Cook
Quoted 13 lines · Michal Hocko
Quoted 9 lines · Cristopher Lameter
Quoted 3 lines · Michal Hocko
NAK without a proper changelog. Seriously, we do not blindly apply changes from other projects without a deep understanding of all consequences.
Functionalitywise this is trivial. A page must be a slab page in order to be able to determine the slab cache of an object. Its definitely not ok if the page is not a slab page.
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?

Quoted 8 lines

Quoted 4 lines
I can put this behind CHECK_DATA_CORRUPTION() instead of BUG(), which allows the system builder to choose between WARN and BUG. Some people absolutely want the kernel to BUG on data corruption as it could be an attack.
CHECK_DATA_CORRUPTION sounds as better fit to me. This would, however require to handle the potenial corruption by returning and leaking the memory.

IIUC, that would be the "return s" path? I should likely change the WARN_ON_ONCE there to be CHECK_DATA_CORRUPTION too. I'll add this to my series.

-Kees

Signature · 8 lines

Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-04 19:13 UTC #12 lore raw

In reply to Michal Hocko

Quoted 2 lines · Michal Hocko

Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?

Because it is a serious bug to attempt to free a non slab object using slab operations. This is often the result of memory corruption, coding errs etc. The system needs to stop right there.

Michal Hocko

2017-04-04 19:42 UTC #13 lore raw

Quoted 8 lines · Cristopher Lameter

Quoted 2 lines · Michal Hocko
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?
Because it is a serious bug to attempt to free a non slab object using slab operations. This is often the result of memory corruption, coding errs etc. The system needs to stop right there.

Why when an alternative is a memory leak?

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-04 19:58 UTC #14 lore raw

Quoted 11 lines · Michal Hocko

Quoted 8 lines · Cristopher Lameter
Quoted 2 lines · Michal Hocko
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?
Because it is a serious bug to attempt to free a non slab object using slab operations. This is often the result of memory corruption, coding errs etc. The system needs to stop right there.
Why when an alternative is a memory leak?

Because the slab allocators fail also in case you free an object multiple times etc etc. Continuation is supported by enabling a special resiliency feature via the kernel command line. The alternative is selectable but not the default.

Michal Hocko

2017-04-04 20:13 UTC #15 lore raw

Quoted 12 lines · Cristopher Lameter

Quoted 10 lines · Michal Hocko
Quoted 8 lines · Cristopher Lameter
Quoted 2 lines · Michal Hocko
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?
Because it is a serious bug to attempt to free a non slab object using slab operations. This is often the result of memory corruption, coding errs etc. The system needs to stop right there.

Quoted 6 lines

Quoted 1 line
Why when an alternative is a memory leak?
Because the slab allocators fail also in case you free an object multiple times etc etc. Continuation is supported by enabling a special resiliency feature via the kernel command line. The alternative is selectable but not the default.

I disagree! We should try to continue as long as we _know_ that the internal state of the allocator is still consistent and a further operation will not spread the corruption even more. This is clearly not the case for an invalid pointer to kfree.

I can see why checking for an early allocator corruption is not always feasible and you can only detect after-the-fact but this is not the case here and putting your system down just because some buggy code is trying to free something it hasn't allocated is not really useful. I completely agree with Linus that we overuse BUG way too much and this is just another example of it.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kees Cook

author

2017-04-11 04:58 UTC #16 lore raw

Quoted 25 lines · Michal Hocko

Quoted 12 lines · Cristopher Lameter
Quoted 10 lines · Michal Hocko
Quoted 8 lines · Cristopher Lameter
Quoted 2 lines · Michal Hocko
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?
Because it is a serious bug to attempt to free a non slab object using slab operations. This is often the result of memory corruption, coding errs etc. The system needs to stop right there.
Quoted 6 lines
Quoted 1 line
Why when an alternative is a memory leak?
Because the slab allocators fail also in case you free an object multiple times etc etc. Continuation is supported by enabling a special resiliency feature via the kernel command line. The alternative is selectable but not the default.
I disagree! We should try to continue as long as we _know_ that the internal state of the allocator is still consistent and a further operation will not spread the corruption even more. This is clearly not the case for an invalid pointer to kfree.

Quoted 6 lines

I can see why checking for an early allocator corruption is not always feasible and you can only detect after-the-fact but this is not the case here and putting your system down just because some buggy code is trying to free something it hasn't allocated is not really useful. I completely agree with Linus that we overuse BUG way too much and this is just another example of it.

Instead of the proposed BUG here, what's the correct "safe" return value?

-Kees

Signature · 8 lines

Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michal Hocko

2017-04-11 13:46 UTC #17 lore raw

Quoted 26 lines · Kees Cook

Quoted 25 lines · Michal Hocko
Quoted 12 lines · Cristopher Lameter
Quoted 10 lines · Michal Hocko
Quoted 8 lines · Cristopher Lameter
Quoted 2 lines · Michal Hocko
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?
Because it is a serious bug to attempt to free a non slab object using slab operations. This is often the result of memory corruption, coding errs etc. The system needs to stop right there.
Quoted 6 lines
Quoted 1 line
Why when an alternative is a memory leak?
Because the slab allocators fail also in case you free an object multiple times etc etc. Continuation is supported by enabling a special resiliency feature via the kernel command line. The alternative is selectable but not the default.
I disagree! We should try to continue as long as we _know_ that the internal state of the allocator is still consistent and a further operation will not spread the corruption even more. This is clearly not the case for an invalid pointer to kfree.

Quoted 8 lines

Quoted 6 lines
I can see why checking for an early allocator corruption is not always feasible and you can only detect after-the-fact but this is not the case here and putting your system down just because some buggy code is trying to free something it hasn't allocated is not really useful. I completely agree with Linus that we overuse BUG way too much and this is just another example of it.
Instead of the proposed BUG here, what's the correct "safe" return value?

I would assume that _you_ as the one who proposes the change would take some time to read and understand the code and know this answer. This is how we do changes to the kernel: have an objective, understand the code and generate the patch.

I am really sad that this particular patch has shown that you didn't bother to consider the later part and blindly applied something that you haven't thought through properly. Please try harder next time.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kees Cook

author

2017-04-11 14:14 UTC #18 lore raw

Quoted 36 lines · Michal Hocko

Quoted 26 lines · Kees Cook
Quoted 25 lines · Michal Hocko
Quoted 12 lines · Cristopher Lameter
Quoted 10 lines · Michal Hocko
Quoted 8 lines · Cristopher Lameter
Quoted 2 lines · Michal Hocko
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?
Because it is a serious bug to attempt to free a non slab object using slab operations. This is often the result of memory corruption, coding errs etc. The system needs to stop right there.
Quoted 6 lines
Quoted 1 line
Why when an alternative is a memory leak?
Because the slab allocators fail also in case you free an object multiple times etc etc. Continuation is supported by enabling a special resiliency feature via the kernel command line. The alternative is selectable but not the default.
I disagree! We should try to continue as long as we _know_ that the internal state of the allocator is still consistent and a further operation will not spread the corruption even more. This is clearly not the case for an invalid pointer to kfree.
Quoted 8 lines
Quoted 6 lines
I can see why checking for an early allocator corruption is not always feasible and you can only detect after-the-fact but this is not the case here and putting your system down just because some buggy code is trying to free something it hasn't allocated is not really useful. I completely agree with Linus that we overuse BUG way too much and this is just another example of it.
Instead of the proposed BUG here, what's the correct "safe" return value?

Quoted 8 lines

I would assume that _you_ as the one who proposes the change would take some time to read and understand the code and know this answer. This is how we do changes to the kernel: have an objective, understand the code and generate the patch.
I am really sad that this particular patch has shown that you didn't bother to consider the later part and blindly applied something that you haven't thought through properly. Please try harder next time.

Our objectives are different: I want the kernel to immediately stop when corruption is detected. Since others are interested in making it survivable, I was hoping to get a hint about what such an improvement would look like. Instead this condescending attitude, can you instead provide constructive help that will get our users closer to the safe kernel operation we're all interested in?

-Kees

Signature · 8 lines

Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michal Hocko

2017-04-11 14:20 UTC #19 lore raw

Quoted 42 lines · Kees Cook

Quoted 41 lines · Michal Hocko
Quoted 26 lines · Kees Cook
Quoted 25 lines · Michal Hocko
Quoted 12 lines · Cristopher Lameter
Quoted 10 lines · Michal Hocko
Quoted 8 lines · Cristopher Lameter
Quoted 2 lines · Michal Hocko
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?
Because it is a serious bug to attempt to free a non slab object using slab operations. This is often the result of memory corruption, coding errs etc. The system needs to stop right there.
Quoted 6 lines
Quoted 1 line
Why when an alternative is a memory leak?
Because the slab allocators fail also in case you free an object multiple times etc etc. Continuation is supported by enabling a special resiliency feature via the kernel command line. The alternative is selectable but not the default.
I disagree! We should try to continue as long as we _know_ that the internal state of the allocator is still consistent and a further operation will not spread the corruption even more. This is clearly not the case for an invalid pointer to kfree.
Quoted 8 lines
Quoted 6 lines
I can see why checking for an early allocator corruption is not always feasible and you can only detect after-the-fact but this is not the case here and putting your system down just because some buggy code is trying to free something it hasn't allocated is not really useful. I completely agree with Linus that we overuse BUG way too much and this is just another example of it.
Instead of the proposed BUG here, what's the correct "safe" return value?
I would assume that _you_ as the one who proposes the change would take some time to read and understand the code and know this answer. This is how we do changes to the kernel: have an objective, understand the code and generate the patch.

Quoted 8 lines

Quoted 3 lines
I am really sad that this particular patch has shown that you didn't bother to consider the later part and blindly applied something that you haven't thought through properly. Please try harder next time.
Our objectives are different: I want the kernel to immediately stop when corruption is detected. Since others are interested in making it survivable, I was hoping to get a hint about what such an improvement would look like.

I do not think sprinkling BUG_ONs will help that objective. And BUG_ON under IRQ disable is likely not helping an error survivable...

Quoted 3 lines

Instead this condescending attitude, can you instead provide constructive help that will get our users closer to the safe kernel operation we're all interested in?

I would do something like...
---

mm/slab.c +11 -2

index bd63450a9b16..87c99a5e9e18 100644
@@ -393,10 +393,15 @@ static inline void set_store_user_dirty(struct kmem_cache *cachep) {}
 static int slab_max_order = SLAB_MAX_ORDER_LO;
 static bool slab_max_order_set __initdata;
 
+static inline struct kmem_cache *page_to_cache(struct page *page)
+{
+	return page->slab_cache;
+}
+
 static inline struct kmem_cache *virt_to_cache(const void *obj)
 {
 	struct page *page = virt_to_head_page(obj);
-	return page->slab_cache;
+	return page_to_cache(page);
 }
 
 static inline void *index_to_obj(struct kmem_cache *cache, struct page *page,

@@ -3813,14 +3818,18 @@ void kfree(const void *objp)
 {
 	struct kmem_cache *c;
 	unsigned long flags;
+	struct page *page;
 
 	trace_kfree(_RET_IP_, objp);
 
 	if (unlikely(ZERO_OR_NULL_PTR(objp)))
 		return;
+	page = virt_to_head_page(obj);
+	if (CHECK_DATA_CORRUPTION(!PageSlab(page)))
+		return;
 	local_irq_save(flags);
 	kfree_debugcheck(objp);
-	c = virt_to_cache(objp);
+	c = page_to_cache(page);
 	debug_check_no_locks_freed(objp, c->object_size);
 
 	debug_check_no_obj_freed(objp, c->object_size);

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kees Cook

author

2017-04-11 16:05 UTC #20 lore raw

Quoted 47 lines · Michal Hocko

Quoted 46 lines · Kees Cook
Quoted 44 lines · Michal Hocko
Quoted 26 lines · Kees Cook
Quoted 25 lines · Michal Hocko
Quoted 12 lines · Cristopher Lameter
Quoted 10 lines · Michal Hocko
Quoted 8 lines · Cristopher Lameter
Quoted 2 lines · Michal Hocko
Yes, but we do not have to blow the kernel, right? Why cannot we simply leak that memory?
Because it is a serious bug to attempt to free a non slab object using slab operations. This is often the result of memory corruption, coding errs etc. The system needs to stop right there.
Quoted 6 lines
Quoted 1 line
Why when an alternative is a memory leak?
Because the slab allocators fail also in case you free an object multiple times etc etc. Continuation is supported by enabling a special resiliency feature via the kernel command line. The alternative is selectable but not the default.
I disagree! We should try to continue as long as we _know_ that the internal state of the allocator is still consistent and a further operation will not spread the corruption even more. This is clearly not the case for an invalid pointer to kfree.
Quoted 8 lines
Quoted 6 lines
I can see why checking for an early allocator corruption is not always feasible and you can only detect after-the-fact but this is not the case here and putting your system down just because some buggy code is trying to free something it hasn't allocated is not really useful. I completely agree with Linus that we overuse BUG way too much and this is just another example of it.
Instead of the proposed BUG here, what's the correct "safe" return value?
I would assume that _you_ as the one who proposes the change would take some time to read and understand the code and know this answer. This is how we do changes to the kernel: have an objective, understand the code and generate the patch.
I am really sad that this particular patch has shown that you didn't bother to consider the later part and blindly applied something that you haven't thought through properly. Please try harder next time.

Quoted 7 lines

Quoted 4 lines
Our objectives are different: I want the kernel to immediately stop when corruption is detected. Since others are interested in making it survivable, I was hoping to get a hint about what such an improvement would look like.
I do not think sprinkling BUG_ONs will help that objective. And BUG_ON under IRQ disable is likely not helping an error survivable...

Yes, agreed. Handling it cleanly is always better.

Quoted 37 lines

Quoted 3 lines

Instead this condescending attitude, can you instead provide constructive help that will get our users closer to the safe kernel operation we're all interested in?

I would do something like...
---

mm/slab.c +7 -1

index bd63450a9b16..87c99a5e9e18 100644
@@ -393,10 +393,15 @@ static inline void set_store_user_dirty(struct kmem_cache *cachep) {}
 static int slab_max_order = SLAB_MAX_ORDER_LO;
 static bool slab_max_order_set __initdata;
 
+static inline struct kmem_cache *page_to_cache(struct page *page)
+{
+       return page->slab_cache;
+}
+
 static inline struct kmem_cache *virt_to_cache(const void *obj)
 {
        struct page *page = virt_to_head_page(obj);
-       return page->slab_cache;
+       return page_to_cache(page);
 }
 
 static inline void *index_to_obj(struct kmem_cache *cache, struct page *page,

@@ -3813,14 +3818,18 @@ void kfree(const void *objp)
 {
        struct kmem_cache *c;
        unsigned long flags;
+       struct page *page;
 
        trace_kfree(_RET_IP_, objp);
 
        if (unlikely(ZERO_OR_NULL_PTR(objp)))
                return;

mm/slab.c void kfree(const void *objp) · line 3835

Quoted 10 lines

mm/slab.c +4 -1

+       page = virt_to_head_page(obj);
+       if (CHECK_DATA_CORRUPTION(!PageSlab(page)))
+               return;
        local_irq_save(flags);
        kfree_debugcheck(objp);
-       c = virt_to_cache(objp);
+       c = page_to_cache(page);
        debug_check_no_locks_freed(objp, c->object_size);
 
        debug_check_no_obj_freed(objp, c->object_size);

Awesome! Thank you very much! I'll play with this.

-Kees

Signature · 8 lines

Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-11 16:16 UTC #21 lore raw

In reply to Michal Hocko

mm/slab.c void kfree(const void *objp) · line 3828

Quoted 13 lines · Michal Hocko

static inline void *index_to_obj(struct kmem_cache *cache, struct page *page,

mm/slab.c +3 -0

@@ -3813,14 +3818,18 @@ void kfree(const void *objp)
 {
 	struct kmem_cache *c;
 	unsigned long flags;
+	struct page *page;
 
 	trace_kfree(_RET_IP_, objp);
 
 	if (unlikely(ZERO_OR_NULL_PTR(objp)))
 		return;
+	page = virt_to_head_page(obj);
+	if (CHECK_DATA_CORRUPTION(!PageSlab(page)))

There is a flag SLAB_DEBUG_OBJECTS that is available for this check. Consistency checks are configuraable in the slab allocator.

Mentioned that before and got this lecture about data consistency checks.

Kees Cook

author

2017-04-11 16:19 UTC #22 lore raw

Quoted 19 lines · Christoph Lameter

mm/slab.c void kfree(const void *objp) · line 3828
Quoted 13 lines · Michal Hocko
static inline void *index_to_obj(struct kmem_cache *cache, struct page *page,
mm/slab.c +3 -0
@@ -3813,14 +3818,18 @@ void kfree(const void *objp)
 {
      struct kmem_cache *c;
      unsigned long flags;
+     struct page *page;
 
      trace_kfree(_RET_IP_, objp);
 
      if (unlikely(ZERO_OR_NULL_PTR(objp)))
              return;
+     page = virt_to_head_page(obj);
+     if (CHECK_DATA_CORRUPTION(!PageSlab(page)))
There is a flag SLAB_DEBUG_OBJECTS that is available for this check. Consistency checks are configuraable in the slab allocator.

Quoted 1 line

Mentioned that before and got this lecture about data consistency checks.

It seems that enabling the debug checks comes with a non-trivial performance impact. I'd like to see consistency checks by default so we can handle intentional heap corruption attacks better. This check isn't expensive...

-Kees

Signature · 8 lines

Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-11 16:23 UTC #23 lore raw

Quoted 4 lines · Kees Cook

It seems that enabling the debug checks comes with a non-trivial performance impact. I'd like to see consistency checks by default so we can handle intentional heap corruption attacks better. This check isn't expensive...

Its in a very hot code and frequently used code path.

Kees Cook

author

2017-04-11 16:31 UTC #24 lore raw

Quoted 8 lines · Christoph Lameter

Quoted 4 lines · Kees Cook
It seems that enabling the debug checks comes with a non-trivial performance impact. I'd like to see consistency checks by default so we can handle intentional heap corruption attacks better. This check isn't expensive...
Its in a very hot code and frequently used code path.

Yeah, absolutely. All the more reason to make sure the kernel can't be attacked through it. :) As with the automotive industry analogy[1] from Konstantin, we need to make sure Linux not only run fast and efficiently, but also fails gracefully by default.

Quoted 2 lines

Note also that these checks can be enabled and disabled at runtime for each slab cache.

Correct, but my understanding is that enabling them through the debug system ends up being much more expensive than this smaller check. The debug code is fairly comprehensive, but it's not been designed for efficient attack detection, etc.

-Kees

[1] http://kernsec.org/files/lss2015/giant-bags-of-mostly-water.pdf

Signature · 8 lines

Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-11 16:26 UTC #25 lore raw

In reply to Kees Cook

Quoted 4 lines · Kees Cook

It seems that enabling the debug checks comes with a non-trivial performance impact. I'd like to see consistency checks by default so we can handle intentional heap corruption attacks better. This check isn't expensive...

Note also that these checks can be enabled and disabled at runtime for each slab cache.

Michal Hocko

2017-04-11 16:41 UTC #26 lore raw

In reply to Christoph Lameter

Quoted 9 lines · Cristopher Lameter

Quoted 7 lines · Michal Hocko
static inline void *index_to_obj(struct kmem_cache *cache, struct page *page,
mm/slab.c +1 -0
@@ -3813,14 +3818,18 @@ void kfree(const void *objp)
 {
 	struct kmem_cache *c;
 	unsigned long flags;
+	struct page *page;
 

Quoted 8 lines

mm/slab.c void kfree(const void *objp) · line 3828
Quoted 6 lines
mm/slab.c +2 -0
 	trace_kfree(_RET_IP_, objp);
 
 	if (unlikely(ZERO_OR_NULL_PTR(objp)))
 		return;
+	page = virt_to_head_page(obj);
+	if (CHECK_DATA_CORRUPTION(!PageSlab(page)))
There is a flag SLAB_DEBUG_OBJECTS that is available for this check.

Which is way too late, at least for the kfree path. page->slab_cache on anything else than PageSlab is just a garbage. And my understanding of the patch objective is to stop those from happening.

Quoted 1 line

Consistency checks are configuraable in the slab allocator.

and they have to be compiled in (at least for SLAB) AFAIR.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-11 18:03 UTC #27 lore raw

Quoted 6 lines · Michal Hocko

Quoted 2 lines
There is a flag SLAB_DEBUG_OBJECTS that is available for this check.
Which is way too late, at least for the kfree path. page->slab_cache on anything else than PageSlab is just a garbage. And my understanding of the patch objective is to stop those from happening.

We are looking here at SLAB. SLUB code can legitimately have a compound page there because large allocations fallback to the page allocator.

Garbage would be attempting to free a page that has !PageSLAB set but also is no compound page. That condition is already checked in kfree() with a BUG_ON() and that BUG_ON has been there for a long time. Certainly we can make SLAB consistent if there is no check there already. Slab just attempts a free on that object which will fail too.

So we are already handling that condition. Why change things? Add a BUG_ON if you want to make SLAB consistent.

Michal Hocko

2017-04-11 18:30 UTC #28 lore raw

Quoted 9 lines · Cristopher Lameter

Quoted 6 lines · Michal Hocko
Quoted 2 lines
There is a flag SLAB_DEBUG_OBJECTS that is available for this check.
Which is way too late, at least for the kfree path. page->slab_cache on anything else than PageSlab is just a garbage. And my understanding of the patch objective is to stop those from happening.

Quoted 6 lines

We are looking here at SLAB. SLUB code can legitimately have a compound page there because large allocations fallback to the page allocator.
Garbage would be attempting to free a page that has !PageSLAB set but also is no compound page. That condition is already checked in kfree() with a BUG_ON() and that BUG_ON has been there for a long time.

Are you talking about SLAB or SLUB here? The only BUG_ON(PageSlab(page)) in SLAB I can see is in kmem_freepages and that is way too late because we already rely on cachep which is not trustworthy. Or am I missing some other place you have in mind?

Quoted 6 lines

Certainly we can
make SLAB consistent if there is no check there already. Slab just attempts a free on that object which will fail too.
So we are already handling that condition. Why change things? Add a BUG_ON if you want to make SLAB consistent.

I hate to repeat myself but let me do it for the last time in this thread. BUG_ON for something that is recoverable is completely inappropriate. And I consider kfree with a bogus pointer something that we can easily recover from. There are other cases where the internal state of the allocator is compromised to the point where continuing is not possible and BUGing there is acceptable but kfree(garbage) is not that case.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-11 18:44 UTC #29 lore raw

Quoted 10 lines · Michal Hocko

Quoted 2 lines
So we are already handling that condition. Why change things? Add a BUG_ON if you want to make SLAB consistent.
I hate to repeat myself but let me do it for the last time in this thread. BUG_ON for something that is recoverable is completely inappropriate. And I consider kfree with a bogus pointer something that we can easily recover from. There are other cases where the internal state of the allocator is compromised to the point where continuing is not possible and BUGing there is acceptable but kfree(garbage) is not that case.

kfree(garbage) by the core kernel has so far been taken as a sign of severe memory corruption and the kernels have been oopsing when this occurred. This has been that way for a decade or so. kfree() is used by the allocators and various other core kernel components. If the metadata of the core kernel is compromised then it is safest to stop right there.

If you want to change things then someone has to do some work. What you are saying is not the way things are implemented. Sorry.

Making both allocators consistent is ok with me and is a improvement of the code.

Michal Hocko

2017-04-11 18:56 UTC #30 lore raw

Quoted 13 lines · Cristopher Lameter

Quoted 10 lines · Michal Hocko
Quoted 2 lines
So we are already handling that condition. Why change things? Add a BUG_ON if you want to make SLAB consistent.
I hate to repeat myself but let me do it for the last time in this thread. BUG_ON for something that is recoverable is completely inappropriate. And I consider kfree with a bogus pointer something that we can easily recover from. There are other cases where the internal state of the allocator is compromised to the point where continuing is not possible and BUGing there is acceptable but kfree(garbage) is not that case.

Quoted 3 lines

kfree(garbage) by the core kernel has so far been taken as a sign of severe memory corruption and the kernels have been oopsing when this occurred. This has been that way for a decade or so.

which doesn't make it a valid decision. We just overuse BUG*

Quoted 6 lines

kfree() is used by
the allocators and various other core kernel components. If the metadata of the core kernel is compromised then it is safest to stop right there.
If you want to change things then someone has to do some work. What you are saying is not the way things are implemented. Sorry.

I didn't say anything like that. Hence the proposed patch which still needs some more thinking and evaluation.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-11 18:59 UTC #31 lore raw

Quoted 2 lines · Michal Hocko

I didn't say anything like that. Hence the proposed patch which still needs some more thinking and evaluation.

This patch does not even affect kfree(). Could you start another discussion thread where you discuss your suggestions for the changes in the allocators and how we could go about this?

Michal Hocko

2017-04-11 19:39 UTC #32 lore raw

Quoted 6 lines · Cristopher Lameter

Quoted 2 lines · Michal Hocko
I didn't say anything like that. Hence the proposed patch which still needs some more thinking and evaluation.
This patch does not even affect kfree().

Ehm? Are we even talking about the same thing? The whole discussion was to catch invalid pointers to _kfree_ and why BUG* is not the best way to handle that.

Quoted 3 lines

Could you start another
discussion thread where you discuss your suggestions for the changes in the allocators and how we could go about this?

I presume Kees will pursue
http://lkml.kernel.org/r/[email protected] or something along those lines.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-17 15:22 UTC #33 lore raw

Quoted 11 lines · Michal Hocko

Quoted 6 lines · Cristopher Lameter
Quoted 2 lines · Michal Hocko
I didn't say anything like that. Hence the proposed patch which still needs some more thinking and evaluation.
This patch does not even affect kfree().
Ehm? Are we even talking about the same thing? The whole discussion was to catch invalid pointers to _kfree_ and why BUG* is not the best way to handle that.

The patch does not do that. See my review. Invalid points to kfree are already caught with a bug on. See kfree in mm/slub.c

Michal Hocko

2017-04-18 06:41 UTC #34 lore raw

Quoted 8 lines · Cristopher Lameter

Quoted 6 lines · Michal Hocko
Quoted 5 lines · Cristopher Lameter
Quoted 2 lines · Michal Hocko
I didn't say anything like that. Hence the proposed patch which still needs some more thinking and evaluation.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
This patch does not even affect kfree().
Ehm? Are we even talking about the same thing? The whole discussion was to catch invalid pointers to _kfree_ and why BUG* is not the best way to handle that.
The patch does not do that. See my review. Invalid points to kfree are already caught with a bug on. See kfree in mm/slub.c

Are you even reading those emails? First of all we are talking about slab here. Secondly I've already pointed out that the BUG_ON(!PageSlab) in kmem_freepages is already too late because we do operate on a potential garbage from invalid page...

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2017-04-18 13:31 UTC #35 lore raw

Quoted 7 lines · Michal Hocko

Quoted 2 lines
The patch does not do that. See my review. Invalid points to kfree are already caught with a bug on. See kfree in mm/slub.c
Are you even reading those emails? First of all we are talking about slab here. Secondly I've already pointed out that the BUG_ON(!PageSlab) in kmem_freepages is already too late because we do operate on a potential garbage from invalid page...

Again this is confusing because you are discussing something that was not covered by the patch submitted. Please start another discussion thread on kfree() separately from the discussion of the patch here.

Christoph Lameter

2017-04-18 13:37 UTC #36 lore raw

In reply to Michal Hocko

Quoted 4 lines · Michal Hocko

Are you even reading those emails? First of all we are talking about slab here. Secondly I've already pointed out that the BUG_ON(!PageSlab) in kmem_freepages is already too late because we do operate on a potential garbage from invalid page...

Before I forget:

1. The patch affects both slab and slub since it patches mm/slab.h and is called by both allocators.

2. The check in the patch we are discussing here when calling kmem_cache_free() will be executing before kmem_freepages() is called in slab.

Kees Cook

author

2017-04-28 01:11 UTC #37 lore raw

In reply to Michal Hocko

Quoted 33 lines · Michal Hocko

I would do something like...
---

mm/slab.c +7 -1

index bd63450a9b16..87c99a5e9e18 100644
@@ -393,10 +393,15 @@ static inline void set_store_user_dirty(struct kmem_cache *cachep) {}
 static int slab_max_order = SLAB_MAX_ORDER_LO;
 static bool slab_max_order_set __initdata;
 
+static inline struct kmem_cache *page_to_cache(struct page *page)
+{
+       return page->slab_cache;
+}
+
 static inline struct kmem_cache *virt_to_cache(const void *obj)
 {
        struct page *page = virt_to_head_page(obj);
-       return page->slab_cache;
+       return page_to_cache(page);
 }
 
 static inline void *index_to_obj(struct kmem_cache *cache, struct page *page,

@@ -3813,14 +3818,18 @@ void kfree(const void *objp)
 {
        struct kmem_cache *c;
        unsigned long flags;
+       struct page *page;
 
        trace_kfree(_RET_IP_, objp);
 
        if (unlikely(ZERO_OR_NULL_PTR(objp)))
                return;

mm/slab.c void kfree(const void *objp) · line 3835

Quoted 10 lines

mm/slab.c +4 -1

+       page = virt_to_head_page(obj);
+       if (CHECK_DATA_CORRUPTION(!PageSlab(page)))
+               return;
        local_irq_save(flags);
        kfree_debugcheck(objp);
-       c = virt_to_cache(objp);
+       c = page_to_cache(page);
        debug_check_no_locks_freed(objp, c->object_size);
 
        debug_check_no_obj_freed(objp, c->object_size);

Sorry for the delay, I've finally had time to look at this again.

So, this only handles the kfree() case, not the kmem_cache_free() nor kmem_cache_free_bulk() cases, so it misses all the non-kmalloc allocations (and kfree() ultimately calls down to kmem_cache_free()). Similarly, my proposed patch missed the kfree() path. :P

As I work on a replacement, is the goal to avoid the checks while under local_irq_save()? (i.e. I can't just put the check in virt_to_cache(), etc.)

-Kees

Signature · 8 lines

Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michal Hocko

2017-04-28 06:16 UTC #38 lore raw

Quoted 43 lines · Kees Cook

Quoted 42 lines · Michal Hocko

I would do something like...
---

mm/slab.c +11 -2

index bd63450a9b16..87c99a5e9e18 100644
@@ -393,10 +393,15 @@ static inline void set_store_user_dirty(struct kmem_cache *cachep) {}
 static int slab_max_order = SLAB_MAX_ORDER_LO;
 static bool slab_max_order_set __initdata;
 
+static inline struct kmem_cache *page_to_cache(struct page *page)
+{
+       return page->slab_cache;
+}
+
 static inline struct kmem_cache *virt_to_cache(const void *obj)
 {
        struct page *page = virt_to_head_page(obj);
-       return page->slab_cache;
+       return page_to_cache(page);
 }
 
 static inline void *index_to_obj(struct kmem_cache *cache, struct page *page,

@@ -3813,14 +3818,18 @@ void kfree(const void *objp)
 {
        struct kmem_cache *c;
        unsigned long flags;
+       struct page *page;
 
        trace_kfree(_RET_IP_, objp);
 
        if (unlikely(ZERO_OR_NULL_PTR(objp)))
                return;
+       page = virt_to_head_page(obj);
+       if (CHECK_DATA_CORRUPTION(!PageSlab(page)))
+               return;
        local_irq_save(flags);
        kfree_debugcheck(objp);
-       c = virt_to_cache(objp);
+       c = page_to_cache(page);
        debug_check_no_locks_freed(objp, c->object_size);

Quoted 8 lines

mm/slab.c void kfree(const void *objp) · line 3835
Quoted 1 line
mm/slab.c +0 -0
        debug_check_no_obj_freed(objp, c->object_size);
Sorry for the delay, I've finally had time to look at this again.
So, this only handles the kfree() case, not the kmem_cache_free() nor kmem_cache_free_bulk() cases, so it misses all the non-kmalloc allocations (and kfree() ultimately calls down to kmem_cache_free()). Similarly, my proposed patch missed the kfree() path. :P

yes

Quoted 3 lines

As I work on a replacement, is the goal to avoid the checks while under local_irq_save()? (i.e. I can't just put the check in virt_to_cache(), etc.)

You would have to check all callers of virt_to_cache. I would simply replace BUG_ON(!PageSlab()) in cache_from_obj. kmem_cache_free already handles NULL cache. kmem_cache_free_bulk and build_detached_freelist can be made to do so.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michal Hocko

2017-04-27 12:06 UTC #39 lore raw

In reply to Michal Hocko

Quoted 9 lines · Michal Hocko

Quoted 4 lines · Kees Cook
As found in PaX, this adds a cheap check on heap consistency, just to notice if things have gotten corrupted in the page lookup.
Signed-off-by Kees Cook <[email protected]>
NAK without a proper changelog. Seriously, we do not blindly apply changes from other projects without a deep understanding of all consequences.

This still seems to be in the mmotm tree
http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-add-additional-consistency-check.patch
I hope we have agreed that this is not the right approach to handle invalid pointers in kfree and rather go soemthing like
http://lkml.kernel.org/r/[email protected]
instead.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

Acked-by on v1 1/1

2017-04-11 18:30 UTC #40 lore raw

In reply to Kees Cook

Quoted 2 lines · Kees Cook

As found in PaX, this adds a cheap check on heap consistency, just to notice if things have gotten corrupted in the page lookup.

Ok this only affects kmem_cache_free() and not kfree(). For kmem_cache_free() we already have a lot of stuff in the hotpath due to cgruops. If you want this also for kfree() then we need a separate patch.

Also for kmem_cache_free(): Here we always have a slab cache and thus we could check the flags that could modify what behavior we want.

Acked-by Christoph Lameter <[email protected]>