Re: [PATCH v3 09/13] mm: slub: add kernel address sanitizer support for slub allocator

[Andrey Ryabinin]

On 09/26/2014 08:48 AM, Dmitry Vyukov wrote:
> On Wed, Sep 24, 2014 at 5:44 AM, Andrey Ryabinin <a.ryabinin@xxxxxxxxxxx> wrote:
>> --- a/lib/Kconfig.kasan
>> +++ b/lib/Kconfig.kasan
>> @@ -6,6 +6,7 @@ if HAVE_ARCH_KASAN
>>  config KASAN
>>         bool "AddressSanitizer: runtime memory debugger"
>>         depends on !MEMORY_HOTPLUG
>> +       depends on SLUB_DEBUG
> 
> 
> What does SLUB_DEBUG do? I think that generally we don't want any
> other *heavy* debug checks to be required for kasan.
> 

SLUB_DEBUG enables support for different debugging features.
It doesn't enables this debugging features by default, it only allows
you to switch them on/off in runtime.
Generally SLUB_DEBUG option is enabled in most kernels. SLUB_DEBUG disabled
only with intention to get minimal kernel.

Without SLUB_DEBUG there will be no redzones, no user tracking info (allocation/free stacktraces).
KASAN won't be so usefull without SLUB_DEBUG.


[...]

>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -30,6 +30,7 @@
>>  #include <linux/kasan.h>
>>
>>  #include "kasan.h"
>> +#include "../slab.h"
>>
>>  /*
>>   * Poisons the shadow memory for 'size' bytes starting from 'addr'.
>> @@ -265,6 +266,102 @@ void kasan_free_pages(struct page *page, unsigned int order)
>>                                 KASAN_FREE_PAGE);
>>  }
>>
>> +void kasan_free_slab_pages(struct page *page, int order)
> 
> Doesn't this callback followed by actually freeing the pages, and so
> kasan_free_pages callback that will poison the range? If so, I would
> prefer to not double poison.
> 

Yes, this could be removed.

> 
>> +{
>> +       kasan_poison_shadow(page_address(page),
>> +                       PAGE_SIZE << order, KASAN_SLAB_FREE);
>> +}
>> +
>> +void kasan_mark_slab_padding(struct kmem_cache *s, void *object)
>> +{
>> +       unsigned long object_end = (unsigned long)object + s->size;
>> +       unsigned long padding_end = round_up(object_end, PAGE_SIZE);
>> +       unsigned long padding_start = round_up(object_end,
>> +                                       KASAN_SHADOW_SCALE_SIZE);
>> +       size_t size = padding_end - padding_start;
>> +
>> +       if (size)
>> +               kasan_poison_shadow((void *)padding_start,
>> +                               size, KASAN_SLAB_PADDING);
>> +}
>> +
>> +void kasan_slab_alloc(struct kmem_cache *cache, void *object)
>> +{
>> +       kasan_kmalloc(cache, object, cache->object_size);
>> +}
>> +
>> +void kasan_slab_free(struct kmem_cache *cache, void *object)
>> +{
>> +       unsigned long size = cache->size;
>> +       unsigned long rounded_up_size = round_up(size, KASAN_SHADOW_SCALE_SIZE);
>> +
> 
> Add a comment saying that SLAB_DESTROY_BY_RCU objects can be "legally"
> used after free.
> 

Ok.

>> +       if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
>> +               return;
>> +
>> +       kasan_poison_shadow(object, rounded_up_size, KASAN_KMALLOC_FREE);
>> +}
>> +
>> +void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size)
>> +{
>> +       unsigned long redzone_start;
>> +       unsigned long redzone_end;
>> +
>> +       if (unlikely(object == NULL))
>> +               return;
>> +
>> +       redzone_start = round_up((unsigned long)(object + size),
>> +                               KASAN_SHADOW_SCALE_SIZE);
>> +       redzone_end = (unsigned long)object + cache->size;
>> +
>> +       kasan_unpoison_shadow(object, size);
>> +       kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start,
>> +               KASAN_KMALLOC_REDZONE);
>> +
>> +}
>> +EXPORT_SYMBOL(kasan_kmalloc);
>> +
>> +void kasan_kmalloc_large(const void *ptr, size_t size)
>> +{
>> +       struct page *page;
>> +       unsigned long redzone_start;
>> +       unsigned long redzone_end;
>> +
>> +       if (unlikely(ptr == NULL))
>> +               return;
>> +
>> +       page = virt_to_page(ptr);
>> +       redzone_start = round_up((unsigned long)(ptr + size),
>> +                               KASAN_SHADOW_SCALE_SIZE);
>> +       redzone_end = (unsigned long)ptr + (PAGE_SIZE << compound_order(page));
> 
> If size == N*PAGE_SIZE - KASAN_SHADOW_SCALE_SIZE - 1, the object does
> not receive any redzone at all. 

If size == N*PAGE_SIZE - KASAN_SHADOW_SCALE_SIZE - 1, there will be redzone
KASAN_SHADOW_SCALE_SIZE + 1 bytes. There will be no readzone if and only if
(size == PAGE_SIZE << compound_order(page))

> Can we pass full memory block size
> from above to fix it? Will compound_order(page) do?
> 

What is full memory block size?
PAGE_SIZE << compound_order(page) is how much was really allocated.


[..]

>>
>>  static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
>> @@ -1416,8 +1426,10 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
>>                 setup_object(s, page, p);
>>                 if (likely(idx < page->objects))
>>                         set_freepointer(s, p, p + s->size);
> 
> Sorry, I don't fully follow this code, so I will just ask some questions.
> Can we have some slab padding after last object in this case as well?
> 
This case is for not the last object. Padding is the place after the last object.
The last object initialized bellow in else case.

>> -               else
>> +               else {
>>                         set_freepointer(s, p, NULL);
>> +                       kasan_mark_slab_padding(s, p);
> 
> kasan_mark_slab_padding poisons only up to end of the page. Can there
> be multiple pages that we need to poison?
> 
Yep, that's a good catch.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/