linux-kernel - Re: [PATCH v5] mm, kasan: switch SLUB to stackdepot, enable memory quarantine for SLUB

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]

Date:	Tue, 12 Jul 2016 12:17:29 +0200
From:	Alexander Potapenko <glider@...gle.com>
To:	Andrey Ryabinin <aryabinin@...tuozzo.com>
Cc:	Andrey Konovalov <adech.fo@...il.com>,
	Christoph Lameter <cl@...ux.com>,
	Dmitriy Vyukov <dvyukov@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Joonsoo Kim <js1304@...il.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Kuthonuzo Luruo <kuthonuzo.luruo@....com>,
	kasan-dev <kasan-dev@...glegroups.com>,
	Linux Memory Management List <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] mm, kasan: switch SLUB to stackdepot, enable memory
 quarantine for SLUB

On Fri, Jul 8, 2016 at 5:31 PM, Andrey Ryabinin <aryabinin@...tuozzo.com> wrote:
>
>
> On 07/08/2016 01:36 PM, Alexander Potapenko wrote:
>> On Tue, Jun 28, 2016 at 6:51 PM, Andrey Ryabinin
>> <aryabinin@...tuozzo.com> wrote:
>
>>>>       *flags |= SLAB_KASAN;
>>>> +
>>>>       /* Add alloc meta. */
>>>>       cache->kasan_info.alloc_meta_offset = *size;
>>>>       *size += sizeof(struct kasan_alloc_meta);
>>>> @@ -392,17 +387,35 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
>>>>           cache->object_size < sizeof(struct kasan_free_meta)) {
>>>>               cache->kasan_info.free_meta_offset = *size;
>>>>               *size += sizeof(struct kasan_free_meta);
>>>> +     } else {
>>>> +             cache->kasan_info.free_meta_offset = 0;
>>>
>>> Why is that required now?
>> Because we want to store the free metadata in the object when it's possible.
>
> We did the before this patch. free_meta_offset is 0 by default, thus there was no need to nullify it here.
> But now this patch suddenly adds reset of free_meta_offset. So I'm asking why?
> Is free_meta_offset not 0 by default anymore?
Yes, since the new cache is created using zalloc() (which I didn't
know before) I'd better remove this assignment.
>
>
>>>>
>>>>  void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>>>> @@ -568,6 +573,9 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>>>>       if (unlikely(object == NULL))
>>>>               return;
>>>>
>>>> +     if (!(cache->flags & SLAB_KASAN))
>>>> +             return;
>>>> +
>>>
>>> This hunk is superfluous and wrong.
>> Can you please elaborate?
>> Do you mean we don't need to check for SLAB_KASAN here, or that we
>> don't need SLAB_KASAN at all?
>
> The former, we can poison/unpoison !SLAB_KASAN caches too.
>
>
>
>>>>  }
>>>>
>>>> @@ -2772,12 +2788,22 @@ static __always_inline void slab_free(struct kmem_cache *s, struct page *page,
>>>>                                     void *head, void *tail, int cnt,
>>>>                                     unsigned long addr)
>>>>  {
>>>> +     void *free_head = head, *free_tail = tail;
>>>> +
>>>> +     slab_free_freelist_hook(s, &free_head, &free_tail, &cnt);
>>>> +     /* slab_free_freelist_hook() could have emptied the freelist. */
>>>> +     if (cnt == 0)
>>>> +             return;
>>>
>>> I suppose that we can do something like following, instead of that mess in slab_free_freelist_hook() above
>>>
>>>         slab_free_freelist_hook(s, &free_head, &free_tail);
>>>         if (s->flags & SLAB_KASAN && s->flags & SLAB_DESTROY_BY_RCU)
>> Did you mean "&& !(s->flags & SLAB_DESTROY_BY_RCU)" ?
>
> Sure.
>
>>>                 return;
>> Yes, my code is overly complicated given that kasan_slab_free() should
>> actually return the same value for every element of the list.
>> (do you think it makes sense to check that?)
>
> IMO that's would be superfluous.
>
>> I can safely remove those freelist manipulations.
>>>
>>>



-- 
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