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 9082FC4345F for ; Mon, 29 Apr 2024 14:52:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 06B716B008C; Mon, 29 Apr 2024 10:52:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 01BD46B0099; Mon, 29 Apr 2024 10:52:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E26B66B009B; Mon, 29 Apr 2024 10:52:54 -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 C4CFE6B008C for ; Mon, 29 Apr 2024 10:52:54 -0400 (EDT) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 68A6C80252 for ; Mon, 29 Apr 2024 14:52:54 +0000 (UTC) X-FDA: 82062861468.02.7C53A2B Received: from out-170.mta1.migadu.com (out-170.mta1.migadu.com [95.215.58.170]) by imf03.hostedemail.com (Postfix) with ESMTP id 4CF6020006 for ; Mon, 29 Apr 2024 14:52:52 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b="xqp4Fc/V"; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf03.hostedemail.com: domain of chengming.zhou@linux.dev designates 95.215.58.170 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=1714402372; 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=Yw4ulu8ftgPuJpsyuQp9BIsUJ4D+yOSy5kuEcyNjLxM=; b=FbjP8rSGwKp0t/X4hKXCj8vJpDrjfWbAh5uhG5uXApug7DDw1H7XBGUMFhwppr7TNdlXqJ yQ+fazdwsgxsLW8fz93i188Ohp6VPz6RaTshRdtadI7BDaG2tAsWGxtE35KTNYKezSBns4 NL0Sx5NMkpNLtFJqC2YDTMkREf2GnO0= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b="xqp4Fc/V"; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf03.hostedemail.com: domain of chengming.zhou@linux.dev designates 95.215.58.170 as permitted sender) smtp.mailfrom=chengming.zhou@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714402372; a=rsa-sha256; cv=none; b=jenoBsXZ9iQxx42FAwpkVdEqLxl0IMqed8jhcGh9OhibktRu6YvAtR0hQO3FybSmtAqon5 LCyUS4CuY3g6yUhaDw9W1V5cESjZM2mrif4suBC0ccuGO1elyhP9avAFMfByEirJ4G3PHB /jfpQ1eNaBEw3jmOZjAQ/rxfEuXIiPs= Message-ID: <73f80886-e390-4320-84dd-68e7cd7e8c62@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1714402369; 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=Yw4ulu8ftgPuJpsyuQp9BIsUJ4D+yOSy5kuEcyNjLxM=; b=xqp4Fc/Vpi2QqcQsslE+BHBPozhJ7LqljB9QKZcHGFaQDOKwnQmcrKfUKSNPnK5ojF5Ktt ub4/gwd0tjcY4XdM5rxpkIZOQwSMk95yXRr5igSchCvJYpyYiH6/yfebE1WSCbwC33M03b TUAlVRSvkDPVvKAtqhxo+CJA1lBWSuU= Date: Mon, 29 Apr 2024 22:52:42 +0800 MIME-Version: 1.0 Subject: Re: [PATCH] slub: Fixes freepointer encoding for single free Content-Language: en-US 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> <83fda406-0340-4b7b-9f02-e9eb41c77f0e@clip-os.org> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Chengming Zhou In-Reply-To: <83fda406-0340-4b7b-9f02-e9eb41c77f0e@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: 4CF6020006 X-Stat-Signature: sbrrsitomo8g7smwo636mqet19jqtyd3 X-HE-Tag: 1714402372-551466 X-HE-Meta: U2FsdGVkX1/hZZPrHNFcaYq8sHsc2DuO6V3YbnoZDvB6VvWar+KR+70S1wNNgXZ8W+9BSnaXxd5d4ZK4/drnYE10N8Asf07Fne7yqinC/fpoLF3cdaQ7dMth92gfdRtNlg8ScbqWe6BkgOnce0/7k9d1wbMGT4675KYKC1WS2geZmUYPiqhDB34Cb77iGlnQTsEGlePcIXUIVDZeTjLPixTHN/E4vjF2TaVRQ0/hnLG0JrsxGGZGPkCC6UC6mtTOrpZt0yeRF6egxCfGALKf4MKkh3qJ5sQpOyCI8ziJZNaf/ROlYtL53PJWxh3nodkS2IPD0LZMQmEppqIFotFXgaq/QZXME/uUsCWaZqoLdt33EnrS8+LouVjWt2W/bI8cwdfmReqcxNsLWjFK8vlG+BApKLxHG+bscTptC2dzJS7/RmvC7gwOoQspq+Tt2P4PwVRRZnJ7SdCaDT7HYILzUlmVgKd6AOa0YiIZqGECIUVU7IQsojeYCjNNjXkPVxnLYNM9zjJ7+SmzG3rOaeOa6364SGtS2/xAi/TPE5m9dJaFprWh0CacVcaBCt8EHTzlQWUjwntz+8VQC4Iq1GQUMAFLU2f/+EteprSrAFyV5XzYwADkX9okFUK14Jc/B//vD96uhg1ol6Yup0OVipaIvTaMUyvGip7KxZlfo6oPUjuqcKvSMmIJotAKN8sWHExnBLQHaBw9H09+ioK9pJdsg2b8/Wpv18xSNZA7Jw+6xErCmmjOBibhEHj7921NwBxfnxqxjRzS19vaVAzTAgnA3Ctar50WRKLnjPXhOUgvW60HJbtSZymrOK8wrMhpYMFFhltmUGlOWTY4AQsQj9rAAUeyxijyKM5m0Z9FSoNbPRrWitPZjPpDYfoiZ1Et7dkMNf+Y1FB/e7HEqhWm9Z1qBnEbrumXKag1lB5952AZOXRDZYX0kxZA0rkl1FckXb5j9Xp6EZ02qFcc9JvYCMg gaUDoK5z w+XcFG1y43DAUjoaYsEMHepxDxxPT6/29kYcIzm2VJZ8HFwzd0ag2aFVdvWQmyTjRLYoq0FzKUuqf6/hEYTM+SE41TKykb+Nyh9KhYNy3UICFbo1Zt3LfSRGUaOKP5QuPG+ZGqLSz4ARrSJXdfu4iEBxTZg== 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 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!