From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f72.google.com (mail-it0-f72.google.com [209.85.214.72]) by kanga.kvack.org (Postfix) with ESMTP id 07BA36B0005 for ; Wed, 1 Jun 2016 12:21:35 -0400 (EDT) Received: by mail-it0-f72.google.com with SMTP id i127so48598212ita.2 for ; Wed, 01 Jun 2016 09:21:35 -0700 (PDT) Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0092.outbound.protection.outlook.com. [104.47.1.92]) by mx.google.com with ESMTPS id g58si3223400ote.168.2016.06.01.09.21.33 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 01 Jun 2016 09:21:34 -0700 (PDT) Subject: Re: [PATCH] mm: kasan: don't touch metadata in kasan_[un]poison_element() References: <1464785606-20349-1-git-send-email-glider@google.com> From: Andrey Ryabinin Message-ID: <574F0BB6.1040400@virtuozzo.com> Date: Wed, 1 Jun 2016 19:22:14 +0300 MIME-Version: 1.0 In-Reply-To: <1464785606-20349-1-git-send-email-glider@google.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Alexander Potapenko , adech.fo@gmail.com, cl@linux.com, dvyukov@google.com, akpm@linux-foundation.org, rostedt@goodmis.org, iamjoonsoo.kim@lge.com, js1304@gmail.com, kcc@google.com, kuthonuzo.luruo@hpe.com Cc: kasan-dev@googlegroups.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org On 06/01/2016 03:53 PM, Alexander Potapenko wrote: > To avoid draining the mempools, KASAN shouldn't put the mempool elements > into the quarantine upon mempool_free(). Correct, but unfortunately this patch doesn't fix that. > It shouldn't store > allocation/deallocation stacks upon mempool_alloc()/mempool_free() either. Why not? > Therefore make kasan_[un]poison_element() just change the shadow memory, > not the metadata. > > Signed-off-by: Alexander Potapenko > Reported-by: Kuthonuzo Luruo > --- [...] > +void kasan_slab_alloc(struct kmem_cache *cache, void *object, > + bool just_unpoison, gfp_t flags) > { > - kasan_kmalloc(cache, object, cache->object_size, flags); > + if (just_unpoison) This set to 'false' in all call sites. > + kasan_unpoison_shadow(object, cache->object_size); > + else > + kasan_kmalloc(cache, object, cache->object_size, flags); > } > > void kasan_poison_slab_free(struct kmem_cache *cache, void *object) > @@ -611,6 +615,31 @@ void kasan_kmalloc_large(const void *ptr, size_t size, gfp_t flags) > KASAN_PAGE_REDZONE); > } > > +void kasan_unpoison_kmalloc(const void *object, size_t size, gfp_t flags) > +{ > + struct page *page; > + unsigned long redzone_start; > + unsigned long redzone_end; > + > + if (unlikely(object == ZERO_SIZE_PTR) || (object == NULL)) > + return; > + > + page = virt_to_head_page(object); > + redzone_start = round_up((unsigned long)(object + size), > + KASAN_SHADOW_SCALE_SIZE); > + > + if (unlikely(!PageSlab(page))) > + redzone_end = (unsigned long)object + > + (PAGE_SIZE << compound_order(page)); > + else > + redzone_end = round_up( > + (unsigned long)object + page->slab_cache->object_size, > + KASAN_SHADOW_SCALE_SIZE); > + kasan_unpoison_shadow(object, size); > + kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start, > + KASAN_KMALLOC_REDZONE); > +} > + > void kasan_krealloc(const void *object, size_t size, gfp_t flags) > { > struct page *page; > @@ -636,7 +665,20 @@ void kasan_kfree(void *ptr) > kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page), > KASAN_FREE_PAGE); > else > - kasan_slab_free(page->slab_cache, ptr); > + kasan_poison_slab_free(page->slab_cache, ptr); > +} > + > +void kasan_poison_kfree(void *ptr) Unused > +{ > + struct page *page; > + > + page = virt_to_head_page(ptr); > + > + if (unlikely(!PageSlab(page))) > + kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page), > + KASAN_FREE_PAGE); > + else > + kasan_poison_slab_free(page->slab_cache, ptr); > } > > void kasan_kfree_large(const void *ptr) > diff --git a/mm/mempool.c b/mm/mempool.c > index 9e075f8..bcd48c6 100644 > --- a/mm/mempool.c > +++ b/mm/mempool.c > @@ -115,9 +115,10 @@ static void kasan_poison_element(mempool_t *pool, void *element) > static void kasan_unpoison_element(mempool_t *pool, void *element, gfp_t flags) > { > if (pool->alloc == mempool_alloc_slab) > - kasan_slab_alloc(pool->pool_data, element, flags); > + kasan_slab_alloc(pool->pool_data, element, > + /*just_unpoison*/ false, flags); > if (pool->alloc == mempool_kmalloc) > - kasan_krealloc(element, (size_t)pool->pool_data, flags); > + kasan_unpoison_kmalloc(element, (size_t)pool->pool_data, flags); I think, that the current code here is fine. We only need to fix kasan_poison_element() which calls kasan_kfree() that puts objects into quarantine. -- 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754379AbcFAQVg (ORCPT ); Wed, 1 Jun 2016 12:21:36 -0400 Received: from mail-ve1eur01on0131.outbound.protection.outlook.com ([104.47.1.131]:28518 "EHLO EUR01-VE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750828AbcFAQVe (ORCPT ); Wed, 1 Jun 2016 12:21:34 -0400 X-Greylist: delayed 1769 seconds by postgrey-1.27 at vger.kernel.org; Wed, 01 Jun 2016 12:21:34 EDT Authentication-Results: vger.kernel.org; dkim=none (message not signed) header.d=none;vger.kernel.org; dmarc=none action=none header.from=virtuozzo.com; Subject: Re: [PATCH] mm: kasan: don't touch metadata in kasan_[un]poison_element() To: Alexander Potapenko , , , , , , , , , References: <1464785606-20349-1-git-send-email-glider@google.com> CC: , , From: Andrey Ryabinin Message-ID: <574F0BB6.1040400@virtuozzo.com> Date: Wed, 1 Jun 2016 19:22:14 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <1464785606-20349-1-git-send-email-glider@google.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [195.214.232.10] X-ClientProxiedBy: VI1PR07CA0118.eurprd07.prod.outlook.com (10.165.229.172) To DB6PR0801MB1302.eurprd08.prod.outlook.com (10.168.11.20) X-MS-Office365-Filtering-Correlation-Id: a9c102f9-d36c-4ddb-ca7f-08d38a38cace X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1302;2:bg+95ghU6zOZBRv0ftUjPQ4VVK+pMHTWo2r8qUu4bzTPWyFPA/5voaveDbvFrmANpJTH9gVhw2vYbWcxUCXrOqwtisPO6gWnCYSSFQbC0GeTaAbLIKDeXAi3e4LH/vEBHLP6H8TU25dyClacJki8VUF9Vp7JuCEzut5WngXUQRVHkJTyIuG7yiKHTz8SHqNs;3:cz9RS4wERLWpbv+MomPaxHXioADf/BOy5XmBQvCCHXCXXGHc41kfu0dQ+CbugGUWJhoy2HhDLuuY9FEcq1cMp9ZWzhuxPCc5OcBGbgUvcqcH0TgLlZL/fxvlZY4J583x X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DB6PR0801MB1302; X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1302;25:R7rPkiJbZffTXYffkWXe5uHVbyNKZxFPZQJH/Cg6xVh+pKSb6hUXs48DIpVbf4soNaGTKhX6eytQca/zGE3D9j6ijtdkpI4zG3spDju2A+TRb6pqm44Un62ffX2q32EYElBtS7zBQvU0zqlvobFk/ML9Q0ocFnhHiSv3194wkXqqSFObW/bn9/N1ghRJ9yDky/HFroGr8Dm98ga6DhS+5R3RL15rytqnYqa0uNXONOpbcCHdPthyOJallqCVckNmOy7ZjSmsO9Voj9Mpd+INdPrXEIi3ErZCX4OftaHLX6zhoVy6D0/7ansxs9OSplnzG2A1ricNEXZpx1vtD1FGmen45h/N0tJJpmPJxgcViH11BHJiVnKBCuM7yqZsSQzNJYGa5Ij8gfHNMHIL6Ec0zTdsGwgF/kRj9nU61VTUKSEnYItr1UHZQZPO/Mx7PicqA55+wLztT10Nu+c/FBrmKbO7KNr4pG1jM9ZKxKnTpoMmf1/xuPA/4cB0URQP3Tb8FnFRmTI1yoRDtwYMbr8H1cQAxtYzngfRAOQ5f6JLyUwntp7mhGa1+rhXm7cHkMozBRd8+VgGO+wVc5BWhHeOXpmQqD6MhJYMaMXdqi8YYrKJU4VFg0o3c+7qGKj0pvj6GGAV8+K7Z0IEkw4KFOjBIMoWfDOR/M9+E6QdYwLs3afhIlsouK2+DoHI0GQ6r/tzw3MEXjZGIC5ix6JvkmvBmA== X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(227479698468861)(131327999870524)(211936372134217); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040130)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6041072)(6043046);SRVR:DB6PR0801MB1302;BCL:0;PCL:0;RULEID:;SRVR:DB6PR0801MB1302; X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1302;4:cq3x+u3dj8QLwmovw+MJH5vlEmf+eGmN63z0tK6PldUB3XE9/SWa1E+hYKd8YUjdK0oTNPvb6MbqbCwo3G/2IemBbyBGER4MnHY0hSWnJftVArLpoGboHIS7PRHkU/LSOTidpja2UnGIFlzC8yKj9qq2hNHwpEMJ8f0dJlKbnNa6OgCKky1mNIp0uVy0SjKd3wKlkHjl4tLZJEiD0EcqEBX1m3Qm7+m6EXPZtVS20ESdowJznz2E2V0wElppwivvdzt1FWGpnE5NqYaX2rgyORjbxQIlY4M2AIx7kBpcvzzMBeCcX6QAx5jn5EN/tzf9Dvc/7WCUOxCVbSTwA/ANfg9nezCx+gdHI5kTK6IIKED9omAHEcLcBdqOaBl/8axAhY6Whx8D5Z5+H3P/DcOBoYmEGrTHyyXJNCiOgVbfi9g57ze0sOGWmebTDbFo2AblR8rzHtwDQuWMk69ITKlSYD67VJYkATe8aYagk8UcLMkQFa0wySt/FkVPx15ZkXw/R1jEfhkuGd6d0Avkol3oWpDG12z7E/byq3TAgPEt2h8= X-Forefront-PRVS: 096029FF66 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6049001)(6009001)(377454003)(24454002)(92566002)(65806001)(81166006)(66066001)(83506001)(54356999)(586003)(19580395003)(3846002)(80316001)(65816999)(8676002)(77096005)(2906002)(87266999)(19580405001)(76176999)(2950100001)(59896002)(4326007)(50986999)(230700001)(36756003)(6116002)(65956001)(189998001)(23746002)(33656002)(5008740100001)(86362001)(42186005)(47776003)(5001770100001)(50466002)(2201001)(5004730100002)(4001350100001)(921003)(1121003)(2101003)(83996005);DIR:OUT;SFP:1102;SCL:1;SRVR:DB6PR0801MB1302;H:[10.30.19.223];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?Windows-1252?Q?1;DB6PR0801MB1302;23:KFMZpEkRRyPUnGhDwRU6YGvI1Cn25Z7/a0R?= =?Windows-1252?Q?uOlj4H1+rebuveDuuXMXv9DWMNq53EfPmoTAy7dE3+QnugkFghFY6Mcw?= =?Windows-1252?Q?cROmtKClwHppRtaKYAzYr3og2SbIfDj2skM8yeeuU6huPizCEH4jGLiX?= =?Windows-1252?Q?CnNtWOpg5L+O5vg6sQSejUhTwDGFfv/c2p8ay6TGrJoXN4j6QFt9MPBq?= =?Windows-1252?Q?K1JIfAn7rTgzD1doNfEoq2aroh0PITd1pJQTIDQ5z8C7bsnvuVUPg+3+?= =?Windows-1252?Q?xueQmq0rXSBhpkde8HBTc0IvbElSCFpLXQkwXs526H12khLY8nVzVPq3?= =?Windows-1252?Q?173tgR/oMAbjPlVH1NhG1JNcrEI3XvGIxNJMNup1vcvmpDifuqonhlP+?= =?Windows-1252?Q?0CAKR6uPRkv8/L0OnqZhoB4Nuidt8wu08ciI9HHjxnqDvndCXMmy4aCw?= =?Windows-1252?Q?mY2WUVLz+gMlCtzAQjxk+z6S3GKKT79NzZ2zjrwQMuZe/fvis93k1Zy6?= =?Windows-1252?Q?W2apUp+EEbt9vVtrqCNfc95ZJZz1iwoW2ALXngKOZps9hFR64icFat+U?= =?Windows-1252?Q?jDkSRLSpQmATzJx9WwC0cQcCKkYYPNIYG1wumCKJjt5aBUX8LH0zy8iJ?= =?Windows-1252?Q?xRZXvD7HEb7bvzW6YMtjwCiV4EYrmTg5DXfUzkY4O89uYcKEJ3P2BHuv?= =?Windows-1252?Q?IsU9JBKH/fII0Jwrbe9zOPBynaqeWqFrajqsZcAwTbsl/OVC7k9j3f/L?= =?Windows-1252?Q?G0QZB8jHzggIXpVXXSls8h0Ul1uI+SUpHNr4DCCGiSWNv9kSgwhKCD7X?= =?Windows-1252?Q?uD72F6Qcs1TJXBiyvr4umY9S6XAelIPgla7y7EUk9Gz/525OXays1jgg?= =?Windows-1252?Q?InQ7ec82JpM2zGRrgJ4lwWltInYrXjmFhtvUk93HelJ53qqF/kUFNvmr?= =?Windows-1252?Q?HitFRhJdKZvyFRhR3bjeRkQ0qDLnOfxDMJXSvCOfZCkLtLT99dpDx5PY?= =?Windows-1252?Q?dZk+6oWDrJ539a5jDQMupmENcfbyRKb7KspyXy3+q1L6yihb4SrSTOBL?= =?Windows-1252?Q?07k9ZVU8BpINGKbpcFqXragRJEld/DurwTaSBS8bZPvtInA8o8448koZ?= =?Windows-1252?Q?Up7U/rD1FR9jRBViUNuZlsT2sNTN908RqsYobtpb2bXdzsUbqjzufDFE?= =?Windows-1252?Q?Ews2K5uow+hW6gqpoGnYF9Xd9tPutxMGfNNnv7hdc2cpIZALKzlCai3p?= =?Windows-1252?Q?n/Hw+biG582byqGcgCzGq7QBYBPS1RCTXWK5zBBc=3D?= X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1302;5:5O3niOfYqVa1ZXIem/e1okUn2WLKVVumZqQSCb649sM6yNshNpGx+IMT3XmnWrafTGuTp443/jgY3gG0dmMI/R8/sUqxKMiE71DtDwiFDoRUYXsDvjGoadI0B+UtMO5cAw7skwhj61eLlzdvvqufmA==;24:E3BfVqr3x1z/bsSxJ1SavHx99eh79D4hDHS+W6gZo5sKd71RphUs7CF5Gs0xkEqvc+Ev7+ssELQh6BqYiiIAQY6beb2ohfUyDxw0/mUi1CU=;7:+CZlf2mV4G2LtY6ACiLdQkX/uKNtHkSqwQ49fyqgp7rsWcVWzG80h+amc+q9ykV7ux1X7XmGTTQOCRyGDtT5OvfMELuzm1NdGeiIK89yRUFeuZ+tbiRPP45fc1Y/7zokOKzerCO+69ipjx5fLr5cj0xexar1kMKW3ErqiPWux9/S0p+/b0aPDZ4GcaKEaG+x;20:hSp0CQZCWUTmgxHQ9UHKto+DGAP9vBuB+F4IuQVpbQOpMgFoD0Ta9MByIWpMc77VTaPizF6t/h9xJwgc7kS+1uppZH4oVw4Smvr2O/3ODu5CnwQ+DpqCFxoM10se+cQFZMjuOb6zj99jOETrPCv32VbSP2LJ/iQFTXbMZA+7d+E= SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: virtuozzo.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Jun 2016 16:21:29.9254 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0801MB1302 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/01/2016 03:53 PM, Alexander Potapenko wrote: > To avoid draining the mempools, KASAN shouldn't put the mempool elements > into the quarantine upon mempool_free(). Correct, but unfortunately this patch doesn't fix that. > It shouldn't store > allocation/deallocation stacks upon mempool_alloc()/mempool_free() either. Why not? > Therefore make kasan_[un]poison_element() just change the shadow memory, > not the metadata. > > Signed-off-by: Alexander Potapenko > Reported-by: Kuthonuzo Luruo > --- [...] > +void kasan_slab_alloc(struct kmem_cache *cache, void *object, > + bool just_unpoison, gfp_t flags) > { > - kasan_kmalloc(cache, object, cache->object_size, flags); > + if (just_unpoison) This set to 'false' in all call sites. > + kasan_unpoison_shadow(object, cache->object_size); > + else > + kasan_kmalloc(cache, object, cache->object_size, flags); > } > > void kasan_poison_slab_free(struct kmem_cache *cache, void *object) > @@ -611,6 +615,31 @@ void kasan_kmalloc_large(const void *ptr, size_t size, gfp_t flags) > KASAN_PAGE_REDZONE); > } > > +void kasan_unpoison_kmalloc(const void *object, size_t size, gfp_t flags) > +{ > + struct page *page; > + unsigned long redzone_start; > + unsigned long redzone_end; > + > + if (unlikely(object == ZERO_SIZE_PTR) || (object == NULL)) > + return; > + > + page = virt_to_head_page(object); > + redzone_start = round_up((unsigned long)(object + size), > + KASAN_SHADOW_SCALE_SIZE); > + > + if (unlikely(!PageSlab(page))) > + redzone_end = (unsigned long)object + > + (PAGE_SIZE << compound_order(page)); > + else > + redzone_end = round_up( > + (unsigned long)object + page->slab_cache->object_size, > + KASAN_SHADOW_SCALE_SIZE); > + kasan_unpoison_shadow(object, size); > + kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start, > + KASAN_KMALLOC_REDZONE); > +} > + > void kasan_krealloc(const void *object, size_t size, gfp_t flags) > { > struct page *page; > @@ -636,7 +665,20 @@ void kasan_kfree(void *ptr) > kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page), > KASAN_FREE_PAGE); > else > - kasan_slab_free(page->slab_cache, ptr); > + kasan_poison_slab_free(page->slab_cache, ptr); > +} > + > +void kasan_poison_kfree(void *ptr) Unused > +{ > + struct page *page; > + > + page = virt_to_head_page(ptr); > + > + if (unlikely(!PageSlab(page))) > + kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page), > + KASAN_FREE_PAGE); > + else > + kasan_poison_slab_free(page->slab_cache, ptr); > } > > void kasan_kfree_large(const void *ptr) > diff --git a/mm/mempool.c b/mm/mempool.c > index 9e075f8..bcd48c6 100644 > --- a/mm/mempool.c > +++ b/mm/mempool.c > @@ -115,9 +115,10 @@ static void kasan_poison_element(mempool_t *pool, void *element) > static void kasan_unpoison_element(mempool_t *pool, void *element, gfp_t flags) > { > if (pool->alloc == mempool_alloc_slab) > - kasan_slab_alloc(pool->pool_data, element, flags); > + kasan_slab_alloc(pool->pool_data, element, > + /*just_unpoison*/ false, flags); > if (pool->alloc == mempool_kmalloc) > - kasan_krealloc(element, (size_t)pool->pool_data, flags); > + kasan_unpoison_kmalloc(element, (size_t)pool->pool_data, flags); I think, that the current code here is fine. We only need to fix kasan_poison_element() which calls kasan_kfree() that puts objects into quarantine.