Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

[Vlastimil Babka]

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);
>  		return 0;
>  	}
>  

Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

[Matthew Wilcox]

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);
> >  		return 0;

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.

Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

[Matthew Wilcox]

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.

Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

[Harry Yoo]

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.

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

[Harry Yoo]

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);
> > >  		return 0;
> 
> 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-specifiers

> 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()

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

[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);
> > > >  		return 0;
> > 
> > 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...

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

[Matthew Wilcox]

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.

Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

[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.

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

[Harry Yoo]

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

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity checks if object is invalid

[Vlastimil Babka]

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);
>>  		return 0;
>>  	}
>>  
>