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

[Harry Yoo <harry.yoo@oracle.com>]

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