Re: [PATCH RFC v2] Randomized slab caches for kmalloc()

[GONG, Ruiqi]

Sorry for the late reply. I was trapped by other in-house kernel issues
these days.

On 2023/05/24 13:54, Hyeonggon Yoo wrote:
> On Mon, May 22, 2023 at 04:58:25PM +0800, GONG, Ruiqi wrote:
>>
>>
>> On 2023/05/22 16:03, Hyeonggon Yoo wrote:
>>> On Mon, May 22, 2023 at 4:35 PM Gong Ruiqi <gongruiqi1@xxxxxxxxxx> wrote:
>>>> On 2023/05/17 6:35, Hyeonggon Yoo wrote:
>>> [...]
>>>>>>>> +#ifdef CONFIG_RANDOM_KMALLOC_CACHES
>>>>>>>> +# define SLAB_RANDOMSLAB       ((slab_flags_t __force)0x01000000U)
>>>>>>>> +#else
>>>>>>>> +# define SLAB_RANDOMSLAB       0
>>>>>>>> +#endif
>>>>>
>>>>> There is already the SLAB_KMALLOC flag that indicates if a cache is a
>>>>> kmalloc cache. I think that would be enough for preventing merging
>>>>> kmalloc caches?
>>>>
>>>> After digging into the code of slab merging (e.g. slab_unmergeable(),
>>>> find_mergeable(), SLAB_NEVER_MERGE, SLAB_MERGE_SAME etc), I haven't
>>>> found an existing mechanism that prevents normal kmalloc caches with
>>>> SLAB_KMALLOC from being merged with other slab caches. Maybe I missed
>>>> something?
>>>>
>>>> While SLAB_RANDOMSLAB, unlike SLAB_KMALLOC, is added into
>>>> SLAB_NEVER_MERGE, which explicitly indicates the no-merge policy.
>>>
>>> I mean, why not make slab_unmergable()/find_mergeable() not to merge kmalloc
>>> caches when CONFIG_RANDOM_KMALLOC_CACHES is enabled, instead of a new flag?
>>>
>>> Something like this:
>>>
>>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>>> index 607249785c07..13ac08e3e6a0 100644
>>> --- a/mm/slab_common.c
>>> +++ b/mm/slab_common.c
>>> @@ -140,6 +140,9 @@ int slab_unmergeable(struct kmem_cache *s)
>>>   if (slab_nomerge || (s->flags & SLAB_NEVER_MERGE))
>>>   return 1;
>>>
>>> + if (IS_ENALBED(CONFIG_RANDOM_KMALLOC_CACHES) && (flags & SLAB_KMALLOC))
>>> + return 1;
>>> +
>>>   if (s->ctor)
>>>   return 1;
>>>
>>> @@ -176,6 +179,9 @@ struct kmem_cache *find_mergeable(unsigned int
>>> size, unsigned int align,
>>>   if (flags & SLAB_NEVER_MERGE)
>>>   return NULL;
>>>
>>> + if (IS_ENALBED(CONFIG_RANDOM_KMALLOC_CACHES) && (flags & SLAB_KMALLOC))
>>> + return NULL;
>>> +
>>>   list_for_each_entry_reverse(s, &slab_caches, list) {
>>>   if (slab_unmergeable(s))
>>>   continue;
>>
>> Ah I see. My concern is that it would affect not only normal kmalloc
>> caches, but kmalloc_{dma,cgroup,rcl} as well: since they were all marked
>> with SLAB_KMALLOC when being created, this code could potentially change
>> their mergeablity. I think it's better not to influence those irrelevant
>> caches.
> 
> I see. no problem at all as we're not running out of cache flags.
> 
> By the way, is there any reason to only randomize normal caches
> and not dma/cgroup/rcl caches?

The reason is mainly because based on my knowledge they are not commonly
used for exploiting the kernel, i.e. they are not on the "attack
surface", so it's unnecessary to do so. I'm not sure if other hardening
experts have different opinions on that.

> 
> Thanks,
>