linux-kernel - Re: [PATCH v5 1/2] mm, kasan: improve double-free detection

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: <57617FE2.1050808@virtuozzo.com>
Date:	Wed, 15 Jun 2016 19:18:42 +0300
From:	Andrey Ryabinin <aryabinin@...tuozzo.com>
To:	Dmitry Vyukov <dvyukov@...gle.com>
CC:	Kuthonuzo Luruo <kuthonuzo.luruo@....com>,
	Alexander Potapenko <glider@...gle.com>,
	Christoph Lameter <cl@...ux.com>,
	Pekka Enberg <penberg@...nel.org>,
	David Rientjes <rientjes@...gle.com>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	kasan-dev <kasan-dev@...glegroups.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Yury Norov <ynorov@...iumnetworks.com>
Subject: Re: [PATCH v5 1/2] mm, kasan: improve double-free detection



On 06/10/2016 08:09 PM, Dmitry Vyukov wrote:
> On Fri, Jun 10, 2016 at 7:03 PM, Andrey Ryabinin
> <aryabinin@...tuozzo.com> wrote:
>>
>>
>> On 06/09/2016 08:00 PM, Andrey Ryabinin wrote:
>>> On 06/07/2016 09:03 PM, Kuthonuzo Luruo wrote:
>>>
>>> Next time, when/if you send patch series, send patches in one thread, i.e. patches should be replies to the cover letter.
>>> Your patches are not linked together, which makes them harder to track.
>>>
>>>
>>>> Currently, KASAN may fail to detect concurrent deallocations of the same
>>>> object due to a race in kasan_slab_free(). This patch makes double-free
>>>> detection more reliable by serializing access to KASAN object metadata.
>>>> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
>>>> lock/unlock per-object metadata. Double-free errors are now reported via
>>>> kasan_report().
>>>>
>>>> Per-object lock concept from suggestion/observations by Dmitry Vyukov.
>>>>
>>>
>>>
>>> So, I still don't like this, this too way hacky and complex.
>>> I have some thoughts about how to make this lockless and robust enough.
>>> I'll try to sort this out tomorrow.
>>>
>>
>>
>> So, I something like this should work.
>> Tested very briefly.
>>
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index ac4b3c4..8691142 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
>> @@ -75,6 +75,8 @@ struct kasan_cache {
>>  int kasan_module_alloc(void *addr, size_t size);
>>  void kasan_free_shadow(const struct vm_struct *vm);
>>
>> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object);
>> +
>>  size_t ksize(const void *);
>>  static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); }
>>
>> @@ -102,6 +104,9 @@ static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
>>  static inline void kasan_poison_object_data(struct kmem_cache *cache,
>>                                         void *object) {}
>>
>> +static inline void kasan_init_slab_obj(struct kmem_cache *cache,
>> +                               const void *object) { }
>> +
>>  static inline void kasan_kmalloc_large(void *ptr, size_t size, gfp_t flags) {}
>>  static inline void kasan_kfree_large(const void *ptr) {}
>>  static inline void kasan_poison_kfree(void *ptr) {}
>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>> index 6845f92..ab0fded 100644
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -388,11 +388,9 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
>>         *size += sizeof(struct kasan_alloc_meta);
>>
>>         /* Add free meta. */
>> -       if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor ||
>> -           cache->object_size < sizeof(struct kasan_free_meta)) {
>> -               cache->kasan_info.free_meta_offset = *size;
>> -               *size += sizeof(struct kasan_free_meta);
>> -       }
>> +       cache->kasan_info.free_meta_offset = *size;
>> +       *size += sizeof(struct kasan_free_meta);
>> +
> 
> 
> Why?!
> Please don't worsen runtime characteristics of KASAN. We run real
> systems with it.
> Most objects are small. This can lead to significant memory consumption.
> 

Yeah, this is a temp hack actually, because I didn't finish this part yet.
Basically, I want to make free stack always available (i.e. always save it in redzone),
because the is always better to have more information. Also this makes bug
report code a bit easier.

Of course, increasing memory usage is not what we want, so my plan is to make this:
 - remove alloc_size, because we already now object size. I mean cache->object_size.
      For kmalloc()'ed objects object_size is rounded up size, but exact size of allocation
       usually is not valuable information (Personally, I can't remember it being useful).

 - Unify allocation stack and free stack and keep them both in redzone. This is exactly 16-bytes, so this won't
    increase memory usage. So only quarantine pointer may be stored in freed object.

Proposed changes will actually decrease memory usage, because 8-byte objects will occupy less space.