linux-kernel - Re: [PATCH v3] 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]

Message-ID: <CAG_fn=X8szV17tk+TBGXbKy881aNBeA=7F48_wD62LHYhjpvnw@mail.gmail.com>
Date:	Fri, 17 Jun 2016 16:27:40 +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 v3] mm, kasan: switch SLUB to stackdepot, enable memory
 quarantine for SLUB

On Wed, Jun 15, 2016 at 6:50 PM, Andrey Ryabinin
<aryabinin@...tuozzo.com> wrote:
>
>
> On 06/15/2016 06:26 PM, Alexander Potapenko wrote:
>> For KASAN builds:
>>  - switch SLUB allocator to using stackdepot instead of storing the
>>    allocation/deallocation stacks in the objects;
>>  - define SLAB_RED_ZONE, SLAB_POISON, SLAB_STORE_USER to zero,
>>    effectively disabling these debug features, as they're redundant in
>>    the presence of KASAN;
>
> So, why we forbid these? If user wants to set these, why not? If you don't want it, just don't turn them on, that's it.
SLAB_RED_ZONE simply doesn't work with KASAN.
With additional efforts it may work, but I don't think we really need
that. Extra red zones will just bloat the heap, and won't give any
interesting signal except "someone corrupted this object from
non-instrumented code".
SLAB_POISON doesn't crash on simple tests, but I am not sure there are
no corner cases which I haven't checked, so I thought it's safer to
disable it.
As I said before, we can make SLAB_STORE_USER use stackdepot in a
later CL, thus we disable it now.

> And sometimes POISON/REDZONE might be actually useful. KASAN doesn't catch everything,
> e.g. corruption may happen in assembly code, or DMA by  some device.
>
>
>>  - change the freelist hook so that parts of the freelist can be put into
>>    the quarantine.
>>
>> Signed-off-by: Alexander Potapenko <glider@...gle.com>
>> ---
>
> ...
>
>> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
>> index fb87923..8c75953 100644
>> --- a/mm/kasan/kasan.h
>> +++ b/mm/kasan/kasan.h
>> @@ -110,7 +110,7 @@ static inline bool kasan_report_enabled(void)
>>  void kasan_report(unsigned long addr, size_t size,
>>               bool is_write, unsigned long ip);
>>
>> -#ifdef CONFIG_SLAB
>> +#if defined(CONFIG_SLAB) || defined(CONFIG_SLUB)
>>  void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);
>>  void quarantine_reduce(void);
>>  void quarantine_remove_cache(struct kmem_cache *cache);
>> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
>> index 4973505..89259c2 100644
>> --- a/mm/kasan/quarantine.c
>> +++ b/mm/kasan/quarantine.c
>> @@ -149,7 +149,12 @@ static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
>>
>>       local_irq_save(flags);
>>       alloc_info->state = KASAN_STATE_FREE;
>> +#ifdef CONFIG_SLAB
>>       ___cache_free(cache, object, _THIS_IP_);
>> +#elif defined(CONFIG_SLUB)
>> +     do_slab_free(cache, virt_to_head_page(object), object, NULL, 1,
>> +             _RET_IP_);
>> +#endif
>
> Please, add some simple wrapper instead of this.
>
>>       local_irq_restore(flags);
>>  }
>>
Done. I've reused ___cache_free().
>
> ...
>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 825ff45..f023dd4 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -191,7 +191,11 @@ static inline bool kmem_cache_has_cpu_partial(struct kmem_cache *s)
>>  #define MAX_OBJS_PER_PAGE    32767 /* since page.objects is u15 */
>>
>>  /* Internal SLUB flags */
>> +#ifndef CONFIG_KASAN
>>  #define __OBJECT_POISON              0x80000000UL /* Poison object */
>> +#else
>> +#define __OBJECT_POISON              0x00000000UL /* Disable object poisoning */
>> +#endif
>>  #define __CMPXCHG_DOUBLE     0x40000000UL /* Use cmpxchg_double */
>>
>>  #ifdef CONFIG_SMP
>> @@ -454,10 +458,8 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p)
>>   */
>>  #if defined(CONFIG_SLUB_DEBUG_ON)
>>  static int slub_debug = DEBUG_DEFAULT_FLAGS;
>> -#elif defined(CONFIG_KASAN)
>> -static int slub_debug = SLAB_STORE_USER;
>>  #else
>> -static int slub_debug;
>> +static int slub_debug = SLAB_STORE_USER;
>
> Huh! So now it is on!? By default, and for everyone!
>
Good catch, thanks!


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