Re: [PATCH 01/12] mm, slab: ignore hardened usercopy parameters when disabled

[Vlastimil Babka]

On 11/21/22 22:35, Kees Cook wrote:
> On November 21, 2022 9:11:51 AM PST, Vlastimil Babka <vbabka@xxxxxxx> wrote:
>>With CONFIG_HARDENED_USERCOPY not enabled, there are no
>>__check_heap_object() checks happening that would use the kmem_cache
>>useroffset and usersize fields. Yet the fields are still initialized,
>>preventing merging of otherwise compatible caches. Thus ignore the
>>values passed to cache creation and leave them zero when
>>CONFIG_HARDENED_USERCOPY is disabled.
>>
>>In a quick virtme boot test, this has reduced the number of caches in
>>/proc/slabinfo from 131 to 111.
>>
>>Cc: Kees Cook <keescook@xxxxxxxxxxxx>
>>Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
>>---
>> mm/slab_common.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>>diff --git a/mm/slab_common.c b/mm/slab_common.c
>>index 0042fb2730d1..a8cb5de255fc 100644
>>--- a/mm/slab_common.c
>>+++ b/mm/slab_common.c
>>@@ -317,7 +317,8 @@ kmem_cache_create_usercopy(const char *name,
>> 	flags &= CACHE_CREATE_MASK;
>> 
>> 	/* Fail closed on bad usersize of useroffset values. */
>>-	if (WARN_ON(!usersize && useroffset) ||
>>+	if (!IS_ENABLED(CONFIG_HARDENED_USERCOPY) ||
>>+	    WARN_ON(!usersize && useroffset) ||
>> 	    WARN_ON(size < usersize || size - usersize < useroffset))
>> 		usersize = useroffset = 0;
>> 
>>@@ -640,6 +641,9 @@ void __init create_boot_cache(struct kmem_cache *s, const char *name,
>> 		align = max(align, size);
>> 	s->align = calculate_alignment(flags, align, size);
>> 
>>+	if (!IS_ENABLED(CONFIG_HARDENED_USERCOPY))
>>+		useroffset = usersize = 0;
>>+
>> 	s->useroffset = useroffset;
>> 	s->usersize = usersize;
>> 
> 
> "Always non-mergeable" is intentional here, but I do see the argument
> for not doing it under hardened-usercopy.
> 
> That said, if you keep this part, maybe go the full step and ifdef away
> useroffset/usersize's struct member definition and other logic, especially
> for SLUB_TINY benefits, so 2 ulongs are dropped from the cache struct?

Okay, probably won't make much difference in practice, but for consistency...
----8<----