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

[Dmitry Vyukov <dvyukov@google.com>]

On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
>
> On 05/06/2016 02:47 PM, Kuthonuzo Luruo wrote:
>> 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().
>>
>> Testing:
>> - Tested with a modified version of the 'slab_test' microbenchmark where
>>   allocs occur on CPU 0; then all other CPUs concurrently attempt to free
>>   the same object.
>> - Tested with new 'test_kasan' kasan_double_free() test in accompanying
>>   patch.
>>
>> Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
>> ---
>>
>> Changes in v2:
>> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>>   using fewer bits overall.
>> - Double-free pr_err promoted to kasan_report().
>> - kasan_init_object() introduced to initialize KASAN object metadata
>>   during slab creation. KASAN_STATE_INIT initialization removed from
>>   kasan_poison_object_data().
>>
>> ---
>>  include/linux/kasan.h |    8 +++
>>  mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++-------------
>>  mm/kasan/kasan.h      |   15 +++++-
>>  mm/kasan/quarantine.c |    7 +++-
>>  mm/kasan/report.c     |   31 +++++++++++--
>>  mm/slab.c             |    1 +
>>  6 files changed, 142 insertions(+), 38 deletions(-)
>>
>
> Sorry, but this patch is crap.
>
> Something like this, will fix the race:
>
> ---
>  mm/kasan/kasan.c      | 20 ++++----------------
>  mm/kasan/kasan.h      | 10 +++-------
>  mm/kasan/quarantine.c |  1 -
>  mm/kasan/report.c     | 11 ++---------
>  4 files changed, 9 insertions(+), 33 deletions(-)
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..8d078dc 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -419,13 +419,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
> @@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache, void *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;
> +               if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
> +                                       &alloc_info->state)) {
>                         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:
> +               } else {
>                         pr_err("Double free");
>                         dump_stack();
> -                       break;
> -               default:
> -                       break;
>                 }
>         }
>         return false;
> @@ -571,7 +559,7 @@ 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;
> +               set_bit(KASAN_STATE_ALLOCATED, &alloc_info->state);
>                 alloc_info->alloc_size = size;
>                 set_track(&alloc_info->track, flags);
>         }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..2dcdc8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -60,10 +60,7 @@ struct kasan_global {
>   */
>
>  enum kasan_state {
> -       KASAN_STATE_INIT,
> -       KASAN_STATE_ALLOC,
> -       KASAN_STATE_QUARANTINE,
> -       KASAN_STATE_FREE
> +       KASAN_STATE_ALLOCATED,
>  };
>
>  #define KASAN_STACK_DEPTH 64
> @@ -75,9 +72,8 @@ struct kasan_track {
>
>  struct kasan_alloc_meta {
>         struct kasan_track track;
> -       u32 state : 2;  /* enum kasan_state */
> -       u32 alloc_size : 30;
> -       u32 reserved;
> +       unsigned long state;
> +       u32 alloc_size;
>  };
>
>  struct kasan_free_meta {
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..ca33fd3 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache *cache)
>         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..c2b0e51 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -140,18 +140,12 @@ 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:
> +       if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
>                 pr_err("Object allocated with size %u bytes.\n",
>                        alloc_info->alloc_size);
>                 pr_err("Allocation:\n");
>                 print_track(&alloc_info->track);

alloc_info->track is not necessary initialized when
KASAN_STATE_ALLOCATED is set. Worse, it can be initialized to a wrong
stack.


> -               break;
> -       case KASAN_STATE_FREE:
> -       case KASAN_STATE_QUARANTINE:
> +       } else {
>                 pr_err("Object freed, allocated with size %u bytes\n",
>                        alloc_info->alloc_size);
>                 free_info = get_free_info(cache, object);
> @@ -159,7 +153,6 @@ static void object_err(struct kmem_cache *cache, struct page *page,
>                 print_track(&alloc_info->track);
>                 pr_err("Deallocation:\n");
>                 print_track(&free_info->track);
> -               break;
>         }
>  }
>  #endif
> --
> 2.7.3
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>