On Fri, Jun 21, 2019 at 3:37 AM Kees Cook [off-list ref] wrote:

On Fri, Jun 07, 2019 at 08:42:27AM -0700, Kees Cook wrote:

quoted

On Thu, Jun 06, 2019 at 06:48:43PM +0200, Alexander Potapenko wrote:

quoted

[...]

diff --git a/mm/slub.c b/mm/slub.c
index cd04dbd2b5d0..9c4a8b9a955c 100644
--- a/mm/slub.c
+++ b/mm/slub.c

[...]

@@ -2741,8 +2758,14 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
            prefetch_freepointer(s, next_object);
            stat(s, ALLOC_FASTPATH);
    }
+   /*
+    * If the object has been wiped upon free, make sure it's fully
+    * initialized by zeroing out freelist pointer.
+    */
+   if (unlikely(slab_want_init_on_free(s)) && object)
+           *(void **)object = NULL;

In looking at metadata again, I noticed that I don't think this is
correct, as it needs to be using s->offset to find the location of the
freelist pointer:

        memset(object + s->offset, 0, sizeof(void *));

In the cases we support s->offset is always zero (we don't initialize
slabs with ctors or RCU), but using its value is a sane
generalization.

quoted

quoted

-   if (unlikely(gfpflags & __GFP_ZERO) && object)
+   if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
            memset(object, 0, s->object_size);

init_on_alloc is using "object_size" but init_on_free is using "size". I
assume the "alloc" wipe is smaller because metadata was just written
for the allocation?

As noted in another thread, using "size" is incorrect, because it may
overwrite the redzone after the object.
I'll send a patch to fix that.
Overwriting the metadata indeed shouldn't make sense in the allocation case.

--
Kees Cook

-- 
Alexander Potapenko
Software Engineer

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

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg