| 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
| ||
|
Date: Tue, 2 Aug 2016 14:05:25 +0200
From: Alexander Potapenko <glider@...gle.com>
To: Andrey Ryabinin <aryabinin@...tuozzo.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Dave Jones <davej@...emonkey.org.uk>,
Vegard Nossum <vegard.nossum@...cle.com>,
Sasha Levin <alexander.levin@...izon.com>,
Dmitry Vyukov <dvyukov@...gle.com>,
kasan-dev <kasan-dev@...glegroups.com>,
LKML <linux-kernel@...r.kernel.org>,
Linux Memory Management List <linux-mm@...ck.org>
Subject: Re: [PATCH 6/6] kasan: improve double-free reports.
On Tue, Aug 2, 2016 at 1:39 PM, Alexander Potapenko <glider@...gle.com> wrote:
> On Mon, Aug 1, 2016 at 4:45 PM, Andrey Ryabinin <aryabinin@...tuozzo.com> 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@...tuozzo.com>
>> ---
>> 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.
By the way, I think we could be doing a better job by always detecting
an invalid pointer being passed to kasan_slab_free().
>> + 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
--
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
Powered by blists - more mailing lists