From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751713AbbBPX4U (ORCPT ); Mon, 16 Feb 2015 18:56:20 -0500 Received: from ozlabs.org ([103.22.144.67]:51570 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751408AbbBPX4R (ORCPT ); Mon, 16 Feb 2015 18:56:17 -0500 From: Rusty Russell To: Andrey Ryabinin , linux-kernel@vger.kernel.org Cc: Dmitry Vyukov , Konstantin Serebryany , Dmitry Chernenkov , Andrey Konovalov , Yuri Gribov , Konstantin Khlebnikov , Sasha Levin , Christoph Lameter , Joonsoo Kim , Andrew Morton , Dave Hansen , Andi Kleen , x86@kernel.org, linux-mm@kvack.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Michal Marek , "open list\:KERNEL BUILD + fi..." Subject: Re: [PATCH v11 19/19] kasan: enable instrumentation of global variables In-Reply-To: <54E20238.3090902@samsung.com> References: <1404905415-9046-1-git-send-email-a.ryabinin@samsung.com> <1422985392-28652-1-git-send-email-a.ryabinin@samsung.com> <1422985392-28652-20-git-send-email-a.ryabinin@samsung.com> <87a90ea7ge.fsf@rustcorp.com.au> <54E20238.3090902@samsung.com> User-Agent: Notmuch/0.17 (http://notmuchmail.org) Emacs/24.3.1 (x86_64-pc-linux-gnu) Date: Tue, 17 Feb 2015 10:25:08 +1030 Message-ID: <877fvhtns3.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Andrey Ryabinin writes: > On 02/16/2015 05:58 AM, Rusty Russell wrote: >> Andrey Ryabinin writes: >>> This feature let us to detect accesses out of bounds of >>> global variables. This will work as for globals in kernel >>> image, so for globals in modules. Currently this won't work >>> for symbols in user-specified sections (e.g. __init, __read_mostly, ...) >>> @@ -1807,6 +1808,7 @@ static void unset_module_init_ro_nx(struct module *mod) { } >>> void __weak module_memfree(void *module_region) >>> { >>> vfree(module_region); >>> + kasan_module_free(module_region); >>> } >> >> This looks racy (memory reuse?). Perhaps try other order? >> > > You are right, it's racy. Concurrent kasan_module_alloc() could fail because > kasan_module_free() wasn't called/finished yet, so whole module_alloc() will fail > and module loading will fail. > However, I just find out that this race is not the worst problem here. > When vfree(addr) called in interrupt context, memory at addr will be reused for > storing 'struct llist_node': > > void vfree(const void *addr) > { > ... > if (unlikely(in_interrupt())) { > struct vfree_deferred *p = this_cpu_ptr(&vfree_deferred); > if (llist_add((struct llist_node *)addr, &p->list)) > schedule_work(&p->wq); > > > In this case we have to free shadow *after* freeing 'module_region', because 'module_region' > is still used in llist_add() and in free_work() latter. > free_work() (in mm/vmalloc.c) processes list in LIFO order, so to free shadow after freeing > 'module_region' kasan_module_free(module_region); should be called before vfree(module_region); > > It will be racy still, but this is not so bad as potential crash that we have now. > Honestly, I have no idea how to fix this race nicely. Any suggestions? I think you need to take over the rcu callback for the kasan case. Perhaps we rename that __module_memfree(), and do: void module_memfree(void *p) { #ifdef CONFIG_KASAN ... #endif __module_memfree(p); } Note: there are calls to module_memfree from other code (BPF and kprobes). I assume you looked at those too... Cheers, Rusty. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ozlabs.org ([103.22.144.67]:51570 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751408AbbBPX4R (ORCPT ); Mon, 16 Feb 2015 18:56:17 -0500 From: Rusty Russell Subject: Re: [PATCH v11 19/19] kasan: enable instrumentation of global variables In-Reply-To: <54E20238.3090902@samsung.com> References: <1404905415-9046-1-git-send-email-a.ryabinin@samsung.com> <1422985392-28652-1-git-send-email-a.ryabinin@samsung.com> <1422985392-28652-20-git-send-email-a.ryabinin@samsung.com> <87a90ea7ge.fsf@rustcorp.com.au> <54E20238.3090902@samsung.com> Date: Tue, 17 Feb 2015 10:25:08 +1030 Message-ID: <877fvhtns3.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kbuild-owner@vger.kernel.org List-ID: To: Andrey Ryabinin , linux-kernel@vger.kernel.org Cc: Dmitry Vyukov , Konstantin Serebryany , Dmitry Chernenkov , Andrey Konovalov , Yuri Gribov , Konstantin Khlebnikov , Sasha Levin , Christoph Lameter , Joonsoo Kim , Andrew Morton , Dave Hansen , Andi Kleen , x86@kernel.org, linux-mm@kvack.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Michal Marek , "open list:KERNEL BUILD + fi..." Andrey Ryabinin writes: > On 02/16/2015 05:58 AM, Rusty Russell wrote: >> Andrey Ryabinin writes: >>> This feature let us to detect accesses out of bounds of >>> global variables. This will work as for globals in kernel >>> image, so for globals in modules. Currently this won't work >>> for symbols in user-specified sections (e.g. __init, __read_mostly, ...) >>> @@ -1807,6 +1808,7 @@ static void unset_module_init_ro_nx(struct module *mod) { } >>> void __weak module_memfree(void *module_region) >>> { >>> vfree(module_region); >>> + kasan_module_free(module_region); >>> } >> >> This looks racy (memory reuse?). Perhaps try other order? >> > > You are right, it's racy. Concurrent kasan_module_alloc() could fail because > kasan_module_free() wasn't called/finished yet, so whole module_alloc() will fail > and module loading will fail. > However, I just find out that this race is not the worst problem here. > When vfree(addr) called in interrupt context, memory at addr will be reused for > storing 'struct llist_node': > > void vfree(const void *addr) > { > ... > if (unlikely(in_interrupt())) { > struct vfree_deferred *p = this_cpu_ptr(&vfree_deferred); > if (llist_add((struct llist_node *)addr, &p->list)) > schedule_work(&p->wq); > > > In this case we have to free shadow *after* freeing 'module_region', because 'module_region' > is still used in llist_add() and in free_work() latter. > free_work() (in mm/vmalloc.c) processes list in LIFO order, so to free shadow after freeing > 'module_region' kasan_module_free(module_region); should be called before vfree(module_region); > > It will be racy still, but this is not so bad as potential crash that we have now. > Honestly, I have no idea how to fix this race nicely. Any suggestions? I think you need to take over the rcu callback for the kasan case. Perhaps we rename that __module_memfree(), and do: void module_memfree(void *p) { #ifdef CONFIG_KASAN ... #endif __module_memfree(p); } Note: there are calls to module_memfree from other code (BPF and kprobes). I assume you looked at those too... Cheers, Rusty. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f52.google.com (mail-pa0-f52.google.com [209.85.220.52]) by kanga.kvack.org (Postfix) with ESMTP id 605C16B0032 for ; Mon, 16 Feb 2015 18:56:22 -0500 (EST) Received: by pabkx10 with SMTP id kx10so1829321pab.0 for ; Mon, 16 Feb 2015 15:56:22 -0800 (PST) Received: from ozlabs.org (ozlabs.org. [103.22.144.67]) by mx.google.com with ESMTPS id sz8si7595761pbc.86.2015.02.16.15.56.20 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Feb 2015 15:56:21 -0800 (PST) From: Rusty Russell Subject: Re: [PATCH v11 19/19] kasan: enable instrumentation of global variables In-Reply-To: <54E20238.3090902@samsung.com> References: <1404905415-9046-1-git-send-email-a.ryabinin@samsung.com> <1422985392-28652-1-git-send-email-a.ryabinin@samsung.com> <1422985392-28652-20-git-send-email-a.ryabinin@samsung.com> <87a90ea7ge.fsf@rustcorp.com.au> <54E20238.3090902@samsung.com> Date: Tue, 17 Feb 2015 10:25:08 +1030 Message-ID: <877fvhtns3.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain Sender: owner-linux-mm@kvack.org List-ID: To: Andrey Ryabinin , linux-kernel@vger.kernel.org Cc: Dmitry Vyukov , Konstantin Serebryany , Dmitry Chernenkov , Andrey Konovalov , Yuri Gribov , Konstantin Khlebnikov , Sasha Levin , Christoph Lameter , Joonsoo Kim , Andrew Morton , Dave Hansen , Andi Kleen , x86@kernel.org, linux-mm@kvack.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Michal Marek , "open list:KERNEL BUILD + fi..." Andrey Ryabinin writes: > On 02/16/2015 05:58 AM, Rusty Russell wrote: >> Andrey Ryabinin writes: >>> This feature let us to detect accesses out of bounds of >>> global variables. This will work as for globals in kernel >>> image, so for globals in modules. Currently this won't work >>> for symbols in user-specified sections (e.g. __init, __read_mostly, ...) >>> @@ -1807,6 +1808,7 @@ static void unset_module_init_ro_nx(struct module *mod) { } >>> void __weak module_memfree(void *module_region) >>> { >>> vfree(module_region); >>> + kasan_module_free(module_region); >>> } >> >> This looks racy (memory reuse?). Perhaps try other order? >> > > You are right, it's racy. Concurrent kasan_module_alloc() could fail because > kasan_module_free() wasn't called/finished yet, so whole module_alloc() will fail > and module loading will fail. > However, I just find out that this race is not the worst problem here. > When vfree(addr) called in interrupt context, memory at addr will be reused for > storing 'struct llist_node': > > void vfree(const void *addr) > { > ... > if (unlikely(in_interrupt())) { > struct vfree_deferred *p = this_cpu_ptr(&vfree_deferred); > if (llist_add((struct llist_node *)addr, &p->list)) > schedule_work(&p->wq); > > > In this case we have to free shadow *after* freeing 'module_region', because 'module_region' > is still used in llist_add() and in free_work() latter. > free_work() (in mm/vmalloc.c) processes list in LIFO order, so to free shadow after freeing > 'module_region' kasan_module_free(module_region); should be called before vfree(module_region); > > It will be racy still, but this is not so bad as potential crash that we have now. > Honestly, I have no idea how to fix this race nicely. Any suggestions? I think you need to take over the rcu callback for the kasan case. Perhaps we rename that __module_memfree(), and do: void module_memfree(void *p) { #ifdef CONFIG_KASAN ... #endif __module_memfree(p); } Note: there are calls to module_memfree from other code (BPF and kprobes). I assume you looked at those too... Cheers, Rusty. -- 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: email@kvack.org