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

[Andrey Ryabinin]

On 05/09/2016 01:31 PM, Dmitry Vyukov wrote:
> On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
> <aryabinin@xxxxxxxxxxxxx> wrote:
>>
>> 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.

It should be initialized to something. If it's not initialized, than that object wasn't allocated.
So this would be a *very* random pointer access. Also this would mean that ->state itself might be not initialized too.
Anyway, we can't do much in such scenario since we can't trust any data.
And I don't think that we should because very likely this will cause panic eventually.

> Worse, it can be initialized to a wrong
> stack.
> 

Define "wrong stack" here.

I assume that you are talking about race in the following scenario:

------
Proccess A:                      Proccess B:

p_A = kmalloc();
/* use p_A */
kfree(p_A);
                                 p_A = kmalloc();
                                      ....
                                      set_bit(KASAN_STATE_ALLOCATED); //bit set, but stack is not saved yet.
/* use after free p_A */

if (test_bit(KASAN_STATE_ALLOCATED))
        print_stack() // will print track from Proccess A
-----

So, would be the stack trace from A wrong in such situation? I don't think so.
We could change ->state and save stack trace in the 'right' order with proper barriers,
but would it prevent us from showing wrong stacktrace? - It wouldn't.

Now, the only real problem with current code, is that we don't print free stack if we think that object is in
allocated state. We should do this, because more information is always better, e.g. we might hit long-delayed use-after-free,
in which case free stack would be useful (just like in scenario above).