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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aIcJdhoSTQlsdR5r@harry>
Date: Mon, 28 Jul 2025 14:24:06 +0900
From: Harry Yoo <harry.yoo@...cle.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: liqiong <liqiong@...china.com>, Vlastimil Babka <vbabka@...e.cz>,
        Christoph Lameter <cl@...two.org>,
        David Rientjes <rientjes@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] mm: slub: avoid deref of free pointer in sanity
 checks if object is invalid

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