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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <206E0154-63A6-45CF-8E19-BD01B35AEF0B@kernel.org>
Date:   Mon, 21 Nov 2022 13:35:30 -0800
From:   Kees Cook <kees@...nel.org>
To:     Vlastimil Babka <vbabka@...e.cz>, Christoph Lameter <cl@...ux.com>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Pekka Enberg <penberg@...nel.org>
CC:     Hyeonggon Yoo <42.hyeyoo@...il.com>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Matthew Wilcox <willy@...radead.org>, patches@...ts.linux.dev,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH 01/12] mm, slab: ignore hardened usercopy parameters when disabled

On November 21, 2022 9:11:51 AM PST, Vlastimil Babka <vbabka@...e.cz> 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@...omium.org>
>Signed-off-by: Vlastimil Babka <vbabka@...e.cz>
>---
> 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?

-Kees


-- 
Kees Cook