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 A1F1EC4345F for ; Mon, 29 Apr 2024 12:59:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 022826B008C; Mon, 29 Apr 2024 08:59:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F12DE6B00A1; Mon, 29 Apr 2024 08:59:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DDB066B0096; Mon, 29 Apr 2024 08:59:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id BFAB56B0087 for ; Mon, 29 Apr 2024 08:59:40 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 6DBA0C06B1 for ; Mon, 29 Apr 2024 12:59:40 +0000 (UTC) X-FDA: 82062576120.10.FE5E050 Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) by imf23.hostedemail.com (Postfix) with ESMTP id 5C839140017 for ; Mon, 29 Apr 2024 12:59:37 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=clip-os.org header.s=gm1 header.b=kBizUQ4V; dmarc=pass (policy=none) header.from=clip-os.org; spf=pass (imf23.hostedemail.com: domain of nicolas.bouchinet@clip-os.org designates 217.70.183.196 as permitted sender) smtp.mailfrom=nicolas.bouchinet@clip-os.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714395577; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=6jSnqT7ikJtsnT7a8wUgvbHGyx8IAD/OHjf9GzKJDVI=; b=1CIz03Ei0bJ3tmoemJPH55qo+B4E3NcoiyLKudmPL/NQ2Zv5zkBmmE436xVOrnXKgxFg3G Ru3jzPyDjxZAC/VI+0fAn4OIcID/7yT3H6x6q29R8Gd8enV/9sRnxZYFbpc1wjtrI3NkD5 iACJdUJn1WXdClXRGbHR9HdddtiG2qE= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=clip-os.org header.s=gm1 header.b=kBizUQ4V; dmarc=pass (policy=none) header.from=clip-os.org; spf=pass (imf23.hostedemail.com: domain of nicolas.bouchinet@clip-os.org designates 217.70.183.196 as permitted sender) smtp.mailfrom=nicolas.bouchinet@clip-os.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714395577; a=rsa-sha256; cv=none; b=utggCL6rwV3SVkSAxwJl2jgxfz4LEo4nLA6XgXKzcH66SGMZSNf70tYTCAG1as/fIW59PE R4RGySFZRjBJUajbHqJQiiJdT0XHTChaq7pKYCRd1MWjaPj6NVQW8MaO/qo6bgazax7GRF P7nPUdBIYxck0RFdbIAQD4AmwKlxb8A= Received: by mail.gandi.net (Postfix) with ESMTPSA id 8A007E0002; Mon, 29 Apr 2024 12:59:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=clip-os.org; s=gm1; t=1714395575; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6jSnqT7ikJtsnT7a8wUgvbHGyx8IAD/OHjf9GzKJDVI=; b=kBizUQ4VJRjisKhAL3KPmoRsGGJvwuoRH6lurnhhl/Y9IznZb5l66ELv7BaHqyiYZCAZYy LtRY8Tw+8WHHt1MaYSlJqj7m6GLPXEop8KvL+2ELrPDGVdPdqRubogODoTY8y9F7gZSeof Kous1US5twXkRihCmm9wC1724TnI7KK5cr3T7avTKBxY6BH1lyV5cpGYdEs1yzCL1Ai8rY ZTmqnoGp1KhNxV/Q5pJd0z+wZTxyOaKeADqhuvPt4iSKtvOb4G14Z9P3xzotEcQP4ujGg8 ibJtf7TO4d2mvw6Y0U+8IrD7ACl7qiMlXUs+Hbmam4KasdMk328oAMbx1XD7yg== Message-ID: <7074c0b4-62d4-444d-8e59-e23bbbccf9b8@clip-os.org> Date: Mon, 29 Apr 2024 14:59:32 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] slub: Fixes freepointer encoding for single free From: Nicolas Bouchinet To: Vlastimil Babka , 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, roman.gushchin@linux.dev, 42.hyeyoo@gmail.com, Xiongwei Song References: <5c34b253-b476-494a-96c9-fe3c95b9b74d@linux.dev> <6f977874-2a18-44ef-b207-9eb0baad9d66@suse.cz> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-GND-Sasl: nicolas.bouchinet@clip-os.org X-Rspam-User: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 5C839140017 X-Stat-Signature: ixs3s9k5dwqaaggo4qh4qnme48yw5b1t X-HE-Tag: 1714395577-171371 X-HE-Meta: U2FsdGVkX1+xzmBqG/OWYICpVuNv7X9F9cjxuh0dtyM9SJwILXlczW4U33T+o/NKtpRLPeAbEHVNBwaVJW6GWI08FlxkQ2JPLxsjMUtlxt+NS/9Zk6DdlJIVfcJFGUHe0goysEwgCpoL0W25peHA5L9wOJemhSn5ueDeFOef48K05BltFCxuwivvWkBJsCf/B+qQoO53KnKiwxwV9UZ/sRlGP/QZgy23MSTGsV4RuvJHwCS9TNLjgxnvPj52ssQPZpFKCidjlNnClperbRgnGsVnPfSiJ1pXW6KQpJ80lk+AOMP7C38oMGDcdiuwh+THDsYmOq8IA05/GugZXaoVJcvpADPT6B36GMVaiy9eDo576WBziy4i8QaWBTp5obXHvCn0O65wiv/VA+nKXGn/kfgA4EgrTydUK0ZGOqyI7xJ2Trz2K92t7dDBb13+pAP4rWepoloO/JJP0AuHprUAMpQaH+UrB09WgG278lI/OQ2wRzYYxQmVc3QZeD3GgxHRoBv0PWAczkJF1hqvFh30YppFiQyRXNECBHYIsj2/vdcq0sE50WjSYg3NyicvbkAlYNFH4BFLCrsaVRrn70kUIRA43ZR+D3Sxb6WyJSIRnfAkBIMGAhIMwPpaSHrfgQ4+vA3qUbF/+N0xIM8NNjr1tP0teyk4A86jnLyrU7wUNGuvYzhtLzmElGNDzlFx4PZppSlVP8fCy/uKfm8RsF9Rbw/z+qbF4y2nO5iwPSEga2/0xqRwfDRFVFtcni3j+EtVCZlpYWlV9Nvd1iwmAI6mc6Y8YnnkGgX+Se7LNW9Z+C4m2M9u01PsmDDdBX5Ul1Zt+NL4TJOZrtTKa7GdYJpMLTh8S4zi2W3mz0PRmy6KhhMwpYQ0Ihol1X+6i6pxGi1duHdTYZ/hPlfiFMNg5mHyP5lTiH1ieNVX/oYmMaywRXkCeI3OatZaX7o6R4PzAUKqDA0VZKoHRzTWq1fkHdd i5V7oqPr nWtfHM9L9kOEO/Xa8B14CPuJ6klzKccnDJycc8f5mWPXbjfY3DJisGo8/PH8YqPiZ54Jk69ERmLzsYXh4FhjcMpkfESrfulzpGveUeEXg+kIoHWmz8vgxFCnwCQ== 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: On 4/29/24 11:09, Nicolas Bouchinet wrote: > Hi Vlastimil, > > thanks for your review and your proposal. > > On 4/29/24 10:52, Vlastimil Babka wrote: >> On 4/25/24 5:14 PM, Chengming Zhou wrote: >>> On 2024/4/25 23:02, Nicolas Bouchinet wrote: >> Thanks for finding the bug and the fix! >> >>>> 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. >>>>> y not suitable for a stable-candidate fix we need >>>>>> 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. >>> Thank you for the reference! I also don't get why it needs to zero >>> the metadata and tracking information. >> Hmm taking a step back, it seems really suboptimal to initialize the >> outside-object freepointer as part of init_on_free: >> >> - the freeing itself will always set it one way or another, in this case >> free_to_partial_list() will do set_freepointer() after >> free_debug_processing() >> >> - we lose the ability to detect if the allocated slab object's user >> wrote to >> it, which is a buffer overflow >> >> So the best option to me would be to adjust the init in >> slab_free_hook() to >> avoid the outside-object freepointer similarly to how it avoids the >> red zone. >> >> We'll still not have the buffer overflow detection ability for bulk free >> where slab_free_freelist_hook() will set the free pointer before we >> reach >> the checks, but changing that is most likely not worth the trouble, and >> especially not suitable for a stable-candidate fix we need here. > > It seems like a good alternative to me, I'll push a V2 patch with > those changes. > > I help maintaining the Linux-Hardened patchset in which we have a slab > object canary feature that helps detecting overflows. It is located > just after the object freepointer. I've tried a patch where the freepointer is avoided but it results in the same bug. It seems that the commit 0f181f9fbea8bc7ea ("mm/slub.c: init_on_free=1 should wipe freelist ptr for bulk allocations") inits the freepointer on allocation if init_on_free is set in order to return a clean initialized object to the caller. > >> >>>> The patch could also be optimized a bit by restricting >>>> set_freepointer() call to the `CONFIG_SLAB_FREELIST_HARDENED` >>>> option value. >>>> >>> Yeah. Maybe memcg_alloc_abort_single() needs this too. >>> >>> Thanks. >>> >>>> 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 > Thanks again for your review, > > Nicolas >