linux-kernel - Re: [PATCH] slub: Fix Off-By-One in the While condition in on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z8RZB-Adxp9Lgo0t@harry>
Date: Sun, 2 Mar 2025 22:11:35 +0900
From: Harry Yoo <harry.yoo@...cle.com>
To: Lilith Gkini <lilithpgkini@...il.com>
Cc: Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Hyeonggon Yoo <42.hyeyoo@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] slub: Fix Off-By-One in the While condition in
 on_freelist()

On Thu, Feb 27, 2025 at 06:40:16PM +0200, Lilith Gkini wrote:
> Hey, Harry.
> 
> > Yes. but do we care about the return value of on_freelist()
> > when the freelist is corrupted? (we don't know if it was null-terminated
> > or not because it's corrupted.)
> 
> Oh! So we shouldn't care about the return value of on_freelist() if the
> freelist is corrupted? My thinking was that if the check_valid_pointer()
> detects a corrupted freelist and fixes it by setting the freepointer to
> NULL it fixes it, so we would care about the result. But maybe if the
> whole object_err() triggers, I think that causes an OOPs if I recall, we
> should consider this as corrupted and we shouldn't care about what it
> returns, cause something went terribly wrong?
> 
> I guess that sounds like a sound logic.
> 
> If that is how we should do this then yeah, my second suggested line is
> moot and in my later patch I shouldn't include it.

I think either way is fine, as both are not 100% accurate anyway if the
freelist is corrupted.
 
> > Two falses because when the freepointer of the tail object
> > is corrupted, the loop stops when nr equals slab->objects?
> 
> Yeah! Since the freelist pointer is corrupted and doesn't end in NULL
> on_freelist() won't find a NULL, and in the case of the tail having
> garbage that the check_valid_pointer() would normally catch, the code
> won't catch it, because it's at the tail which would exceed the
> slab->objects and the While loop will exit before that, since the
> freelist* (not slab, as you pointed out) is full.
> 
> > This is true because for partial slabs, the number of objects
> > plus 1 for the garbage, does not exceed slab->objects?
> 
> Yeah. That goes back to that second line I suggested in the previous
> email. If the number of objects in the freelist is less than the
> slab->objects and the corrupted freelist has some garbage address that
> the check_valid_pointer() will catch, it will set it to NULL instead, and
> as I said since the freelist now includes a NULL, my thinking was that
> on_freelist() should return true if we were searching for NULL.
> 
> But if thats not how on_freelist() should work in a case of a corrupted
> freelist and we don't add my second line that nulls the "fp" then this
> will return false instead.
> 
> > If you don't mind, could you please avoid bottom posting and
> > reply with inline comments like how I reply to you?
> > It makes it easier to follow the conversation.
> 
> Hopefully I did that correctly this time. Should I always include all
> the previous messages in the reply chain? It was getting rather long and
> I thought it would look messy.

You can omit some parts of the email if you're not replying to it.

Also, you don't have to copy and paste parts of the message when replying.
You can break the original message into into smaller sections and add
your comments in between—just like I do.

> > You can either send a new email or reply to this email with
> > In-Reply-To header. And please cc SLAB ALLOCATOR folks in MAINTAINERS file
> > including me—I recently changed my name and email (previously Hyeonggon Yoo).
> 
> Oh! You are one of the maintainers? That explains a lot, haha.

Not a maintainer (M:) but a reviewer (R:) :-)

> I just assumed you were someone from the mailing list who happened to be
> really passionate about SLUB.

Many people start by being curious and passionate about a specific
subsystem.

Anyway, as we discussed major concerns, (I think you can either add or not
add the second suggested line), could you please send a patch with a
nice commit message?

-- 
Cheers,
Harry