| 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: <CACT4Y+YoFuH5DUyOHbs80TkgNd_5uNJZuuzkaVq5JaLqF318Pg@mail.gmail.com>
Date: Fri, 10 Jun 2016 19:09:29 +0200
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Andrey Ryabinin <aryabinin@...tuozzo.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 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.
> redzone_adjust = optimal_redzone(cache->object_size) -
> (*size - cache->object_size);
> if (redzone_adjust > 0)
> @@ -431,13 +429,6 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
> kasan_poison_shadow(object,
> round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
> KASAN_KMALLOC_REDZONE);
> -#ifdef CONFIG_SLAB
> - if (cache->flags & SLAB_KASAN) {
> - struct kasan_alloc_meta *alloc_info =
> - get_alloc_info(cache, object);
> - alloc_info->state = KASAN_STATE_INIT;
> - }
> -#endif
> }
>
> #ifdef CONFIG_SLAB
> @@ -501,6 +492,20 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
> BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
> return (void *)object + cache->kasan_info.free_meta_offset;
> }
> +
> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object)
> +{
> + struct kasan_alloc_meta *alloc_info;
> + struct kasan_free_meta *free_info;
> +
> + if (!(cache->flags & SLAB_KASAN))
> + return;
> +
> + alloc_info = get_alloc_info(cache, object);
> + free_info = get_free_info(cache, object);
> + __memset(alloc_info, 0, sizeof(*alloc_info));
> + __memset(free_info, 0, sizeof(*free_info));
> +}
> #endif
>
> void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
> @@ -523,37 +528,47 @@ static void kasan_poison_slab_free(struct kmem_cache *cache, void *object)
> bool kasan_slab_free(struct kmem_cache *cache, void *object)
> {
> #ifdef CONFIG_SLAB
> + struct kasan_free_meta *free_info = get_free_info(cache, object);
> + struct kasan_track new_free_stack, old_free_stack;
> + s8 old_shadow;
> +
> /* RCU slabs could be legally used after free within the RCU period */
> if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
> return false;
>
> - if (likely(cache->flags & SLAB_KASAN)) {
> - struct kasan_alloc_meta *alloc_info =
> - get_alloc_info(cache, object);
> - struct kasan_free_meta *free_info =
> - get_free_info(cache, object);
> -
> - switch (alloc_info->state) {
> - case KASAN_STATE_ALLOC:
> - alloc_info->state = KASAN_STATE_QUARANTINE;
> - quarantine_put(free_info, cache);
> - set_track(&free_info->track, GFP_NOWAIT);
> - kasan_poison_slab_free(cache, object);
> - return true;
> - case KASAN_STATE_QUARANTINE:
> - case KASAN_STATE_FREE:
> - pr_err("Double free");
> - dump_stack();
> - break;
> - default:
> - break;
> - }
> + if (unlikely(!(cache->flags & SLAB_KASAN)))
> + return false;
> +
> + set_track(&new_free_stack, GFP_NOWAIT);
> + old_free_stack = xchg(&free_info->track, new_free_stack);
> + old_shadow = xchg((s8 *)kasan_mem_to_shadow(object),
> + KASAN_KMALLOC_FREE);
> +
> + if (old_shadow < 0 || old_shadow >= KASAN_SHADOW_SCALE_SIZE) {
> + struct kasan_track free_stack;
> +
> + /* Paired with xchg() above */
> + free_stack = smp_load_acquire(&free_info->track);
> +
> + /*
> + * We didn't raced with another instance of kasan_slab_free()
> + * so the previous free stack supposed to be in old_free_stack.
> + * Otherwise, free_stack will contain stack trace of another
> + * kfree() call.
> + */
> + if (free_stack.id == new_free_stack.id)
> + free_stack = old_free_stack;
> +
> + kasan_report_double_free(cache, object,
> + free_stack, old_shadow);
> + return false;
> }
> - return false;
> -#else
> kasan_poison_slab_free(cache, object);
> - return false;
> + return true;
> +
> #endif
> + kasan_poison_slab_free(cache, object);
> + return false;
> }
>
> void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
> @@ -581,7 +596,6 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
> struct kasan_alloc_meta *alloc_info =
> get_alloc_info(cache, object);
>
> - alloc_info->state = KASAN_STATE_ALLOC;
> alloc_info->alloc_size = size;
> set_track(&alloc_info->track, flags);
> }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index fb87923..9b46d2e 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -59,24 +59,21 @@ struct kasan_global {
> * Structures to keep alloc and free tracks *
> */
>
> -enum kasan_state {
> - KASAN_STATE_INIT,
> - KASAN_STATE_ALLOC,
> - KASAN_STATE_QUARANTINE,
> - KASAN_STATE_FREE
> -};
> -
> #define KASAN_STACK_DEPTH 64
>
> struct kasan_track {
> - u32 pid;
> - depot_stack_handle_t stack;
> +union {
> + struct {
> + u32 pid;
> + depot_stack_handle_t stack;
> + };
> + u64 id;
> +};
> };
>
> struct kasan_alloc_meta {
> struct kasan_track track;
> - u32 state : 2; /* enum kasan_state */
> - u32 alloc_size : 30;
> + u32 alloc_size;
> };
>
> struct qlist_node {
> @@ -109,6 +106,9 @@ static inline bool kasan_report_enabled(void)
>
> void kasan_report(unsigned long addr, size_t size,
> bool is_write, unsigned long ip);
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> + struct kasan_track free_stack, s8 shadow);
> +
>
> #ifdef CONFIG_SLAB
> void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 4973505..3ec039c 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -144,11 +144,9 @@ static void *qlink_to_object(struct qlist_node *qlink, struct kmem_cache *cache)
> static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
> {
> void *object = qlink_to_object(qlink, cache);
> - struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
> unsigned long flags;
>
> local_irq_save(flags);
> - alloc_info->state = KASAN_STATE_FREE;
> ___cache_free(cache, object, _THIS_IP_);
> local_irq_restore(flags);
> }
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index b3c122d..a0f4519 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -140,28 +140,13 @@ static void object_err(struct kmem_cache *cache, struct page *page,
> pr_err("Object at %p, in cache %s\n", object, cache->name);
> if (!(cache->flags & SLAB_KASAN))
> return;
> - switch (alloc_info->state) {
> - case KASAN_STATE_INIT:
> - pr_err("Object not allocated yet\n");
> - break;
> - case KASAN_STATE_ALLOC:
> - pr_err("Object allocated with size %u bytes.\n",
> - alloc_info->alloc_size);
> - pr_err("Allocation:\n");
> - print_track(&alloc_info->track);
> - break;
> - case KASAN_STATE_FREE:
> - case KASAN_STATE_QUARANTINE:
> - pr_err("Object freed, allocated with size %u bytes\n",
> - alloc_info->alloc_size);
> - free_info = get_free_info(cache, object);
> - pr_err("Allocation:\n");
> - print_track(&alloc_info->track);
> - pr_err("Deallocation:\n");
> - print_track(&free_info->track);
> - break;
> - }
> + free_info = get_free_info(cache, object);
> + pr_err("Allocation:\n");
> + print_track(&alloc_info->track);
> + pr_err("Deallocation:\n");
> + print_track(&free_info->track);
> }
> +
> #endif
>
> static void print_address_description(struct kasan_access_info *info)
> @@ -245,17 +230,31 @@ static void print_shadow_for_address(const void *addr)
>
> static DEFINE_SPINLOCK(report_lock);
>
> -static void kasan_report_error(struct kasan_access_info *info)
> +static void kasan_start_report(unsigned long *flags)
> {
> - unsigned long flags;
> - const char *bug_type;
> -
> /*
> * Make sure we don't end up in loop.
> */
> kasan_disable_current();
> - spin_lock_irqsave(&report_lock, flags);
> + spin_lock_irqsave(&report_lock, *flags);
> pr_err("==================================================================\n");
> +}
> +
> +static void kasan_end_report(unsigned long *flags)
> +{
> + pr_err("==================================================================\n");
> + add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> + spin_unlock_irqrestore(&report_lock, *flags);
> + kasan_enable_current();
> +}
> +
> +static void kasan_report_error(struct kasan_access_info *info)
> +{
> + unsigned long flags;
> + const char *bug_type;
> +
> + kasan_start_report(&flags);
> +
> if (info->access_addr <
> kasan_shadow_to_mem((void *)KASAN_SHADOW_START)) {
> if ((unsigned long)info->access_addr < PAGE_SIZE)
> @@ -276,10 +275,29 @@ static void kasan_report_error(struct kasan_access_info *info)
> print_address_description(info);
> print_shadow_for_address(info->first_bad_addr);
> }
> - pr_err("==================================================================\n");
> - add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> - spin_unlock_irqrestore(&report_lock, flags);
> - kasan_enable_current();
> +
> + kasan_end_report(&flags);
> +}
> +
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> + struct kasan_track free_stack, s8 shadow)
> +{
> + unsigned long flags;
> +
> + kasan_start_report(&flags);
> +
> + pr_err("BUG: Double free or corrupt pointer\n");
> + pr_err("Unexpected shadow byte: 0x%hhX\n", shadow);
> +
> + dump_stack();
> + pr_err("Object at %p, in cache %s\n", object, cache->name);
> + get_alloc_info(cache, object);
> + pr_err("Allocation:\n");
> + print_track(&get_alloc_info(cache, object)->track);
> + pr_err("Deallocation:\n");
> + print_track(&free_stack);
> +
> + kasan_end_report(&flags);
> }
>
> void kasan_report(unsigned long addr, size_t size,
> diff --git a/mm/slab.c b/mm/slab.c
> index 763096a..65c942b 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2604,9 +2604,11 @@ static void cache_init_objs(struct kmem_cache *cachep,
> }
>
> for (i = 0; i < cachep->num; i++) {
> + objp = index_to_obj(cachep, page, i);
> + kasan_init_slab_obj(cachep, objp);
> +
> /* constructor could break poison info */
> if (DEBUG == 0 && cachep->ctor) {
> - objp = index_to_obj(cachep, page, i);
> kasan_unpoison_object_data(cachep, objp);
> cachep->ctor(objp);
> kasan_poison_object_data(cachep, objp);
Powered by blists - more mailing lists