From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932087AbcEKJE5 (ORCPT ); Wed, 11 May 2016 05:04:57 -0400 Received: from mail-lf0-f42.google.com ([209.85.215.42]:34002 "EHLO mail-lf0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751751AbcEKJEu convert rfc822-to-8bit (ORCPT ); Wed, 11 May 2016 05:04:50 -0400 MIME-Version: 1.0 In-Reply-To: References: <48cc05da0a19447843c6479cf1c15dbc174503a0.1458036040.git.glider@google.com> Date: Wed, 11 May 2016 11:04:47 +0200 Message-ID: Subject: Re: [PATCH v8 7/7] mm: kasan: Initial memory quarantine implementation From: Alexander Potapenko To: Andrey Ryabinin Cc: Andrey Konovalov , Christoph Lameter , Dmitry Vyukov , Andrew Morton , Steven Rostedt , Joonsoo Kim , Joonsoo Kim , Kostya Serebryany , kasan-dev , LKML , "linux-mm@kvack.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 10, 2016 at 9:57 PM, Andrey Ryabinin wrote: > 2016-05-10 20:17 GMT+03:00 Alexander Potapenko : >> On Tue, May 10, 2016 at 5:39 PM, Andrey Ryabinin wrote: >>> 2016-03-15 13:10 GMT+03:00 Alexander Potapenko : >>> >>>> >>>> static inline int kasan_module_alloc(void *addr, size_t size) { return 0; } >>>> static inline void kasan_free_shadow(const struct vm_struct *vm) {} >>>> diff --git a/lib/test_kasan.c b/lib/test_kasan.c >>>> index 82169fb..799c98e 100644 >>>> --- a/lib/test_kasan.c >>>> +++ b/lib/test_kasan.c >>>> @@ -344,6 +344,32 @@ static noinline void __init kasan_stack_oob(void) >>>> *(volatile char *)p; >>>> } >>>> >>>> +#ifdef CONFIG_SLAB >>>> +static noinline void __init kasan_quarantine_cache(void) >>>> +{ >>>> + struct kmem_cache *cache = kmem_cache_create( >>>> + "test", 137, 8, GFP_KERNEL, NULL); >>>> + int i; >>>> + >>>> + for (i = 0; i < 100; i++) { >>>> + void *p = kmem_cache_alloc(cache, GFP_KERNEL); >>>> + >>>> + kmem_cache_free(cache, p); >>>> + p = kmalloc(sizeof(u64), GFP_KERNEL); >>>> + kfree(p); >>>> + } >>>> + kmem_cache_shrink(cache); >>>> + for (i = 0; i < 100; i++) { >>>> + u64 *p = kmem_cache_alloc(cache, GFP_KERNEL); >>>> + >>>> + kmem_cache_free(cache, p); >>>> + p = kmalloc(sizeof(u64), GFP_KERNEL); >>>> + kfree(p); >>>> + } >>>> + kmem_cache_destroy(cache); >>>> +} >>>> +#endif >>>> + >>> >>> Test looks quite useless. The kernel does allocations/frees all the >>> time, so I don't think that this test >>> adds something valuable. >> Agreed. >>> And what's the result that we expect from this test? No crashes? >>> I'm thinking it would better to remove it. >> Do you think it may make sense to improve it by introducing an actual >> use-after-free? >> Or perhaps we could insert a loop doing 1000 kmalloc()/kfree() calls >> into the existing UAF tests. > > You don't need to do an actual UAF, all you need is to > make sure that repeated kmalloc() + kfree() produces new addresses. > > But I personally wouldn't bother with testing this at all. So, unless > you care, just remove the test. Well, I tend to agree. Such a test won't behave deterministically neither with KASAN nor without, which is not good. >>> >>>> + >>>> +/* smp_load_acquire() here pairs with smp_store_release() in >>>> + * quarantine_reduce(). >>>> + */ >>>> +#define QUARANTINE_LOW_SIZE (smp_load_acquire(&quarantine_size) * 3 / 4) >>> >>> I'd prefer open coding barrier with a proper comment int place, >>> instead of sneaking it into macros. >> Ack. >>> [...] >>> >>>> + >>>> +void quarantine_reduce(void) >>>> +{ >>>> + size_t new_quarantine_size; >>>> + unsigned long flags; >>>> + struct qlist to_free = QLIST_INIT; >>>> + size_t size_to_free = 0; >>>> + void **last; >>>> + >>>> + /* smp_load_acquire() here pairs with smp_store_release() below. */ >>> >>> Besides pairing rules, the comment should also explain *why* we need >>> this and for what >>> load/stores it provides memory ordering guarantees. For example take a >>> look at other >>> comments near barriers in the kernel tree. >> Something along the lines of "We must load A before B, hence the barrier"? > > Yes. > BTW, do we really need these barriers? I didn't tried to understand > this, thus could be wrong here, > but it seems that READ_ONCE/WRITE_ONCE would be enough. You're right. |quarantine_size| doesn't have any associated data accesses to which must be ordered with accesses to |quarantine_size| itself. > >>>> + if (likely(ACCESS_ONCE(global_quarantine.bytes) <= >>>> + smp_load_acquire(&quarantine_size))) >>>> + return; >>>> + >>>> -- 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