Re: [PATCH 6/6] kasan: improve double-free reports.

[Alexander Potapenko]

On Mon, Aug 1, 2016 at 4:45 PM, Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx> wrote:
> Currently we just dump stack in case of double free bug.
> Let's dump all info about the object that we have.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx>
> ---
>  mm/kasan/kasan.c  |  3 +--
>  mm/kasan/kasan.h  |  2 ++
>  mm/kasan/report.c | 54 ++++++++++++++++++++++++++++++++++++++----------------
>  3 files changed, 41 insertions(+), 18 deletions(-)
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 92750e3..88af13c 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -543,8 +543,7 @@ bool kasan_slab_free(struct kmem_cache *cache, void *object)
>
>         shadow_byte = READ_ONCE(*(s8 *)kasan_mem_to_shadow(object));
>         if (shadow_byte < 0 || shadow_byte >= KASAN_SHADOW_SCALE_SIZE) {
> -               pr_err("Double free");
> -               dump_stack();
> +               kasan_report_double_free(cache, object, shadow_byte);
>                 return true;
>         }
>
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 9b7b31e..e5c2181 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -99,6 +99,8 @@ 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,
> +                       s8 shadow);
>
>  #if defined(CONFIG_SLAB) || defined(CONFIG_SLUB)
>  void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index f437398..ee2bdb4 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -116,6 +116,26 @@ static inline bool init_task_stack_addr(const void *addr)
>                         sizeof(init_thread_union.stack));
>  }
>
> +static DEFINE_SPINLOCK(report_lock);
> +
> +static void kasan_start_report(unsigned long *flags)
> +{
> +       /*
> +        * Make sure we don't end up in loop.
> +        */
> +       kasan_disable_current();
> +       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);
Don't we want to add the taint as early as possible once we've
detected the error?
> +       spin_unlock_irqrestore(&report_lock, *flags);
> +       kasan_enable_current();
> +}
> +
>  static void print_track(struct kasan_track *track)
>  {
>         pr_err("PID = %u\n", track->pid);
> @@ -129,8 +149,7 @@ static void print_track(struct kasan_track *track)
>         }
>  }
>
> -static void kasan_object_err(struct kmem_cache *cache, struct page *page,
> -                               void *object, char *unused_reason)
> +static void kasan_object_err(struct kmem_cache *cache, void *object)
>  {
>         struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
>
> @@ -147,6 +166,18 @@ static void kasan_object_err(struct kmem_cache *cache, struct page *page,
>         print_track(&alloc_info->free_track);
>  }
>
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> +                       s8 shadow)
> +{
> +       unsigned long flags;
> +
> +       kasan_start_report(&flags);
> +       pr_err("BUG: Double free or corrupt pointer\n");
How about "Double free or freeing an invalid pointer\n"?
I think "corrupt pointer" doesn't exactly reflect where the bug is.
> +       pr_err("Unexpected shadow byte: 0x%hhX\n", shadow);
> +       kasan_object_err(cache, object);
> +       kasan_end_report(&flags);
> +}
> +
>  static void print_address_description(struct kasan_access_info *info)
>  {
>         const void *addr = info->access_addr;
> @@ -160,8 +191,7 @@ static void print_address_description(struct kasan_access_info *info)
>                         struct kmem_cache *cache = page->slab_cache;
>                         object = nearest_obj(cache, page,
>                                                 (void *)info->access_addr);
> -                       kasan_object_err(cache, page, object,
> -                                       "kasan: bad access detected");
> +                       kasan_object_err(cache, object);
>                         return;
>                 }
>                 dump_page(page, "kasan: bad access detected");
> @@ -226,19 +256,13 @@ static void print_shadow_for_address(const void *addr)
>         }
>  }
>
> -static DEFINE_SPINLOCK(report_lock);
> -
>  static void kasan_report_error(struct kasan_access_info *info)
>  {
>         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);
> -       pr_err("==================================================================\n");
> +       kasan_start_report(&flags);
> +
>         if (info->access_addr <
>                         kasan_shadow_to_mem((void *)KASAN_SHADOW_START)) {
>                 if ((unsigned long)info->access_addr < PAGE_SIZE)
> @@ -259,10 +283,8 @@ 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(unsigned long addr, size_t size,
> --
> 2.7.3
>



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