| 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
| ||
|
Message-ID: <577FDC2C.9030400@virtuozzo.com>
Date: Fri, 8 Jul 2016 20:00:28 +0300
From: Andrey Ryabinin <aryabinin@...tuozzo.com>
To: Alexander Potapenko <glider@...gle.com>, <adech.fo@...il.com>,
<cl@...ux.com>, <dvyukov@...gle.com>, <akpm@...ux-foundation.org>,
<rostedt@...dmis.org>, <iamjoonsoo.kim@....com>,
<js1304@...il.com>, <kcc@...gle.com>, <kuthonuzo.luruo@....com>
CC: <kasan-dev@...glegroups.com>, <linux-mm@...ck.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6] mm, kasan: switch SLUB to stackdepot, enable memory
quarantine for SLUB
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.
>
> 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.
> 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.
> +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();
> 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.
>
> 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().
> /*
> * 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.
> 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
>
Powered by blists - more mailing lists