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 AC242C4345F for ; Mon, 29 Apr 2024 14:32:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2FDD66B0092; Mon, 29 Apr 2024 10:32:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2AE426B0093; Mon, 29 Apr 2024 10:32:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 19DD76B0095; Mon, 29 Apr 2024 10:32:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id F04A46B0092 for ; Mon, 29 Apr 2024 10:32:58 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 3D241A00B8 for ; Mon, 29 Apr 2024 14:32:58 +0000 (UTC) X-FDA: 82062811236.11.2091611 Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) by imf04.hostedemail.com (Postfix) with ESMTP id A24D540019 for ; Mon, 29 Apr 2024 14:32:54 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=clip-os.org header.s=gm1 header.b=Q6HQ1p1I; dmarc=pass (policy=none) header.from=clip-os.org; spf=pass (imf04.hostedemail.com: domain of nicolas.bouchinet@clip-os.org designates 217.70.183.199 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=1714401175; 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=YBV4e+LFdx37RYtGfNqESt5nooG6GvmSE7HSLDnWTtY=; b=cMe8vwqFVucmLyF3wQi5n3gGR4UzR1fpvMGIkY3vTFKXfgNVdZNp+PrVpJJrz9osyQ3XYp LRcFVzKSSBNDXgpZhcB01r64Tm8yEShEctSCA1h3qWSXn8FDIkEgptgFoIM0Ub04WmpwzD BNKDE7kPpni1oh4Hke6t0x/ME1OGKLo= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=clip-os.org header.s=gm1 header.b=Q6HQ1p1I; dmarc=pass (policy=none) header.from=clip-os.org; spf=pass (imf04.hostedemail.com: domain of nicolas.bouchinet@clip-os.org designates 217.70.183.199 as permitted sender) smtp.mailfrom=nicolas.bouchinet@clip-os.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714401175; a=rsa-sha256; cv=none; b=SI6NkrQGJg1S9evsQzgyGu08VMERi5cds1PenzaUOUehrDDztyz5CXSpFo5GrAn9vTBe+W TMcNKWYc/Yg10eWthyymH9J1WEL+UMKMDJ3NDaumn+FtsrWjKe1eJOy4QC9bz0F9zYqITj GA/AsheET1W8Vn1CoAhw76B4YZvOsGo= Received: by mail.gandi.net (Postfix) with ESMTPSA id 355FFFF807; Mon, 29 Apr 2024 14:32:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=clip-os.org; s=gm1; t=1714401172; 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=YBV4e+LFdx37RYtGfNqESt5nooG6GvmSE7HSLDnWTtY=; b=Q6HQ1p1IcoAdNvsrwUpy4LMQvqouNAZvU3e/nvQY47GKEznrvvc2NjXNZwYE8FyxmSRsvs IuWfklj/bpwjmFbxG6N5RvYy883tdwYRNIe0kBDbJvx/A1dHDcRYskM2US2R0leth8SINp Uzo+dXcHB+C/qt8FHA3flRCSEj2o66DUYqO5IAsCl16ZRulGAsl2CSBHKtuqufisN9qbEv uV5ReAyblY6CQsPR9nOuCg/yyzpst6rx4FKRNcvP8HUdYiWaanzLmTz70c530mA6213ru9 lfqME37otCQ6qkU8OhJs11FBsVKDCdk344sbamAr+iG8PeCMLOzafTVS5gMyUg== Message-ID: <83fda406-0340-4b7b-9f02-e9eb41c77f0e@clip-os.org> Date: Mon, 29 Apr 2024 16:32:49 +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> Content-Language: en-US From: Nicolas Bouchinet 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: A24D540019 X-Stat-Signature: hrt4exbuhrgs9djkrp51muoqr3t7xuw6 X-HE-Tag: 1714401174-361704 X-HE-Meta: U2FsdGVkX1+M2x9TqN4q69/Y1/lUvYi+81wjUa7ZNoTkw+WTrvMMYz4TM54BDm5qGo7aXwaAFcaCdBQghLzdTeVM1vcwLIl4v62UrM4vMhPyV+EoaNZHf5C+cAL3ap5xsGYDWQ/FQ1bW46X2itE5GsnF1Bz7s7YmOwLGE0llPlgz7s2S6aPXd9rmqRk0aKHpkLgfl+SZKtepAoEGEcmpqeV/+4Z5bqFbFun8Tnw7ZKVQnWlJJ/6G/zujqX23zISH2u3V/10DhLn3pcHBcLhxg0FPzyAEscygPyJa1wWnAdau8M6KYamRP/iNZRpuXLX4vzbs1mtiYwLP4xAlPM2rjfU+eJzh97sNouUBsJVJ7jzw6LFCJ9ISq8hI4P+dNDn2bJD/AFApoixlWxhhRw+AQv4GouUn1Wermh/lCKT/hJzrbduW2gdhmtPfnZ0JjzbFiiYi+7JShl214tSKTs8LcH41HbqtUt1y6EjJsxGwrTqphXSKbKfjEvyZqqc+sBXQujV66j6lnd7FD3J4GahRZ/1GiyvtHaNLH4fSKJXBEu39QtRzWa3UVKdaeedfyD6KwgSIYgUdehfxpq0dvvh1wTp324CQ/QlrqwcKO/yBGgrXYARVAU7mkjRRCnm/3mnSP7okPybnWT+/RjQDjpp5brwZHJgznzXi4ORyDcpaRzyPW4Sz6Nhz4+nVD3ce+y/jCEMEQZ375tZVh1/+oGYcl8r2tMFotOB1EzbKDu801HEraEJxy+m6GclprTMS2RVrKTfnTiHyRNGc+pSD5ftXPCJdCS+hPFJZC4tWxz7c7BGLqC+dNCHO1hcSmlGZXztBMpnGFcbRJ44SiWcvXB3jaPgYi/iZkum6dhsxzS6vIcASq7BxljAEwVCFQEQBBU7TJHl0JFMMKEhRwuHMVkQy0dz0S+Ckw8lAoQuPS1ewLzWmUALE00WJCX5Xcgpvkzi6PBx/G/SVts11LI3zq9w oWUMUiHx ROx4V3XqtSMW5eRnQbaKBzeOkbnisiCKgn17y8r+Haq7PJiBT6O0+ojGDhmfbDLvpBPHRPu3svGRU/muPg73oOV6Ck8D0oQ1NOhgCFMOlqZK7QUlDXgF4cty2431SDUdAnktHOrdtkKRsJrNEEZEAF70xm+Ht1fVWlbIqL3cQ+LeGm1fCTWGX2V0GjuhGflyF5tLfG3f1H5HOL9gIzW2KPr8s3ZxQNM/YbXm1UthBFdr/n8iZR+nKjthd7dAQ/DiOXitwDeBUD/DTeDYZEpVIYhd+JQ== 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 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 ? Best regards, Nicolas