In object_err(), need dereference the 'object' pointer, it may cause a invalid pointer fault. Use slab_err() instead.
Signed-off-by: Li Qiong liqiong@nfschina.com --- mm/slub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c index 31e11ef256f9..3a2e57e2e2d7 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1587,7 +1587,7 @@ static inline int alloc_consistency_checks(struct kmem_cache *s, return 0;
if (!check_valid_pointer(s, slab, object)) { - object_err(s, slab, object, "Freelist Pointer check fails"); + slab_err(s, slab, "Freelist Pointer (0x%p) check fails", object); return 0; }
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH] mm: slub: fix dereference invalid pointer in alloc_consistency_checks Link: https://lore.kernel.org/stable/20250725024854.1201926-1-liqiong%40nfschina.c...
On Fri, Jul 25, 2025 at 10:48:54AM +0800, Li Qiong wrote:
In object_err(), need dereference the 'object' pointer, it may cause a invalid pointer fault. Use slab_err() instead.
Hi Li Qiong, this patch makes sense to me. But I'd suggest to rephrase it a little bit, like:
mm/slab: avoid deref of free pointer in sanity checks if object is invalid
For debugging purposes, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for object, `object + s->offset` is also invalid and dereferncing it can lead to a crash. Therefore, avoid dereferencing it and only print the object's address in such cases.
Signed-off-by: Li Qiong liqiong@nfschina.com
Which commit introduced this problem? A Fixes: tag is needed to determine which -stable versions it should be backported to.
And to backport MM patches to -stable, you need to explicitly add 'Cc: stable@vger.kernel.org' to the patch.
mm/slub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c index 31e11ef256f9..3a2e57e2e2d7 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1587,7 +1587,7 @@ static inline int alloc_consistency_checks(struct kmem_cache *s, return 0; if (!check_valid_pointer(s, slab, object)) {
object_err(s, slab, object, "Freelist Pointer check fails");
slab_err(s, slab, "Freelist Pointer (0x%p) check fails", object);
Can this be slab_err(s, slab, "Invalid object pointer 0x%p", object); to align with free_consistency_checks()?
return 0;}
It might be worth adding a comment in object_err() stating that it should only be called when check_valid_pointer() returns true for object, and a WARN_ON_ONCE(!check_valid_pointer(s, slab, object)) to catch incorrect usages?
在 2025/7/25 12:01, Harry Yoo 写道:
On Fri, Jul 25, 2025 at 10:48:54AM +0800, Li Qiong wrote:
In object_err(), need dereference the 'object' pointer, it may cause a invalid pointer fault. Use slab_err() instead.
Hi Li Qiong, this patch makes sense to me. But I'd suggest to rephrase it a little bit, like:
mm/slab: avoid deref of free pointer in sanity checks if object is invalid
For debugging purposes, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for object, `object + s->offset` is also invalid and dereferncing it can lead to a crash. Therefore, avoid dereferencing it and only print the object's address in such cases.
Thanks, I will send a v2 patch.
Signed-off-by: Li Qiong liqiong@nfschina.com
Which commit introduced this problem? A Fixes: tag is needed to determine which -stable versions it should be backported to.
And to backport MM patches to -stable, you need to explicitly add 'Cc: stable@vger.kernel.org' to the patch.
mm/slub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c index 31e11ef256f9..3a2e57e2e2d7 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1587,7 +1587,7 @@ static inline int alloc_consistency_checks(struct kmem_cache *s, return 0; if (!check_valid_pointer(s, slab, object)) {
object_err(s, slab, object, "Freelist Pointer check fails");
slab_err(s, slab, "Freelist Pointer (0x%p) check fails", object);Can this be slab_err(s, slab, "Invalid object pointer 0x%p", object); to align with free_consistency_checks()?
return 0;}
It might be worth adding a comment in object_err() stating that it should only be called when check_valid_pointer() returns true for object, and a WARN_ON_ONCE(!check_valid_pointer(s, slab, object)) to catch incorrect usages?
For debugging, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for a object, dereferncing `object + s->offset` can lead to a crash. Therefore, print the object's address in such cases.
Fixes: bb192ed9aa71 ("mm/slub: Convert most struct page to struct slab by spatch") Signed-off-by: Li Qiong liqiong@nfschina.com --- v2: - rephrase the commit message, add comment for object_err(). --- mm/slub.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c index 31e11ef256f9..8b24f1cf3079 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1097,6 +1097,10 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p) size_from_object(s) - off); }
+/* + * object - should be a valid object. + * check_valid_pointer(s, slab, object) should be true. + */ static void object_err(struct kmem_cache *s, struct slab *slab, u8 *object, const char *reason) { @@ -1587,7 +1591,7 @@ static inline int alloc_consistency_checks(struct kmem_cache *s, return 0;
if (!check_valid_pointer(s, slab, object)) { - object_err(s, slab, object, "Freelist Pointer check fails"); + slab_err(s, slab, "Invalid object pointer 0x%p", object); return 0; }
On 7/25/25 08:49, Li Qiong wrote:
For debugging, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for a object, dereferncing `object + s->offset` can lead to a crash. Therefore, print the object's address in such cases.
Fixes: bb192ed9aa71 ("mm/slub: Convert most struct page to struct slab by spatch")
That was the last commit to change the line, but the problem existed before, AFAICS all the time, so I did:
Fixes: 7656c72b5a63 ("SLUB: add macros for scanning objects in a slab") Cc: stable@vger.kernel.org
Signed-off-by: Li Qiong liqiong@nfschina.com
Added to slab/for-next, thanks!
v2:
- rephrase the commit message, add comment for object_err().
mm/slub.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c index 31e11ef256f9..8b24f1cf3079 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1097,6 +1097,10 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p) size_from_object(s) - off); } +/*
- object - should be a valid object.
- check_valid_pointer(s, slab, object) should be true.
- */
static void object_err(struct kmem_cache *s, struct slab *slab, u8 *object, const char *reason) { @@ -1587,7 +1591,7 @@ static inline int alloc_consistency_checks(struct kmem_cache *s, return 0; if (!check_valid_pointer(s, slab, object)) {
object_err(s, slab, object, "Freelist Pointer check fails");
slab_err(s, slab, "Invalid object pointer 0x%p", object);
On Fri, Jul 25, 2025 at 06:47:01PM +0200, Vlastimil Babka wrote:
On 7/25/25 08:49, Li Qiong wrote:
For debugging, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for a object, dereferncing `object + s->offset` can lead to a crash. Therefore, print the object's address in such cases.
I don't know where this patch came from (was it cc'd to linux-mm? i don't see it)
+/*
- object - should be a valid object.
- check_valid_pointer(s, slab, object) should be true.
- */
This comment is very confusing. It tries to ape kernel-doc style, but if it were kernel-doc, the word before the hyphen should be the name of the function, and it isn't. If we did use kernel-doc for this, we'd use @object to denote that we're documenting the argument.
But I don't see the need to pretend this is related to kernel-doc. This would be better:
/* * 'object' must be a valid pointer into this slab. ie * check_valid_pointer() would return true */
I'm sure better wording for that is possible ...
if (!check_valid_pointer(s, slab, object)) {
object_err(s, slab, object, "Freelist Pointer check fails");
slab_err(s, slab, "Invalid object pointer 0x%p", object);
No, the error message is now wrong. It's not an object, it's the freelist pointer.
slab_err(s, slab, "Invalid freelist pointer %p", object);
(the 0x%p is wrong because it will print 0x twice)
But I think there are even more things wrong here. Like slab_err() is not nerely as severe as slab_bug(), which is what used to be called. And object_err() adds a taint, which this skips.
Altogether, this is a poorly thought out patch and should be dropped.
On Fri, Jul 25, 2025 at 06:10:51PM +0100, Matthew Wilcox wrote:
On Fri, Jul 25, 2025 at 06:47:01PM +0200, Vlastimil Babka wrote:
On 7/25/25 08:49, Li Qiong wrote:
For debugging, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for a object, dereferncing `object + s->offset` can lead to a crash. Therefore, print the object's address in such cases.
I don't know where this patch came from (was it cc'd to linux-mm? i don't see it)
I've spent some more time thinking about this and I now believe that there are several calls to object_err() that can be passed a bad pointer:
freelist_corrupted() check_object() on_freelist() alloc_consistency_checks() free_consistency_checks()
so I think this line of attack is inappropriate. Instead, I think we need to make object_err() resilient against wild pointers. Specifically, avoid doing risky things in print_trailer() if object is not within slab.
On Fri, Jul 25, 2025 at 08:22:05PM +0100, Matthew Wilcox wrote:
On Fri, Jul 25, 2025 at 06:10:51PM +0100, Matthew Wilcox wrote:
On Fri, Jul 25, 2025 at 06:47:01PM +0200, Vlastimil Babka wrote:
On 7/25/25 08:49, Li Qiong wrote:
For debugging, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for a object, dereferncing `object + s->offset` can lead to a crash. Therefore, print the object's address in such cases.
I don't know where this patch came from (was it cc'd to linux-mm? i don't see it)
Oops, I missed this email when I was replying to the previous email.
I've spent some more time thinking about this and I now believe that there are several calls to object_err() that can be passed a bad pointer:
freelist_corrupted() check_object() on_freelist() alloc_consistency_checks() free_consistency_checks()
so I think this line of attack is inappropriate. Instead, I think we need to make object_err() resilient against wild pointers. Specifically, avoid doing risky things in print_trailer() if object is not within slab.
Making object_err() more resilient sounds good to me.
On Fri, Jul 25, 2025 at 06:10:51PM +0100, Matthew Wilcox wrote:
On Fri, Jul 25, 2025 at 06:47:01PM +0200, Vlastimil Babka wrote:
On 7/25/25 08:49, Li Qiong wrote:
For debugging, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for a object, dereferncing `object + s->offset` can lead to a crash. Therefore, print the object's address in such cases.
I don't know where this patch came from (was it cc'd to linux-mm? i don't see it)
https://lore.kernel.org/all/20250725024854.1201926-1-liqiong@nfschina.com
Looks like it's rejected by linux-mm for some reason..
+/*
- object - should be a valid object.
- check_valid_pointer(s, slab, object) should be true.
- */
This comment is very confusing. It tries to ape kernel-doc style, but if it were kernel-doc, the word before the hyphen should be the name of the function, and it isn't. If we did use kernel-doc for this, we'd use @object to denote that we're documenting the argument.
Yes, the comment is indeed confusing and agree with your point.
When I suggested it I expected adding something like:
/* print_trailer() may deref invalid freepointer if object pointer is invalid */ WARN_ON_ONCE(!check_valid_pointer(s, slab, object));
to be added to object_err().
But I don't see the need to pretend this is related to kernel-doc. This would be better:
/*
- 'object' must be a valid pointer into this slab. ie
- check_valid_pointer() would return true
*/
I'm sure better wording for that is possible ...
if (!check_valid_pointer(s, slab, object)) {
object_err(s, slab, object, "Freelist Pointer check fails");
slab_err(s, slab, "Invalid object pointer 0x%p", object);No, the error message is now wrong. It's not an object, it's the freelist pointer.
Because it's the object is about to be allocated, it will look like this:
object pointer -> obj: [ garbage ][ freelist pointer ][ garbage ]
SLUB uses check_valid_pointer() to check either 1) freelist pointer of an object is valid (e.g. in check_object()), or 2) an object pointer points to a valid address (e.g. in free_debug_processing()).
In this case it's an object pointer, not a freelist pointer. Or am I misunderstanding something?
slab_err(s, slab, "Invalid freelist pointer %p", object);(the 0x%p is wrong because it will print 0x twice)
"0x%p" is used all over the place in mm/slub.c.
In the printk documentation [1]:
Plain Pointers %p abcdef12 or 00000000abcdef12
0x%p should be 0xabcdef12 or 0x00000000abcdef12, no?
[1] https://www.kernel.org/doc/html/next/core-api/printk-formats.html#printk-spe...
But I think there are even more things wrong here. Like slab_err() is not nerely as severe as slab_bug(), which is what used to be called.
What do you mean by slab_err() is not as severe as slab_bug()? Both object_err() and slab_err() add a taint and trigger a WARNING.
And object_err() adds a taint, which this skips.
adding a taint is done via slab_err()->__slab_err()->add_taint()
On Sat, Jul 26, 2025 at 04:55:06AM +0900, Harry Yoo wrote:
On Fri, Jul 25, 2025 at 06:10:51PM +0100, Matthew Wilcox wrote:
On Fri, Jul 25, 2025 at 06:47:01PM +0200, Vlastimil Babka wrote:
On 7/25/25 08:49, Li Qiong wrote:
For debugging, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for a object, dereferncing `object + s->offset` can lead to a crash. Therefore, print the object's address in such cases.
if (!check_valid_pointer(s, slab, object)) {
object_err(s, slab, object, "Freelist Pointer check fails");
slab_err(s, slab, "Invalid object pointer 0x%p", object);No, the error message is now wrong. It's not an object, it's the freelist pointer.
Because it's the object is about to be allocated, it will look like this:
object pointer -> obj: [ garbage ][ freelist pointer ][ garbage ]
SLUB uses check_valid_pointer() to check either 1) freelist pointer of an object is valid (e.g. in check_object()), or 2) an object pointer points to a valid address (e.g. in free_debug_processing()).
In this case it's an object pointer, not a freelist pointer. Or am I misunderstanding something?
Actually, in alloc_debug_processing() the pointer came from slab->freelist, so I think saying either "invalid freelist pointer" or "invalid object pointer" make sense...
在 2025/7/26 07:00, Harry Yoo 写道:
On Sat, Jul 26, 2025 at 04:55:06AM +0900, Harry Yoo wrote:
On Fri, Jul 25, 2025 at 06:10:51PM +0100, Matthew Wilcox wrote:
On Fri, Jul 25, 2025 at 06:47:01PM +0200, Vlastimil Babka wrote:
On 7/25/25 08:49, Li Qiong wrote:
For debugging, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for a object, dereferncing `object + s->offset` can lead to a crash. Therefore, print the object's address in such cases. if (!check_valid_pointer(s, slab, object)) {
object_err(s, slab, object, "Freelist Pointer check fails");
slab_err(s, slab, "Invalid object pointer 0x%p", object);No, the error message is now wrong. It's not an object, it's the freelist pointer.
Because it's the object is about to be allocated, it will look like this:
object pointer -> obj: [ garbage ][ freelist pointer ][ garbage ]
SLUB uses check_valid_pointer() to check either 1) freelist pointer of an object is valid (e.g. in check_object()), or 2) an object pointer points to a valid address (e.g. in free_debug_processing()).
In this case it's an object pointer, not a freelist pointer. Or am I misunderstanding something?
Actually, in alloc_debug_processing() the pointer came from slab->freelist, so I think saying either "invalid freelist pointer" or "invalid object pointer" make sense...
free_consistency_checks() has 'slab_err(s, slab, "Invalid object pointer 0x%p", object);' Maybe it is better, alloc_consisency_checks() has the same message.
On Mon, Jul 28, 2025 at 10:06:42AM +0800, liqiong wrote:
In this case it's an object pointer, not a freelist pointer. Or am I misunderstanding something?
Actually, in alloc_debug_processing() the pointer came from slab->freelist, so I think saying either "invalid freelist pointer" or "invalid object pointer" make sense...
free_consistency_checks() has 'slab_err(s, slab, "Invalid object pointer 0x%p", object);' Maybe it is better, alloc_consisency_checks() has the same message.
No. Think about it.
On Mon, Jul 28, 2025 at 04:29:22AM +0100, Matthew Wilcox wrote:
On Mon, Jul 28, 2025 at 10:06:42AM +0800, liqiong wrote:
In this case it's an object pointer, not a freelist pointer. Or am I misunderstanding something?
Actually, in alloc_debug_processing() the pointer came from slab->freelist, so I think saying either "invalid freelist pointer" or "invalid object pointer" make sense...
free_consistency_checks() has 'slab_err(s, slab, "Invalid object pointer 0x%p", object);' Maybe it is better, alloc_consisency_checks() has the same message.
No. Think about it.
Haha, since I suggested that change, I feel like I have to rethink it and respond... Maybe I'm wrong again, but I love to be proven wrong :)
On second thought,
Unlike free_consistency_checks() where an arbitrary address can be passed, alloc_consistency_check() is not passed arbitrary addresses but only addresses from the freelist. So if a pointer is invalid there, it means the freelist pointer is invalid. And in all other parts of slub.c, such cases are described as "Free(list) pointer", or "Freechain" being invalid or corrupted.
So to stay consistent "Invalid freelist pointer" would be the right choice :) Sorry for the confusion.
Anyway, Li, to make progress on this I think it make sense to start by making object_err() resiliant against invalid pointers (as suggested by Matthew)? If you go down that path, this patch might no longer be required to fix the bug anyway...
And the change would be quite small. Most part of print_trailer() is printing metadata and raw content of the object, which is risky when the pointer is invalid. In that case we'd only want to print the address of the invalid pointer and the information about slab (print_slab_info()) and nothing more.
在 2025/7/28 13:24, Harry Yoo 写道:
On Mon, Jul 28, 2025 at 04:29:22AM +0100, Matthew Wilcox wrote:
On Mon, Jul 28, 2025 at 10:06:42AM +0800, liqiong wrote:
In this case it's an object pointer, not a freelist pointer. Or am I misunderstanding something?
Actually, in alloc_debug_processing() the pointer came from slab->freelist, so I think saying either "invalid freelist pointer" or "invalid object pointer" make sense...
free_consistency_checks() has 'slab_err(s, slab, "Invalid object pointer 0x%p", object);' Maybe it is better, alloc_consisency_checks() has the same message.
No. Think about it.
Haha, since I suggested that change, I feel like I have to rethink it and respond... Maybe I'm wrong again, but I love to be proven wrong :)
On second thought,
Unlike free_consistency_checks() where an arbitrary address can be passed, alloc_consistency_check() is not passed arbitrary addresses but only addresses from the freelist. So if a pointer is invalid there, it means the freelist pointer is invalid. And in all other parts of slub.c, such cases are described as "Free(list) pointer", or "Freechain" being invalid or corrupted.
So to stay consistent "Invalid freelist pointer" would be the right choice :) Sorry for the confusion.
Anyway, Li, to make progress on this I think it make sense to start by making object_err() resiliant against invalid pointers (as suggested by Matthew)? If you go down that path, this patch might no longer be required to fix the bug anyway...
And the change would be quite small. Most part of print_trailer() is printing metadata and raw content of the object, which is risky when the pointer is invalid. In that case we'd only want to print the address of the invalid pointer and the information about slab (print_slab_info()) and nothing more.
Got it, I will a v3 patch, changing the message, and keep it simple, dropping the comments of object_err(), just fix the issue.
On Mon, Jul 28, 2025 at 05:08:57PM +0800, liqiong wrote:
在 2025/7/28 13:24, Harry Yoo 写道:
On Mon, Jul 28, 2025 at 04:29:22AM +0100, Matthew Wilcox wrote:
On Mon, Jul 28, 2025 at 10:06:42AM +0800, liqiong wrote:
In this case it's an object pointer, not a freelist pointer. Or am I misunderstanding something?
Actually, in alloc_debug_processing() the pointer came from slab->freelist, so I think saying either "invalid freelist pointer" or "invalid object pointer" make sense...
free_consistency_checks() has 'slab_err(s, slab, "Invalid object pointer 0x%p", object);' Maybe it is better, alloc_consisency_checks() has the same message.
No. Think about it.
Haha, since I suggested that change, I feel like I have to rethink it and respond... Maybe I'm wrong again, but I love to be proven wrong :)
On second thought,
Unlike free_consistency_checks() where an arbitrary address can be passed, alloc_consistency_check() is not passed arbitrary addresses but only addresses from the freelist. So if a pointer is invalid there, it means the freelist pointer is invalid. And in all other parts of slub.c, such cases are described as "Free(list) pointer", or "Freechain" being invalid or corrupted.
So to stay consistent "Invalid freelist pointer" would be the right choice :) Sorry for the confusion.
Anyway, Li, to make progress on this I think it make sense to start by making object_err() resiliant against invalid pointers (as suggested by Matthew)? If you go down that path, this patch might no longer be required to fix the bug anyway...
And the change would be quite small. Most part of print_trailer() is printing metadata and raw content of the object, which is risky when the pointer is invalid. In that case we'd only want to print the address of the invalid pointer and the information about slab (print_slab_info()) and nothing more.
Got it, I will a v3 patch, changing the message, and keep it simple, dropping the comments of object_err(), just fix the issue.
Well, I was saying let's start from making object_err() against wild pointers [1] per Matthew's suggestion.
And with that this patch won't be necessary to fix the issue and will be more robust against similar mistakes like this?
[1] https://lore.kernel.org/linux-mm/aIPZXSnkDF5r-PR5@casper.infradead.org
On 7/25/25 18:47, Vlastimil Babka wrote:
On 7/25/25 08:49, Li Qiong wrote:
For debugging, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for a object, dereferncing `object + s->offset` can lead to a crash. Therefore, print the object's address in such cases.
Fixes: bb192ed9aa71 ("mm/slub: Convert most struct page to struct slab by spatch")
That was the last commit to change the line, but the problem existed before, AFAICS all the time, so I did:
Fixes: 7656c72b5a63 ("SLUB: add macros for scanning objects in a slab") Cc: <stable@vger.kernel.org>Signed-off-by: Li Qiong liqiong@nfschina.com
Added to slab/for-next, thanks!
Dropped based on Matthew's review
v2:
- rephrase the commit message, add comment for object_err().
mm/slub.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c index 31e11ef256f9..8b24f1cf3079 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1097,6 +1097,10 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p) size_from_object(s) - off); } +/*
- object - should be a valid object.
- check_valid_pointer(s, slab, object) should be true.
- */
static void object_err(struct kmem_cache *s, struct slab *slab, u8 *object, const char *reason) { @@ -1587,7 +1591,7 @@ static inline int alloc_consistency_checks(struct kmem_cache *s, return 0; if (!check_valid_pointer(s, slab, object)) {
object_err(s, slab, object, "Freelist Pointer check fails");
slab_err(s, slab, "Invalid object pointer 0x%p", object);
linux-stable-mirror@lists.linaro.org
-
Harry Yoo -
kernel test robot -
Li Qiong -
liqiong
-
Matthew Wilcox -
Vlastimil Babka