| 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
| ||
|
Date: Tue, 12 Jul 2016 12:10:52 +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 v6] mm, kasan: switch SLUB to stackdepot, enable memory
quarantine for SLUB
On Fri, Jul 8, 2016 at 7:00 PM, Andrey Ryabinin <aryabinin@...tuozzo.com> wrote:
>
>
> On 07/08/2016 01:36 PM, Alexander Potapenko wrote:
>>
>> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
>> index d1faa01..07e4549 100644
>> --- a/include/linux/slub_def.h
>> +++ b/include/linux/slub_def.h
>> @@ -99,6 +99,10 @@ struct kmem_cache {
>> */
>> int remote_node_defrag_ratio;
>> #endif
>> +#ifdef CONFIG_KASAN
>> + struct kasan_cache kasan_info;
>> +#endif
>> +
>> struct kmem_cache_node *node[MAX_NUMNODES];
>> };
>>
>> @@ -119,10 +123,11 @@ static inline void *nearest_obj(struct kmem_cache *cache, struct page *page,
>> void *object = x - (x - page_address(page)) % cache->size;
>> void *last_object = page_address(page) +
>> (page->objects - 1) * cache->size;
>> - if (unlikely(object > last_object))
>> - return last_object;
>> - else
>> - return object;
>> + void *result = (unlikely(object > last_object)) ? last_object : object;
>> +
>> + if (cache->flags & SLAB_RED_ZONE)
>> + return ((char *)result + cache->red_left_pad);
>> + return result;
>
> This fixes existing problem. It should be a separate patch.
Done.
>
>>
>> void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>> @@ -568,6 +572,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;
>> +
>
> I still think that this hunk should be removed.
Got it. Indeed, you're right, we're checking SLAB_KASAN twice in this function.
>> redzone_start = round_up((unsigned long)(object + size),
>> KASAN_SHADOW_SCALE_SIZE);
>> redzone_end = round_up((unsigned long)object + cache->object_size,
>> @@ -576,7 +583,6 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>> kasan_unpoison_shadow(object, size);
>> kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start,
>> KASAN_KMALLOC_REDZONE);
>> -#ifdef CONFIG_SLAB
>> if (cache->flags & SLAB_KASAN) {
>> struct kasan_alloc_meta *alloc_info =
>> get_alloc_info(cache, object);
>> @@ -585,7 +591,6 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>> alloc_info->alloc_size = size;
>> set_track(&alloc_info->track, flags);
>> }
>> -#endif
>> }
>> EXPORT_SYMBOL(kasan_kmalloc);
>>
>
>
> ...
>
>
>> diff --git a/mm/slab.h b/mm/slab.h
>> index dedb1a9..9a09d06 100644
>> --- a/mm/slab.h
>> +++ b/mm/slab.h
>> @@ -366,6 +366,8 @@ static inline size_t slab_ksize(const struct kmem_cache *s)
>> if (s->flags & (SLAB_RED_ZONE | SLAB_POISON))
>> return s->object_size;
>> # endif
>> + if (s->flags & SLAB_KASAN)
>> + return s->object_size;
>> /*
>> * If we have the need to store the freelist pointer
>> * back there or track user information then we can
>> @@ -462,6 +464,7 @@ void *slab_next(struct seq_file *m, void *p, loff_t *pos);
>> void slab_stop(struct seq_file *m, void *p);
>> int memcg_slab_show(struct seq_file *m, void *p);
>>
>> -void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr);
>> +void *nearest_obj(struct kmem_cache *cache, struct page *page, void *x);
>
> Remove.
Done.
>> +void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr);
>> #endif /* MM_SLAB_H */
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 825ff45..72ecffa 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -454,8 +454,6 @@ 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;
>> #endif
>> @@ -783,6 +781,14 @@ static int check_pad_bytes(struct kmem_cache *s, struct page *page, u8 *p)
>> /* Freepointer is placed after the object. */
>> off += sizeof(void *);
>>
>> +#ifdef CONFIG_KASAN
>> + if (s->kasan_info.alloc_meta_offset)
>> + off += sizeof(struct kasan_alloc_meta);
>> +
>> + if (s->kasan_info.free_meta_offset)
>> + off += sizeof(struct kasan_free_meta);
>> +#endif
>
> That's ugly.
>
> Following is not ugly:
> off += kasan_metadata_size();
Fixed. Also moved kasan_{alloc,free}_meta declarations back.
>
>> if (s->flags & SLAB_STORE_USER)
>> /* We also have user information there */
>> off += 2 * sizeof(struct track);
>> @@ -1322,7 +1328,7 @@ static inline void kfree_hook(const void *x)
>> kasan_kfree_large(x);
>> }
>>
>> -static inline void slab_free_hook(struct kmem_cache *s, void *x)
>> +static inline bool slab_free_hook(struct kmem_cache *s, void *x)
>> {
>> kmemleak_free_recursive(x, s->flags);
>>
>> @@ -1344,7 +1350,7 @@ static inline void slab_free_hook(struct kmem_cache *s, void *x)
>> if (!(s->flags & SLAB_DEBUG_OBJECTS))
>> debug_check_no_obj_freed(x, s->object_size);
>>
>> - kasan_slab_free(s, x);
>> + return kasan_slab_free(s, x);
>> }
>
> Change it back, return value is not unused anymore.
Done.
>
>>
>> static inline void slab_free_freelist_hook(struct kmem_cache *s,
>> @@ -2753,6 +2759,9 @@ slab_empty:
>> discard_slab(s, page);
>> }
>>
>> +static void do_slab_free(struct kmem_cache *s, struct page *page,
>> + void *head, void *tail, int cnt, unsigned long addr);
>> +
>
> You can just place slab_free() after do_slab_free().
Done. I've retained the comment below, but it's now attributed to
do_slab_free().
>> /*
>> * Fastpath with forced inlining to produce a kfree and kmem_cache_free that
>> * can perform fastpath freeing without additional function calls.
>> @@ -2772,12 +2781,23 @@ static __always_inline void slab_free(struct kmem_cache *s, struct page *page,
>> void *head, void *tail, int cnt,
>> unsigned long addr)
>> {
>> + slab_free_freelist_hook(s, head, tail);
>> + /*
>> + * slab_free_freelist_hook() could have put the items into quarantine.
>> + * If so, no need to free them.
>> + */
>> + if (s->flags & SLAB_KASAN && !(s->flags & SLAB_DESTROY_BY_RCU))
>> + return;
>> + do_slab_free(s, page, head, tail, cnt, addr);
>> +}
>> +
>> +static __always_inline void do_slab_free(struct kmem_cache *s,
>> + struct page *page, void *head, void *tail,
>> + int cnt, unsigned long addr)
>> +{
>> void *tail_obj = tail ? : head;
>> struct kmem_cache_cpu *c;
>> unsigned long tid;
>> -
>> - slab_free_freelist_hook(s, head, tail);
>> -
>> redo:
>> /*
>> * Determine the currently cpus per cpu slab.
>> @@ -2811,6 +2831,11 @@ redo:
>>
>> }
>>
>> +void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
>> +{
>> + do_slab_free(cache, virt_to_head_page(x), x, NULL, 1, addr);
>> +}
>> +
>
> Probably would be better to hide this under #ifdef CONFIG_KASAN. It has no other users, and it might be relatively
> large function because do_slab_free() is always inlined.
Done.
>
>> void kmem_cache_free(struct kmem_cache *s, void *x)
>> {
>> s = cache_from_obj(s, x);
>> @@ -3252,7 +3277,7 @@ static void set_min_partial(struct kmem_cache *s, unsigned long min)
>> static int calculate_sizes(struct kmem_cache *s, int forced_order)
>> {
>> unsigned long flags = s->flags;
>> - unsigned long size = s->object_size;
>> + size_t size = s->object_size;
>> int order;
>>
>> /*
>> @@ -3311,7 +3336,10 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
>> * the object.
>> */
>> size += 2 * sizeof(struct track);
>> +#endif
>>
>> + kasan_cache_create(s, &size, &s->flags);
>> +#ifdef CONFIG_SLUB_DEBUG
>> if (flags & SLAB_RED_ZONE) {
>> /*
>> * Add some empty padding so that we can catch
>>
--
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
Powered by blists - more mailing lists