linux-kernel - Re: [PATCH] slub: make sure struct kmem_cache

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]

Date:   Wed, 12 Jul 2017 16:11:28 +0200
From:   Alexander Potapenko <glider@...gle.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Christoph Lameter <cl@...ux.com>,
        Dmitriy Vyukov <dvyukov@...gle.com>,
        Kostya Serebryany <kcc@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH] slub: make sure struct kmem_cache_node is initialized
 before publication

Hi everyone,

On Mon, Jul 10, 2017 at 10:32 PM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Fri, 7 Jul 2017 18:18:31 -0500 (CDT) Christoph Lameter <cl@...ux.com> wrote:
>
>> On Fri, 7 Jul 2017, Andrew Morton wrote:
>>
>> > On Fri,  7 Jul 2017 10:34:08 +0200 Alexander Potapenko <glider@...gle.com> wrote:
>> >
>> > > --- a/mm/slub.c
>> > > +++ b/mm/slub.c
>> > > @@ -3389,8 +3389,8 @@ static int init_kmem_cache_nodes(struct kmem_cache *s)
>> > >                   return 0;
>> > >           }
>> > >
>> > > -         s->node[node] = n;
>> > >           init_kmem_cache_node(n);
>> > > +         s->node[node] = n;
>> > >   }
>> > >   return 1;
>> > >  }
>> >
>> > If this matters then I have bad feelings about free_kmem_cache_nodes():
>>
>> At creation time the kmem_cache structure is private and no one can run a
>> free operation.
I've double-checked the code path and this turned out to be a false
positive caused by KMSAN not instrumenting the contents of mm/slub.c
(i.e. the initialization of the spinlock remained unnoticed).
Christoph is indeed right that kmem_cache_structure is private, so a
race is not possible here.
I am sorry for the false alarm.
>> > Inviting a use-after-free?  I guess not, as there should be no way
>> > to look up these items at this stage.
>>
>> Right.
>
> Still.   It looks bad, and other sites do these things in the other order.
If the maintainers agree the initialization order needs to be fixed,
we'll need to remove the (irrelevant) KMSAN report from the patch
description.
>> > Could the slab maintainers please take a look at these and also have a
>> > think about Alexander's READ_ONCE/WRITE_ONCE question?
>>
>> Was I cced on these?
>
> It's all on linux-mm.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg