Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist()

[Vlastimil Babka <vbabka@suse.cz>]

On 3/4/25 12:06, Lilith Gkini wrote:
> On Tue, Mar 04, 2025 at 09:41:23AM +0100, Vlastimil Babka wrote:
>> It sets the tail to NULL but then also breaks out of the loop (btw that
>> break; could be moved to the if (object) branch to make it more obvious) to
>> the code below, which should also set slab->inuse properly. So the result
>> should be consistent? In that case we're able to salvage at least the
>> uncorrupted part of the freelist. It's likely corrupted by a use-after-free
>> of a single object overwriting the freepointer.
> 
> Yes! You are right!
> 
> I also just tested this. The "Freelist cycle detected" will get
> triggered even if there is an invalid address at the tail in the case
> of a full freelist, which is a bit... inacurate, right? It's technically

Yes. But see my comments on the code below. I wonder why you got it triggered.

> not a cycle in that case since the freepointer is invalid and it doesn't
> point back to the slab.
> 
> - We could avoid this by nulling the fp in that case (as I suggested in v1
> in previous emails) inside the "Freechain corrupt" branch, but also
> reverting the while condition back to it's equal sign like it was and
> then changing the new if check to:
> 	if (fp != NULL && nr > slab->objects) {
> but it feels a bit messy.

I think it's not so bad.

> - Or we could just change the "Freelist cycle detected" message to
> something else.
> 
> - Or we could leave it as "Freelist cycle detected".

I'd prefer that.

> This is only a problem if the freelist is full and the tail is junk.

If the tail is junk it would be better to just fix it to NULL and not report
wrongly a cycle.

> If the freelist is not full the code will act as you suggested.
> 
> 
> If this is becoming too hard to follow I'll include the two diffs.
> 
> For the case were we are fine with the "Freelist cycle detected"
> message, even in the case of a junk tail:

<snip>

> 
> -- 
> 
> and in the case where we want the code to not display "Freelist cycle
> detected" we could do something like this:
> 
> ---
>  mm/slub.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 1f50129dcfb3..eef879d4feb1 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1427,7 +1427,7 @@ static int check_slab(struct kmem_cache *s, struct slab *slab)
>   * Determine if a certain object in a slab is on the freelist. Must hold the
>   * slab lock to guarantee that the chains are in a consistent state.
>   */
> -static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
> +static bool on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
>  {
>  	int nr = 0;
>  	void *fp;
> @@ -1437,27 +1437,36 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search)
>  	fp = slab->freelist;
>  	while (fp && nr <= slab->objects) {
>  		if (fp == search)
> -			return 1;
> +			return true;
>  		if (!check_valid_pointer(s, slab, fp)) {
>  			if (object) {
>  				object_err(s, slab, object,
>  					"Freechain corrupt");
>  				set_freepointer(s, object, NULL);
> +				fp = NULL;
> +				break;

Since we break, nr is not incremented to slab->objects + 1.

>  			} else {
>  				slab_err(s, slab, "Freepointer corrupt");
>  				slab->freelist = NULL;
>  				slab->inuse = slab->objects;
>  				slab_fix(s, "Freelist cleared");
> -				return 0;
> +				return false;
>  			}
> -			break;
>  		}
>  		object = fp;
>  		fp = get_freepointer(s, object);
>  		nr++;
>  	}
>  
> -	max_objects = order_objects(slab_order(slab), s->size);
> +	if (fp != NULL && nr > slab->objects) {

And thus we should not need to set fp to NULL and test it here? Am I missing
something?

> +		slab_err(s, slab, "Freelist cycle detected");
> +		slab->freelist = NULL;
> +		slab->inuse = slab->objects;
> +		slab_fix(s, "Freelist cleared");
> +		return false;
> +	}
> +
> +	max_objects = order_objects(slab_or0der(slab), s->size);
>  	if (max_objects > MAX_OBJS_PER_PAGE)
>  		max_objects = MAX_OBJS_PER_PAGE;
>  
> -- 
> 
> Let me know what you think!

The latter would be better, thanks!