On Fri, Jun 21, 2019 at 3:37 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote: > > On Fri, Jun 07, 2019 at 08:42:27AM -0700, Kees Cook wrote: > > On Thu, Jun 06, 2019 at 06:48:43PM +0200, Alexander Potapenko wrote: > > > [...] > > > 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. > > > > > > - 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
