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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <573085EB.5060808@virtuozzo.com>
Date:	Mon, 9 May 2016 15:43:23 +0300
From:	Andrey Ryabinin <aryabinin@...tuozzo.com>
To:	Dmitry Vyukov <dvyukov@...gle.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>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

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