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 EEA8CC4345F for ; Mon, 29 Apr 2024 16:16:49 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 760CF6B009A; Mon, 29 Apr 2024 12:16:49 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7125B6B009B; Mon, 29 Apr 2024 12:16:49 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5D8E36B009C; Mon, 29 Apr 2024 12:16:49 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 3FF886B009A for ; Mon, 29 Apr 2024 12:16:49 -0400 (EDT) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id EFF60A031A for ; Mon, 29 Apr 2024 16:16:48 +0000 (UTC) X-FDA: 82063072896.24.F24732D Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by imf01.hostedemail.com (Postfix) with ESMTP id 8EFD140010 for ; Mon, 29 Apr 2024 16:16:45 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=clip-os.org header.s=gm1 header.b=P5lBlIyl; spf=pass (imf01.hostedemail.com: domain of nicolas.bouchinet@clip-os.org designates 217.70.183.197 as permitted sender) smtp.mailfrom=nicolas.bouchinet@clip-os.org; dmarc=pass (policy=none) header.from=clip-os.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714407406; a=rsa-sha256; cv=none; b=QmLugwsSdsbkhZBv3B5TuoExby0luBNoHESQUQn22vIQdsEj7XffR0/lsaa2w+w7aRiHV4 UmiyOy/pvY9wZvnpNZVsBVjbt8Nyc3sm+9/sTTl5hdkCnI6VBgWGYpAuqmG0wR2IVZ0P6l wIAsJwP0TBsQ9kUeiZPmV7HIymp/EFI= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=clip-os.org header.s=gm1 header.b=P5lBlIyl; spf=pass (imf01.hostedemail.com: domain of nicolas.bouchinet@clip-os.org designates 217.70.183.197 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=1714407406; 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=tgjaNRLEiT76BlaO5T8jnumXyfuG+m1B71ogzqT5hpI=; b=YdaddJj4iQJAyJEUgvgRhC2FNzzLNHwyLLwXcOlV0YfN52uOmia7WwBLw9IU6HVmZh4qOW Scgl43XvDigmIJvnk7QA5B8lOgt5Z51J5OANkTitvlS2fZWS4NoIp12YX1nd+8nOlyP8Rg HuEA4G/kY/UhIomIBvyh5a6svmpWXW8= Received: by mail.gandi.net (Postfix) with ESMTPSA id 3987C1C0006; Mon, 29 Apr 2024 16:16:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=clip-os.org; s=gm1; t=1714407403; 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=tgjaNRLEiT76BlaO5T8jnumXyfuG+m1B71ogzqT5hpI=; b=P5lBlIylAmV4JmvD5a4rMr3zvpn0GBMngXjPqq69tsEHO/yoOH/wix6rvivyiI+pYc/HEQ pahxz8udA3d/NlvukusIlMKa7cDGNY0IhBefkMpTFSt+7IF/FY7TGN+9iDD3ExjfA5kXqM rpN9LhPIuRZ/PolOtG6xsqVucz3b2q8dNgRfGo+tjATi5pU5rW4wX8fhPbzbziMjGVuQrF y36EsNcx1CAg8Bpri9Ks/7v9Sjlltl52GL95GPh312+fvc3NBc03/ArL6oUkAtD5fHoD/B U7CRrnQTODX5z6IIKauOyyhIRzLWN890BhlgL79c8JdNrorCsJOIUlbdqgeKlg== Message-ID: <10c9a07a-1c6d-4ea7-8c1d-03a7dc5b29d8@clip-os.org> Date: Mon, 29 Apr 2024 18:16:40 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] slub: Fixes freepointer encoding for single free To: Chengming Zhou , Vlastimil Babka , 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> <7074c0b4-62d4-444d-8e59-e23bbbccf9b8@clip-os.org> <83fda406-0340-4b7b-9f02-e9eb41c77f0e@clip-os.org> <73f80886-e390-4320-84dd-68e7cd7e8c62@linux.dev> Content-Language: en-US From: Nicolas Bouchinet In-Reply-To: <73f80886-e390-4320-84dd-68e7cd7e8c62@linux.dev> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-GND-Sasl: nicolas.bouchinet@clip-os.org X-Stat-Signature: psern8qh55cddgatcicokex653i3soj1 X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 8EFD140010 X-HE-Tag: 1714407405-974119 X-HE-Meta: U2FsdGVkX1/KkVHFCHM+XfPmEWEBRqN1LOCmhDtU3SVjQ3JFPxlpSi1KNy/Stgmvj1eCHpYzPZlgKM+ASGVSslSGpDx/8VSK9ZJWRMeUt/zBBlxczf+VcnQ38mXIEWumNxSmGuAay5YU1V6APrAqm1VyjHUdGpbZeXGUCIi9VBkLEv883KPicvf0L0bI9rbGvNH6SbqfrqErt2biGgne40tUFPXDwxBL3mGoTIx8SbQ91WAzpDP3b8zDQlaDn5Tert+Ta4VFSXtuDC3+ZZNoK07iqPORRt3g8xdARjJgnHLI9oSzxqSbBmk+rFbaMmG+k2RkI2G8FSIE1hTHGRJMdZ0V8fierndsE0M2GrqoZyDK86AlfkOOSbrTRklhKtkdTKCBGOYZaROyUv5G9B221OCMijIWX6zdOVTvRJffKM8L8mwPZajcdk6gMVkVn/+UksiNewfBGnHVpGCRBXgOLe0kv2dt8cyoNWJvrpHTR2E4Uo2LScOnt4iPCKpQIEZKp3LaCARsrArgc5qIZ6bxyU/bvJBe/7lM10Vjf9PF2POSxgiLxCg91zQrC6LToVy6Y8Sw2SI7oWgsaelW9/b6ZQMLMkY7KzLs/PrI2ojjT1u+Xa/NjLrOKjE7QmcDyFLtD9CtQL1LBeCZA2VlZcIbiI5wlOI0Y5PbZ+1AXQcC6mfurGMpxWtUsNLgqbkub4+z9LQfe1l3J2nWirNnJpBtdcTn2ELph8Aiy95LBxi0wpsYcuqcVbkawNC+27AYyBQtCbP23IQFE91Y6eTSI7d/wTMkqziTeOxBwaXLVZLM2iiDCR43CCcOBFqApieQv8WCG8her7miesQK4+3mKE9JpYzpFHY5zu+GtTOhqBhJfIUDJWt8gmCbuOyTkx8Ek0JkGqsBr+dGRoPpXGDNZvFJ8Trh6PzsFLhUJuatzKtsXSc34FNZqZy2R8mVSgqQDcpcWS5LNYho+5YeULBNUWX aS1mN+HL gQFvPiWdg5OdR8i+O/UvcK3Otvg== 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 16:52, Chengming Zhou wrote: > On 2024/4/29 22:32, Nicolas Bouchinet wrote: >> On 4/29/24 15:35, Chengming Zhou wrote: >>> On 2024/4/29 20:59, Nicolas Bouchinet wrote: >>>> 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 >>> Ah, right, this ability seems important for debugging overflow problem. >>> >>>>>> 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. >>> Agree. >>> >>>>>> 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. >>>> >>> Good catch! You may need to change maybe_wipe_obj_freeptr() too, >>> I haven't tested this, not sure whether it works for you. :) >>> >>> diff --git a/mm/slub.c b/mm/slub.c >>> index 3e33ff900d35..3f250a167cb5 100644 >>> --- a/mm/slub.c >>> +++ b/mm/slub.c >>> @@ -3796,7 +3796,8 @@ static void *__slab_alloc_node(struct kmem_cache *s, >>>   static __always_inline void maybe_wipe_obj_freeptr(struct kmem_cache *s, >>>                                                     void *obj) >>>   { >>> -       if (unlikely(slab_want_init_on_free(s)) && obj) >>> +       if (unlikely(slab_want_init_on_free(s)) && obj && >>> +           !freeptr_outside_object(s)) >>>                  memset((void *)((char *)kasan_reset_tag(obj) + s->offset), >>>                          0, sizeof(void *)); >>>   } >>> >>> Thanks! >> Indeed since check_object() avoids objects for which freepointer is in the object and since val is equal to SLUB_RED_ACTIVE in our specific case it should work. Do you want me to add you as Co-authored ? >> > Ok, it's great. Thanks! Now I think of it, doesn't it seems a bit odd to only properly init_on_free object's freepointer only if it's inside the object ? IMHO it is equally necessary to avoid information leaking about the freepointer whether it is inside or outside the object. I think it break the semantic of the commit 0f181f9fbea8bc7ea ("mm/slub.c: init_on_free=1 should wipe freelist ptr for bulk allocations") ? Thanks.