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 D166AC4345F for ; Mon, 29 Apr 2024 13:35:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 53A8D6B0093; Mon, 29 Apr 2024 09:35:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4EA566B0095; Mon, 29 Apr 2024 09:35:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3FFD16B0098; Mon, 29 Apr 2024 09:35:53 -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 23C896B0093 for ; Mon, 29 Apr 2024 09:35:53 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id A47E81604E0 for ; Mon, 29 Apr 2024 13:35:52 +0000 (UTC) X-FDA: 82062667344.03.E2A2BAD Received: from out-176.mta0.migadu.com (out-176.mta0.migadu.com [91.218.175.176]) by imf16.hostedemail.com (Postfix) with ESMTP id ABB67180024 for ; Mon, 29 Apr 2024 13:35:49 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=kjVwLeMQ; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf16.hostedemail.com: domain of chengming.zhou@linux.dev designates 91.218.175.176 as permitted sender) smtp.mailfrom=chengming.zhou@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714397750; 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=CpibBkIcCucywmvveltGrcNm6gkhi3AFYlJMmTa/ji8=; b=WBAfFE9iks/pmB8reyRQrnLTg8wI+wg/XIV8YyfKJ8sLHKMR+HmoGMUooCdgNedNPTxbU6 mqeM8S6SZBnQEur8zefo9pal3VrjtS/qRhQ86ObaXLPk5tgMk5WUlWMUeRRhF0cKdo9Sjb 7sUVxW0j+86divgrHsy7DDL1z0/DdDs= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=kjVwLeMQ; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf16.hostedemail.com: domain of chengming.zhou@linux.dev designates 91.218.175.176 as permitted sender) smtp.mailfrom=chengming.zhou@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714397750; a=rsa-sha256; cv=none; b=tbyuwgolrhL4C0pRX9yO7IRVmOqVd6iaudrVIMSrfg0d+fRmWBTinqf3KaWc1WTJ8PKphn qYwvqNN8wLhjEx0ya7Ytrjyx+WBQghf780PS4sIIPqb8iVaVR12ZlOc62kZioC6X1sh2F6 1FN3n795IxnRihhvMTSed9oU6DY3/mM= Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1714397747; 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=CpibBkIcCucywmvveltGrcNm6gkhi3AFYlJMmTa/ji8=; b=kjVwLeMQiLMkGlNGJd0b5ixNqnjqiUNPnOYnjL8yxl1OSm36So93zAYloGs8NUj4+OeFdb sPHbtVepDk3g22o8wgOOfXoQ0G8HWE2iiA+sIkkXLvp/ZsS+6MOt9fFCLi2mzFBR1IX8ER UV+HQ4cE5AiYyLAN4OdhJ5nbnpzxFzU= Date: Mon, 29 Apr 2024 21:35:34 +0800 MIME-Version: 1.0 Subject: Re: [PATCH] slub: Fixes freepointer encoding for single free To: Nicolas Bouchinet , 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> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Chengming Zhou In-Reply-To: <7074c0b4-62d4-444d-8e59-e23bbbccf9b8@clip-os.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Rspam-User: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: ABB67180024 X-Stat-Signature: 8xbu7or98gy5mr4dsur7pdsrkqxyqkgi X-HE-Tag: 1714397749-735365 X-HE-Meta: U2FsdGVkX1/7NB6RuEdRusK9jmIZfX7k6J9ZRGUOrVHy1LBm8GGzhirPKSGLy/VGtIh4mL2S6XK9Py0gdfxzaTcPTFd+OVaxBfN4ON4ttByX9tl3886pVgvnBTLqwJ/RVdlXpBycRo93rYO53go6irDjJTmeOrIKJk6Z3nPxuXIKxmZln7IaDrk2a74Nn3AdCP2BQ/l4CnPQby8wLzbuZM3iWMzOp3Ck2VFE7PEEknHvLY2b1fK769pqIsr5ObgN/k72THwlixQp4wYcaklkTmfxfcMCyrtlYixmVrG5g5l6urNqZOIWsB3hkPMVCMvPW09w3f9bAntsFoTsQeAnOoWZeUdiGYRgl33dzFWIVDKpxLTxVobN9zqskb4VaQ8BqP2xz7GR72Du4/y1jKGfysOQZvWralhBDp3wejLr4r5tm2eEDYo5e/2OE/Bxl7qH0wPGr4d/zZFhxgDb9xxehN7JC2DQ5j2ipB9QOC+Wr+SOLXSMwAYq1nLGM7P/T4YCnR2TSHTw/SzsSRO+WN6HCzPSciVig/naypOBf1JWykWSkFMuLivH8jxHDx2MEzKzk8MIMWq2K5OB4DsBENG64feHuQ00x7/wg8FBmkROweALZfR7TdY1xthyLPUXdEiBAmviK2+dC/050nmVnGuQyGFCRVFeQTuzvmABELVMSrYxxcOxsXeHZyUqMvhnOAtTitRUlva3ZiVw1NnR6bVXTotZCFECVvNsivnn6qGN2iuD3+b0V76EeEfL7jEirDuqerB2Nh6GMhnQswL8RUwoiE0WlSdNWQz4HXvdZ1SRfYwO17/I9TVB1O3sni2fkWlbNmmyzoYQbJFBWC9wrkKTvCzFVzQahz8lZaBSXcFP5bI3TGqMJgZJMg== 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 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!