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

[Alexander Potapenko]

On Tue, May 10, 2016 at 9:57 PM, Andrey Ryabinin <ryabinin.a.a@xxxxxxxxx> wrote:
> 2016-05-10 20:17 GMT+03:00 Alexander Potapenko <glider@xxxxxxxxxx>:
>> On Tue, May 10, 2016 at 5:39 PM, Andrey Ryabinin <ryabinin.a.a@xxxxxxxxx> wrote:
>>> 2016-03-15 13:10 GMT+03:00 Alexander Potapenko <glider@xxxxxxxxxx>:
>>>
>>>>
>>>>  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