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

Date:	Fri, 8 Jul 2016 18:31:00 +0300
From:	Andrey Ryabinin <aryabinin@...tuozzo.com>
To:	Alexander Potapenko <glider@...gle.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 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? 



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