Re: [PATCH v8 7/7] mm: kasan: Initial memory quarantine implementation

[Alexander Potapenko <glider@google.com>]

On Tue, May 10, 2016 at 5:39 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
> 2016-03-15 13:10 GMT+03:00 Alexander Potapenko <glider@google.com>:
>
>>
>>  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.
> [...]
>
>> +
>> +/* 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"?
>> +       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