From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7E561C4345F for ; Thu, 25 Apr 2024 15:02:13 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E45666B009A; Thu, 25 Apr 2024 11:02:12 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DF50D6B009B; Thu, 25 Apr 2024 11:02:12 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CBC846B009C; Thu, 25 Apr 2024 11:02:12 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id A8CA26B009A for ; Thu, 25 Apr 2024 11:02:12 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 4B75E1212D6 for ; Thu, 25 Apr 2024 15:02:12 +0000 (UTC) X-FDA: 82048369704.26.A5552DD Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by imf14.hostedemail.com (Postfix) with ESMTP id 56CB4100008 for ; Thu, 25 Apr 2024 15:02:08 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=clip-os.org header.s=gm1 header.b=CBDRgnS3; spf=pass (imf14.hostedemail.com: domain of nicolas.bouchinet@clip-os.org designates 217.70.183.194 as permitted sender) smtp.mailfrom=nicolas.bouchinet@clip-os.org; dmarc=pass (policy=none) header.from=clip-os.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714057328; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=bCxNDQFbVX2RzXi6RDg984PmKersP4JqNmIp/LrFKKk=; b=K7xn+nbf0i1N7tiJH8zX4QackVscir5WDOZ31UDrgnmciyGaBEMoKDSQXFgTSzM15Wje0V SgBq1gc99y/r1d2Pz1zA3dPYhIE2q0kqTJ3K0i+BYzolLzrkjEJuxjj63LNQbZXpZKhYe8 NuBIu/uXIWoiOxNaf1LGldeW8cQ342s= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714057328; a=rsa-sha256; cv=none; b=mhYQPL/r64vuFpPwfFaG0PIT0WTJdB35Ss741YRSgCAMyCtteTu9VxvU/taltwM6ZpmUH/ ZlPZm3lr5+DK/WXWKtWN3fsRGHe5YVU0exoDFf0EHDKsdpzcs3lbp2pPE/Z5dSNkWaFDDZ oquqPJc9X9+g07ria6FE8VUkfrDMkVM= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=clip-os.org header.s=gm1 header.b=CBDRgnS3; spf=pass (imf14.hostedemail.com: domain of nicolas.bouchinet@clip-os.org designates 217.70.183.194 as permitted sender) smtp.mailfrom=nicolas.bouchinet@clip-os.org; dmarc=pass (policy=none) header.from=clip-os.org Received: by mail.gandi.net (Postfix) with ESMTPSA id 32B9A40006; Thu, 25 Apr 2024 15:02:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=clip-os.org; s=gm1; t=1714057324; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=bCxNDQFbVX2RzXi6RDg984PmKersP4JqNmIp/LrFKKk=; b=CBDRgnS3cvKmonuihs2QLWxhS6NlfYrrujWjG3ZsTBfxaFhl+skCzzKPhKyqqK6ktJrAKx B+9qtYDSGWv8/bvKa7cipza/7TS/0YuMCTsCH3Aw4K2QjeoYBCXAubfvddnSZVfWjY8ubq Re3laaUtCNI86U1pi1BJk6beZlKMPJXG7u9uiGlf+zXc5r1mTCwN5Yj/fnLK41nV/0JRmo cW5OazHUz0F/6nRUyeTzlGvoS1CtNk4R86G3TjyC/9XuB32mW58dAAxvdt5VAqVtITSihg Degu0LWF5xEuQWKrzPf11o2DWzlkc6Yhy0ngsFz1GBlhpPjjxa4l/EM4nD2rTQ== Content-Type: multipart/alternative; boundary="------------PpeWkQ5vSUxxq0iUR2A5OXEA" Message-ID: Date: Thu, 25 Apr 2024 17:02:01 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] slub: Fixes freepointer encoding for single free To: Chengming Zhou , linux-mm@kvack.org, linux-kernel@vger.kernel.org Cc: cl@linux.com, penberg@kernel.org, rientjes@google.com, iamjoonsoo.kim@lge.com, akpm@linux-foundation.org, vbabka@suse.cz, roman.gushchin@linux.dev, 42.hyeyoo@gmail.com References: Content-Language: en-US From: Nicolas Bouchinet In-Reply-To: X-GND-Sasl: nicolas.bouchinet@clip-os.org X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 56CB4100008 X-Stat-Signature: um8kts9demcj3idomcqqgaz1ntpkyzaq X-Rspam-User: X-HE-Tag: 1714057328-468878 X-HE-Meta: U2FsdGVkX19LkDJoWaUOhFdSmIEe+cegax9jQRID0XvKitvovToH4j85LsECvn8HIxjkN5wxWzyYeO9I3HP7dvUkWvZxDtc10L9LwKcq1PQjy17xFYRdSlZNZTZHxfEDrWeG39g+bdo3UnVa5C7pXumTR8zML4kDtUmGAId0wKk+GyICJS/fI37l/4+6Hg2A6g3CqTiCPy+9AZE6Y5UlHWLWa9s2+5qA617xPCLQsB5exLtJq3zOqa0AjBZe6/6SWuS/t1iWUj3y4O6a2v/2j7M7m0QNWKK65zPsNJ/rQhIRK3tZ3bk+CNM1uQ4J+upTT1ii7X60xWnP66zqAHGR47AxlDKiL3rQub8+dk/Wde/qkDoe2cmruLr7upOvkWlaPLuHgsQHydnJrQD1/eToDT22MiwhlsQ0xnPHfOWfnxunnXjKxuS8A4Kuw5A3LIzQXXwHq34ShAQ4lappXCQzzTcCAcbDuhMv1A4g2Q/5epizVujw0YwnhKCfLO6BNGg7ubLA7oiiyYcI4E1Ps0/p1g/tVbFdf40tFhTM1Rczt+1L4ux2KNJxS2BO3GXJnuXmBdNeTTz2W4ghxK2R4T5mf+ixolmh/i611IUB0hva1b0HvoXUwoUHWquuZPmghbnupHMcZZnZLqtXUd+rBFUiukr3nY9mvI0u1rSlj1hI3yNp4sWY8/SkrViOPYouzX8W+bcQvv2kVC5pHush8IrwMhlokGL4jkKPk053SmSGlygOLW2ny0047VZP+cRpjYd3PV3xEYZhrC9JmjTQF4Ko3lg1tjLWAGQxBbcYM6aJ/sPp9SKZJSkT1pN+z+2qTMvd0RBEeIl7Ojq+KBlg9j0hKVuO3O9hl1tl8aJazAKZ9JKQpv46wp079Afjyt84HhPnTJa8QKE+k512D6jnUaZY5AwyCOEKCPCsKEurDOk5Q4rboIIOObhEvOeeWBHOXkLl7NrJvhJrIk4pVn1LMtJ jB6M8QYL bl/ZasW4Lpf7ZBrwWFUKfcknpwBjeC1+HJbqekoJG9hhTSa1ypWPuTx1FvlY+DGgDg2/JsYNY2nV96NTwrM3vVwomL/GiOKYVe3rQfeGLGIyjmdE+mLfe7RP51xNAs+MYIc60hzMNK3Sb+yjI3XJtJfm8ch5BrXL3uAVeCyqwSAGiHufVAiqLD0zct1aSZxGsSzMe0ofoABllRxU6Z5b/fPcsdMTHULmJJKSQs8m0UQMz8gw7IX14tPSw3yZoP62jgP0fiKi7yBoOc5skQV2JM/tyTV0miTwjhn0+fifw1Feoz8dKSqg0gXumHGT4CjKUE2rSo73ztxMgdu8ielxkj2Q8FcFDCMrNvlYQIRu5ZfII+3ILaGu4cvhpyM8Ghfl5rCSufpKYxlUFvYIHJW4g2jYhrzYo7GGnHAUT X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: This is a multi-part message in MIME format. --------------PpeWkQ5vSUxxq0iUR2A5OXEA Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hy, First of all, thanks a lot for your time. On 4/25/24 10:36, Chengming Zhou wrote: > On 2024/4/24 20:47, Nicolas Bouchinet wrote: >> From: Nicolas Bouchinet >> >> Commit 284f17ac13fe ("mm/slub: handle bulk and single object freeing >> separately") splits single and bulk object freeing in two functions >> slab_free() and slab_free_bulk() which leads slab_free() to call >> slab_free_hook() directly instead of slab_free_freelist_hook(). > Right. > >> If `init_on_free` is set, slab_free_hook() zeroes the object. >> Afterward, if `slub_debug=F` and `CONFIG_SLAB_FREELIST_HARDENED` are >> set, the do_slab_free() slowpath executes freelist consistency >> checks and try to decode a zeroed freepointer which leads to a >> "Freepointer corrupt" detection in check_object(). > IIUC, the "freepointer" can be checked on the free path only when > it's outside the object memory. Here slab_free_hook() zeroed the > freepointer and caused the problem. > > But why we should zero the memory outside the object_size? It seems > more reasonable to only zero the object_size when init_on_free is set? The original purpose was to avoid leaking information through the object and its metadata / tracking information as described in init_on_free initial Commit 6471384af2a6 ("mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options"). I have to admit I didn't read the entire lore about the original patchset yet, though it could be interesting to know a bit more the threat models, specifically regarding the object metadata init. The patch could also be optimized a bit by restricting set_freepointer() call to the `CONFIG_SLAB_FREELIST_HARDENED` option value. Thanks again, Nicolas > > Thanks. > >> Object's freepointer thus needs to be properly set using >> set_freepointer() after init_on_free. >> >> To reproduce, set `slub_debug=FU init_on_free=1 log_level=7` on the >> command line of a kernel build with `CONFIG_SLAB_FREELIST_HARDENED=y`. >> >> dmesg sample log: >> [ 10.708715] ============================================================================= >> [ 10.710323] BUG kmalloc-rnd-05-32 (Tainted: G B T ): Freepointer corrupt >> [ 10.712695] ----------------------------------------------------------------------------- >> [ 10.712695] >> [ 10.712695] Slab 0xffffd8bdc400d580 objects=32 used=4 fp=0xffff9d9a80356f80 flags=0x200000000000a00(workingset|slab|node=0|zone=2) >> [ 10.716698] Object 0xffff9d9a80356600 @offset=1536 fp=0x7ee4f480ce0ecd7c >> [ 10.716698] >> [ 10.716698] Bytes b4 ffff9d9a803565f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> [ 10.720703] Object ffff9d9a80356600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> [ 10.720703] Object ffff9d9a80356610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> [ 10.724696] Padding ffff9d9a8035666c: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> [ 10.724696] Padding ffff9d9a8035667c: 00 00 00 00 .... >> [ 10.724696] FIX kmalloc-rnd-05-32: Object at 0xffff9d9a80356600 not freed >> >> Signed-off-by: Nicolas Bouchinet >> --- >> mm/slub.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/mm/slub.c b/mm/slub.c >> index 3aa12b9b323d9..71dbff9ad8f17 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -4342,10 +4342,16 @@ static __fastpath_inline >> void slab_free(struct kmem_cache *s, struct slab *slab, void *object, >> unsigned long addr) >> { >> + bool init = false; >> + >> memcg_slab_free_hook(s, slab, &object, 1); >> + init = slab_want_init_on_free(s); >> >> - if (likely(slab_free_hook(s, object, slab_want_init_on_free(s)))) >> + if (likely(slab_free_hook(s, object, init))) { >> + if (init) >> + set_freepointer(s, object, NULL); >> do_slab_free(s, slab, object, object, 1, addr); >> + } >> } >> >> static __fastpath_inline --------------PpeWkQ5vSUxxq0iUR2A5OXEA Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hy,

First of all, thanks a lot for your time.

On 4/25/24 10:36, Chengming Zhou wrote:
On 2024/4/24 20:47, Nicolas Bouchinet wrote:

From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

Commit 284f17ac13fe ("mm/slub: handle bulk and single object freeing
separately") splits single and bulk object freeing in two functions
slab_free() and slab_free_bulk() which leads slab_free() to call
slab_free_hook() directly instead of slab_free_freelist_hook().

Right.


If `init_on_free` is set, slab_free_hook() zeroes the object.
Afterward, if `slub_debug=F` and `CONFIG_SLAB_FREELIST_HARDENED` are
set, the do_slab_free() slowpath executes freelist consistency
checks and try to decode a zeroed freepointer which leads to a
"Freepointer corrupt" detection in check_object().

IIUC, the "freepointer" can be checked on the free path only when
it's outside the object memory. Here slab_free_hook() zeroed the
freepointer and caused the problem.

But why we should zero the memory outside the object_size? It seems
more reasonable to only zero the object_size when init_on_free is set?

The original purpose was to avoid leaking information through the object and its metadata / tracking information as described in init_on_free initial Commit 6471384af2a6 ("mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options").

I have to admit I didn't read the entire lore about the original patchset yet, though it could be interesting to know a bit more the threat models, specifically regarding the object metadata init.

The patch could also be optimized a bit by restricting set_freepointer() call to the `CONFIG_SLAB_FREELIST_HARDENED` option value.

Thanks again, Nicolas 


Thanks.


Object's freepointer thus needs to be properly set using
set_freepointer() after init_on_free.

To reproduce, set `slub_debug=FU init_on_free=1 log_level=7` on the
command line of a kernel build with `CONFIG_SLAB_FREELIST_HARDENED=y`.

dmesg sample log:
[   10.708715] =============================================================================
[   10.710323] BUG kmalloc-rnd-05-32 (Tainted: G    B           T ): Freepointer corrupt
[   10.712695] -----------------------------------------------------------------------------
[   10.712695]
[   10.712695] Slab 0xffffd8bdc400d580 objects=32 used=4 fp=0xffff9d9a80356f80 flags=0x200000000000a00(workingset|slab|node=0|zone=2)
[   10.716698] Object 0xffff9d9a80356600 @offset=1536 fp=0x7ee4f480ce0ecd7c
[   10.716698]
[   10.716698] Bytes b4 ffff9d9a803565f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[   10.720703] Object   ffff9d9a80356600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[   10.720703] Object   ffff9d9a80356610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[   10.724696] Padding  ffff9d9a8035666c: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[   10.724696] Padding  ffff9d9a8035667c: 00 00 00 00                                      ....
[   10.724696] FIX kmalloc-rnd-05-32: Object at 0xffff9d9a80356600 not freed

Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
---
 mm/slub.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/slub.c b/mm/slub.c
index 3aa12b9b323d9..71dbff9ad8f17 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4342,10 +4342,16 @@ static __fastpath_inline
 void slab_free(struct kmem_cache *s, struct slab *slab, void *object,
 	       unsigned long addr)
 {
+	bool init = false;
+
 	memcg_slab_free_hook(s, slab, &object, 1);
+	init = slab_want_init_on_free(s);
 
-	if (likely(slab_free_hook(s, object, slab_want_init_on_free(s))))
+	if (likely(slab_free_hook(s, object, init))) {
+		if (init)
+			set_freepointer(s, object, NULL);
 		do_slab_free(s, slab, object, object, 1, addr);
+	}
 }
 
 static __fastpath_inline


    

  


--------------PpeWkQ5vSUxxq0iUR2A5OXEA--