On 15.08.2020 19:52, Kees Cook wrote:
> On Thu, Aug 13, 2020 at 06:19:21PM +0300, Alexander Popov wrote:
>> Heap spraying is an exploitation technique that aims to put controlled
>> bytes at a predetermined memory location on the heap. Heap spraying for
>> exploiting use-after-free in the Linux kernel relies on the fact that on
>> kmalloc(), the slab allocator returns the address of the memory that was
>> recently freed. Allocating a kernel object with the same size and
>> controlled contents allows overwriting the vulnerable freed object.
>>
>> Let's extract slab freelist quarantine from KASAN functionality and
>> call it CONFIG_SLAB_QUARANTINE. This feature breaks widespread heap
>> spraying technique used for exploiting use-after-free vulnerabilities
>> in the kernel code.
>>
>> If this feature is enabled, freed allocations are stored in the quarantine
>> and can't be instantly reallocated and overwritten by the exploit
>> performing heap spraying.
>
> It may be worth clarifying that this is specifically only direct UAF and
> doesn't help with spray-and-overflow-into-a-neighboring-object attacks
> (i.e. both tend to use sprays, but the former doesn't depend on a write
> overflow).
Right, thank you.
>> Signed-off-by: Alexander Popov <alex.popov@xxxxxxxxx>
>> ---
>> include/linux/kasan.h | 107 ++++++++++++++++++++-----------------
>> include/linux/slab_def.h | 2 +-
>> include/linux/slub_def.h | 2 +-
>> init/Kconfig | 11 ++++
>> mm/Makefile | 3 +-
>> mm/kasan/Makefile | 2 +
>> mm/kasan/kasan.h | 75 +++++++++++++-------------
>> mm/kasan/quarantine.c | 2 +
>> mm/kasan/slab_quarantine.c | 99 ++++++++++++++++++++++++++++++++++
>> mm/slub.c | 2 +-
>> 10 files changed, 216 insertions(+), 89 deletions(-)
>> create mode 100644 mm/kasan/slab_quarantine.c
>>
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index 087fba34b209..b837216f760c 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
[...]
>> #else /* CONFIG_KASAN_GENERIC */
>> +static inline void kasan_record_aux_stack(void *ptr) {}
>> +#endif /* CONFIG_KASAN_GENERIC */
>>
>> +#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_SLAB_QUARANTINE)
>> +void kasan_cache_shrink(struct kmem_cache *cache);
>> +void kasan_cache_shutdown(struct kmem_cache *cache);
>> +#else /* CONFIG_KASAN_GENERIC || CONFIG_SLAB_QUARANTINE */
>> static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
>> static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
>> -static inline void kasan_record_aux_stack(void *ptr) {}
>> -
>> -#endif /* CONFIG_KASAN_GENERIC */
>> +#endif /* CONFIG_KASAN_GENERIC || CONFIG_SLAB_QUARANTINE */
>
> In doing this extraction, I wonder if function naming should be changed?
> If it's going to live a new life outside of KASAN proper, maybe call
> these functions quarantine_cache_*()? But perhaps that's too much
> churn...
These functions are kasan handlers that are called by allocator.
I.e. allocator calls kasan handlers, and then kasan handlers call
quarantine_put(), quarantine_reduce() and quarantine_remove_cache() among other
things.
Andrey Konovalov wrote:
> If quarantine is to be used without the rest of KASAN, I'd prefer for
> it to be separated from KASAN completely: move to e.g. mm/quarantine.c
> and don't mention KASAN in function/config names.
Hmm, making quarantine completely separate from KASAN would bring troubles.
Currently, in many special places the allocator calls KASAN handlers:
kasan_cache_create()
kasan_slab_free()
kasan_kmalloc_large()
kasan_krealloc()
kasan_slab_alloc()
kasan_kmalloc()
kasan_cache_shrink()
kasan_cache_shutdown()
and some others.
These functions do a lot of interesting things and also work with the quarantine
using these helpers:
quarantine_put()
quarantine_reduce()
quarantine_remove_cache()
Making quarantine completely separate from KASAN would require to move some
internal logic of these KASAN handlers to allocator code.
In this patch I used another approach, that doesn't require changing the API
between allocators and KASAN. I added linux/mm/kasan/slab_quarantine.c with slim
KASAN handlers that implement the minimal functionality needed for quarantine.
Do you think that it's a bad solution?
>> #ifdef CONFIG_KASAN_SW_TAGS
>>
>> diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h
>> index 9eb430c163c2..fc7548f27512 100644
>> --- a/include/linux/slab_def.h
>> +++ b/include/linux/slab_def.h
>> @@ -72,7 +72,7 @@ struct kmem_cache {
>> int obj_offset;
>> #endif /* CONFIG_DEBUG_SLAB */
>>
>> -#ifdef CONFIG_KASAN
>> +#if defined(CONFIG_KASAN) || defined(CONFIG_SLAB_QUARANTINE)
>> struct kasan_cache kasan_info;
>> #endif
>>
>> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
>> index 1be0ed5befa1..71020cee9fd2 100644
>> --- a/include/linux/slub_def.h
>> +++ b/include/linux/slub_def.h
>> @@ -124,7 +124,7 @@ struct kmem_cache {
>> unsigned int *random_seq;
>> #endif
>>
>> -#ifdef CONFIG_KASAN
>> +#if defined(CONFIG_KASAN) || defined(CONFIG_SLAB_QUARANTINE)
>> struct kasan_cache kasan_info;
>> #endif
>>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index d6a0b31b13dc..de5aa061762f 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1931,6 +1931,17 @@ config SLAB_FREELIST_HARDENED
>> sanity-checking than others. This option is most effective with
>> CONFIG_SLUB.
>>
>> +config SLAB_QUARANTINE
>> + bool "Enable slab freelist quarantine"
>> + depends on !KASAN && (SLAB || SLUB)
>> + help
>> + Enable slab freelist quarantine to break heap spraying technique
>> + used for exploiting use-after-free vulnerabilities in the kernel
>> + code. If this feature is enabled, freed allocations are stored
>> + in the quarantine and can't be instantly reallocated and
>> + overwritten by the exploit performing heap spraying.
>> + This feature is a part of KASAN functionality.
>> +
>
> To make this available to distros, I think this needs to be more than
> just a CONFIG. I'd love to see this CONFIG control the availability, but
> have a boot param control a ro-after-init static branch for these
> functions (like is done for init_on_alloc, hardened usercopy, etc). Then
> the branch can be off by default for regular distro users, and more
> cautious folks could enable it with a boot param without having to roll
> their own kernels.
Good point, thanks, added to TODO list.
>> [...]
>> +struct kasan_track {
>> + u32 pid;
>
> pid_t?
Ok, I can change it (here I only moved the current definition of kasan_track).
>> + depot_stack_handle_t stack;
>> +};
>> [...]
>> +#if defined(CONFIG_KASAN_GENERIC) && \
>> + (defined(CONFIG_SLAB) || defined(CONFIG_SLUB)) || \
>> + defined(CONFIG_SLAB_QUARANTINE)
>
> This seems a bit messy. Perhaps an invisible CONFIG to do this logic and
> then the files can test for that? CONFIG_USE_SLAB_QUARANTINE or
> something?
Ok, thanks, I'll try that.
>> [...]
>> + * Heap spraying is an exploitation technique that aims to put controlled
>> + * bytes at a predetermined memory location on the heap. Heap spraying for
>> + * exploiting use-after-free in the Linux kernel relies on the fact that on
>> + * kmalloc(), the slab allocator returns the address of the memory that was
>> + * recently freed. Allocating a kernel object with the same size and
>> + * controlled contents allows overwriting the vulnerable freed object.
>> + *
>> + * If freed allocations are stored in the quarantine, they can't be
>> + * instantly reallocated and overwritten by the exploit performing
>> + * heap spraying.
>
> I would clarify this with the details of what is actually happening:
Ok.
> the allocation isn't _moved_ to a quarantine, yes? It's only marked as not
> available for allocation?
The allocation is put into the quarantine queues, where all allocations wait for
actual freeing.
>> + */
>> +
>> +#include <linux/kasan.h>
>> +#include <linux/bug.h>
>> +#include <linux/slab.h>
>> +#include <linux/mm.h>
>> +#include "../slab.h"
>> +#include "kasan.h"
>> +
>> +void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>> + slab_flags_t *flags)
>> +{
>> + cache->kasan_info.alloc_meta_offset = 0;
>> +
>> + if (cache->flags & SLAB_TYPESAFE_BY_RCU || cache->ctor ||
>> + cache->object_size < sizeof(struct kasan_free_meta)) {
>> + cache->kasan_info.free_meta_offset = *size;
>> + *size += sizeof(struct kasan_free_meta);
>> + BUG_ON(*size > KMALLOC_MAX_SIZE);
>
> Please don't use BUG_ON()[1].
Ok!
> Interesting!
>
> -Kees
>
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on
>
