On Mon, May 22, 2023 at 4:35 PM Gong Ruiqi <[email protected]> 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;
On 2023/05/22 16:03, Hyeonggon Yoo wrote:
> On Mon, May 22, 2023 at 4:35 PM Gong Ruiqi <[email protected]> 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.
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 <[email protected]> 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?
Thanks,
--
Hyeonggon Yoo
Doing kernel stuff as a hobby
Undergraduate | Chungnam National University
Dept. Computer Science & Engineering
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 <[email protected]> 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,
>