lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 22 Nov 2023 17:31:19 +0800
From:   Juntong Deng <juntong.deng@...look.com>
To:     Andrey Konovalov <andreyknvl@...il.com>
Cc:     ryabinin.a.a@...il.com, glider@...gle.com, dvyukov@...gle.com,
        vincenzo.frascino@....com, akpm@...ux-foundation.org,
        kasan-dev@...glegroups.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH v2] kasan: Improve free meta storage in Generic KASAN

On 2023/11/22 10:27, Andrey Konovalov wrote:
> On Tue, Nov 21, 2023 at 10:42 PM Juntong Deng <juntong.deng@...look.com> wrote:
>>
>> Currently free meta can only be stored in object if the object is
>> not smaller than free meta.
>>
>> After the improvement, even when the object is smaller than free meta,
>> it is still possible to store part of the free meta in the object,
>> reducing the increased size of the redzone.
>>
>> Example:
>>
>> free meta size: 16 bytes
>> alloc meta size: 16 bytes
>> object size: 8 bytes
>> optimal redzone size (object_size <= 64): 16 bytes
>>
>> Before improvement:
>> actual redzone size = alloc meta size + free meta size = 32 bytes
>>
>> After improvement:
>> actual redzone size = alloc meta size + (free meta size - object size)
>>                      = 24 bytes
>>
>> Suggested-by: Dmitry Vyukov <dvyukov@...gle.com>
>> Signed-off-by: Juntong Deng <juntong.deng@...look.com>
> 
> I think this change as is does not work well with slub_debug.
> 
> slub_debug puts its metadata (redzone, tracks, and orig_size) right
> after the object (see calculate_sizes and the comment before
> check_pad_bytes). With the current code, KASAN's free meta either fits
> within the object or is placed after the slub_debug metadata and
> everything works well. With this change, KASAN's free meta tail goes
> right past object_size, overlaps with the slub_debug metadata, and
> thus can corrupt it.
> 
> Thus, to make this patch work properly, we need to carefully think
> about all metadatas layout and teach slub_debug that KASAN's free meta
> can go past object_size. Possibly, adjusting s->inuse by the size of
> KASAN's metas (along with moving kasan_cache_create and fixing up
> set_orig_size) would be enough. But I'm not familiar with the
> slub_debug code enough to be sure.
> 
> If you decide to proceed with improving this change, I've left some
> comments for the current code below.
> 
> Thank you!
> 


I delved into the memory layout of SLUB_DEBUG today.

I think a better option would be to let the free meta not pass through
the object when SLUB_DEBUG is enabled.

In other words, the free meta continues to be stored according to the
previous method when SLUB_DEBUG is enabled.

Even if we teach SLUB_DEBUG that KASAN's free meta may pass through the
object and move SLUB_DEBUG's metadata backward, it still destroys the
original design intent of SLUB_DEBUG.

Because SLUB_DEBUG checks for out-of-bounds by filling the redzones
on both sides of the object with magic number, if SLUB_DEBUG's redzones
move backward, leaving a gap, that will break the out-of-bounds
checking.

I will send patch V3 to fix this issue.


>> ---
>> V1 -> V2: Make kasan_metadata_size() adapt to the improved
>> free meta storage
>>
>>   mm/kasan/generic.c | 50 +++++++++++++++++++++++++++++++---------------
>>   1 file changed, 34 insertions(+), 16 deletions(-)
>>
>> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
>> index 4d837ab83f08..802c738738d7 100644
>> --- a/mm/kasan/generic.c
>> +++ b/mm/kasan/generic.c
>> @@ -361,6 +361,8 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>>   {
>>          unsigned int ok_size;
>>          unsigned int optimal_size;
>> +       unsigned int rem_free_meta_size;
>> +       unsigned int orig_alloc_meta_offset;
>>
>>          if (!kasan_requires_meta())
>>                  return;
>> @@ -394,6 +396,9 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>>                  /* Continue, since free meta might still fit. */
>>          }
>>
>> +       ok_size = *size;
>> +       orig_alloc_meta_offset = cache->kasan_info.alloc_meta_offset;
>> +
>>          /*
>>           * Add free meta into redzone when it's not possible to store
>>           * it in the object. This is the case when:
>> @@ -401,21 +406,26 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>>           *    be touched after it was freed, or
>>           * 2. Object has a constructor, which means it's expected to
>>           *    retain its content until the next allocation, or
> 
> Please drop "or" on the line above.
> 
>> -        * 3. Object is too small.
>>           * Otherwise cache->kasan_info.free_meta_offset = 0 is implied.
>> +        * Even if the object is smaller than free meta, it is still
>> +        * possible to store part of the free meta in the object.
>>           */
>> -       if ((cache->flags & SLAB_TYPESAFE_BY_RCU) || cache->ctor ||
>> -           cache->object_size < sizeof(struct kasan_free_meta)) {
>> -               ok_size = *size;
>> -
>> +       if ((cache->flags & SLAB_TYPESAFE_BY_RCU) || cache->ctor) {
>>                  cache->kasan_info.free_meta_offset = *size;
>>                  *size += sizeof(struct kasan_free_meta);
>> +       } else if (cache->object_size < sizeof(struct kasan_free_meta)) {
>> +               rem_free_meta_size = sizeof(struct kasan_free_meta) -
>> +                                                               cache->object_size;
>> +               *size += rem_free_meta_size;
>> +               if (cache->kasan_info.alloc_meta_offset != 0)
>> +                       cache->kasan_info.alloc_meta_offset += rem_free_meta_size;
>> +       }
>>
>> -               /* If free meta doesn't fit, don't add it. */
>> -               if (*size > KMALLOC_MAX_SIZE) {
>> -                       cache->kasan_info.free_meta_offset = KASAN_NO_FREE_META;
>> -                       *size = ok_size;
>> -               }
>> +       /* If free meta doesn't fit, don't add it. */
>> +       if (*size > KMALLOC_MAX_SIZE) {
>> +               cache->kasan_info.free_meta_offset = KASAN_NO_FREE_META;
>> +               cache->kasan_info.alloc_meta_offset = orig_alloc_meta_offset;
>> +               *size = ok_size;
>>          }
>>
>>          /* Calculate size with optimal redzone. */
>> @@ -464,12 +474,20 @@ size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object)
>>          if (in_object)
>>                  return (info->free_meta_offset ?
>>                          0 : sizeof(struct kasan_free_meta));
> 
> This needs to be changed as well to something like min(cache->object,
> sizeof(struct kasan_free_meta)). However, with the slub_debug
> conflicts I mentioned above, we might need to change this to something
> else.
> 
> 
> 
>> -       else
>> -               return (info->alloc_meta_offset ?
>> -                       sizeof(struct kasan_alloc_meta) : 0) +
>> -                       ((info->free_meta_offset &&
>> -                       info->free_meta_offset != KASAN_NO_FREE_META) ?
>> -                       sizeof(struct kasan_free_meta) : 0);
>> +       else {
>> +               size_t alloc_meta_size = info->alloc_meta_offset ?
>> +                                                               sizeof(struct kasan_alloc_meta) : 0;
>> +               size_t free_meta_size = 0;
>> +
>> +               if (info->free_meta_offset != KASAN_NO_FREE_META) {
>> +                       if (info->free_meta_offset)
>> +                               free_meta_size = sizeof(struct kasan_free_meta);
>> +                       else if (cache->object_size < sizeof(struct kasan_free_meta))
>> +                               free_meta_size = sizeof(struct kasan_free_meta) -
>> +                                                                       cache->object_size;
>> +               }
>> +               return alloc_meta_size + free_meta_size;
>> +       }
>>   }
>>
>>   static void __kasan_record_aux_stack(void *addr, bool can_alloc)
>> --
>> 2.39.2

Powered by blists - more mailing lists