Message ID | 20230523073136.4900-1-vbabka@suse.cz |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1966170vqo; Tue, 23 May 2023 00:56:23 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5kPMJwdJ2ACeBIr1rKuef4Y36NnuSEwoHb12DB3rVNk2piW8wm2GjvnBv7GtKV1MI5bZ/c X-Received: by 2002:a05:6a20:7d99:b0:10b:7008:584c with SMTP id v25-20020a056a207d9900b0010b7008584cmr6655048pzj.22.1684828582600; Tue, 23 May 2023 00:56:22 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1684828582; cv=none; d=google.com; s=arc-20160816; b=WXXXbGkXBgVSL7kABtdNE7RzEcm0FsZlQkOa5n9McRjCbB7vU+gJgshDUfe+eyOl8d YunxVHFCIf2RmMGCynuseUAZwx/5VlDZ1J2VcG0pzW1jMvCcepTxQrt1+6ERb2+SI7ny 1z13PwolDm69YtsCd2cPhhZ0V4DqQndw+ZFSyplEfzvLig8TvCbPRdEdOxBQRyZWKsSk AwwNKCit8y0MzpnohXtdft1reux3flF7QDR3JJzYRuu76zMwgipwxY1im9mHSKEPuNaG 0Lftl5bqgXoNvtMcyCEdrG9IQKbICKxPpTaKNpkyHfuN9kQkzVDJpsbLVCniJ5E7SKn8 Ju6Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature:dkim-signature; bh=ZEzc5LOCCEl4X9/BHaKPnOKiEHFUp8WpILtxUD4qjNg=; b=nm0Nks/JoGfpr318hfz0DX49M49YS/+067B9aMLtGJkuSqOreIZIp9UiA7aEh9lFI9 C0O3PzdRHD4EaigCTNL/H2w2Dau00lLykyDn8ZMiQwSrFwg13++qMJa4Fl79wmaohuzc Fnf8N/R9ekeg0wOpBbOSrgPb/KmtCY5ZoQWxQVGCOfNqkyjD5S61+qvKn6DXeCNbE1ub chRy7JAOyeA5sEXhJUfFPZLSbaSXb8BbgV/a8l3p7HBo90E1GpRiM223kShYCxXPHBU9 gKuwDQ8ZfUlfuWic2mp+oBck5jsFZM8qcHOvd+GUIEyifPT8J/HtSJQ655gwfFe4x2KV QSUg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@suse.cz header.s=susede2_rsa header.b=12BbIW+5; dkim=neutral (no key) header.i=@suse.cz header.s=susede2_ed25519 header.b=JjwwBXLw; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id o15-20020a63a80f000000b00524eef9225bsi5857836pgf.550.2023.05.23.00.56.10; Tue, 23 May 2023 00:56:22 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@suse.cz header.s=susede2_rsa header.b=12BbIW+5; dkim=neutral (no key) header.i=@suse.cz header.s=susede2_ed25519 header.b=JjwwBXLw; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235281AbjEWHdA (ORCPT <rfc822;ahmedalshaiji.dev@gmail.com> + 99 others); Tue, 23 May 2023 03:33:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34814 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235277AbjEWHcd (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 23 May 2023 03:32:33 -0400 Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 670D510CB; Tue, 23 May 2023 00:31:58 -0700 (PDT) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 7F80722770; Tue, 23 May 2023 07:31:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1684827104; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=ZEzc5LOCCEl4X9/BHaKPnOKiEHFUp8WpILtxUD4qjNg=; b=12BbIW+5OtsAyqRSCvzLDYP3yElbsULJxVddqtKq/f2DjB9ngYqgGSk1x9aJAe2LFl0aT5 Av9d/xS5Au9Xg+ZXIMYrhmFHYiLJMZCFs1T3yHyWNXlbzGZoY+BYaJLAPzbQ3Hl+NgON2n IfGbXRLe/wU9OxcQZ2tmlvn32ZeejU8= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1684827104; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=ZEzc5LOCCEl4X9/BHaKPnOKiEHFUp8WpILtxUD4qjNg=; b=JjwwBXLwGDpGwrmS1npS/eF+SP7rLitCKRX5eu3zbu10pCWZ8EHThUk1eJoi2X3/KY/Nwv RRAeMt35EdLTFWBw== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 5222913588; Tue, 23 May 2023 07:31:44 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id cr9ZE+BrbGRSIQAAMHmgww (envelope-from <vbabka@suse.cz>); Tue, 23 May 2023 07:31:44 +0000 From: Vlastimil Babka <vbabka@suse.cz> To: Christoph Lameter <cl@linux.com>, Pekka Enberg <penberg@kernel.org>, David Rientjes <rientjes@google.com>, Joonsoo Kim <iamjoonsoo.kim@lge.com> Cc: Roman Gushchin <roman.gushchin@linux.dev>, Hyeonggon Yoo <42.hyeyoo@gmail.com>, Kees Cook <keescook@chromium.org>, linux-mm@kvack.org, linux-hardening@vger.kernel.org, patches@lists.linux.dev, linux-kernel@vger.kernel.org, Vlastimil Babka <vbabka@suse.cz> Subject: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR Date: Tue, 23 May 2023 09:31:36 +0200 Message-Id: <20230523073136.4900-1-vbabka@suse.cz> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_SOFTFAIL,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1766670430382085995?= X-GMAIL-MSGID: =?utf-8?q?1766670815700880761?= |
Series |
mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR
|
|
Commit Message
Vlastimil Babka
May 23, 2023, 7:31 a.m. UTC
With SLOB removed, both remaining allocators support hardened usercopy,
so remove the config and associated #ifdef.
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/Kconfig | 2 --
mm/slab.h | 9 ---------
security/Kconfig | 8 --------
3 files changed, 19 deletions(-)
Comments
On Tue, May 23, 2023 at 09:46:46AM +0200, Vlastimil Babka wrote: > On 5/23/23 09:42, Lorenzo Stoakes wrote: > > On Tue, May 23, 2023 at 09:31:36AM +0200, Vlastimil Babka wrote: > >> With SLOB removed, both remaining allocators support hardened usercopy, > >> so remove the config and associated #ifdef. > >> > >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > >> --- > >> mm/Kconfig | 2 -- > >> mm/slab.h | 9 --------- > >> security/Kconfig | 8 -------- > >> 3 files changed, 19 deletions(-) > >> > >> diff --git a/mm/Kconfig b/mm/Kconfig > >> index 7672a22647b4..041f0da42f2b 100644 > >> --- a/mm/Kconfig > >> +++ b/mm/Kconfig > >> @@ -221,7 +221,6 @@ choice > >> config SLAB > >> bool "SLAB" > >> depends on !PREEMPT_RT > >> - select HAVE_HARDENED_USERCOPY_ALLOCATOR > >> help > >> The regular slab allocator that is established and known to work > >> well in all environments. It organizes cache hot objects in > >> @@ -229,7 +228,6 @@ config SLAB > >> > >> config SLUB > >> bool "SLUB (Unqueued Allocator)" > >> - select HAVE_HARDENED_USERCOPY_ALLOCATOR > >> help > >> SLUB is a slab allocator that minimizes cache line usage > >> instead of managing queues of cached objects (SLAB approach). > >> diff --git a/mm/slab.h b/mm/slab.h > >> index f01ac256a8f5..695ef96b4b5b 100644 > >> --- a/mm/slab.h > >> +++ b/mm/slab.h > >> @@ -832,17 +832,8 @@ struct kmem_obj_info { > >> void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab); > >> #endif > >> > >> -#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR > >> void __check_heap_object(const void *ptr, unsigned long n, > >> const struct slab *slab, bool to_user); > >> -#else > >> -static inline > >> -void __check_heap_object(const void *ptr, unsigned long n, > >> - const struct slab *slab, bool to_user) > >> -{ > >> -} > >> -#endif > > > > Hm, this is still defined in slab.c/slub.c and invoked in usercopy.c, do we > > not want the prototype? > > Well I didn't delete the prototype, just the ifdef/else around, so now it's > there unconditionally. > > > Perhaps replacing with #ifdef > > CONFIG_HARDENED_USERCOPY instead? I may be missing something here :) > > Putting it under that #ifdef would work and match that the implementations > of that function are under that same ifdef, but maybe it's unnecessary noise > in the header? > Yeah my brain inserted extra '-'s there, sorry! Given we only define __check_heap_object() in sl[au]b.c if CONFIG_HARDENED_USERCOPY wouldn't we need to keep the empty version around if !CONFIG_HARDENED_USERCOPY since check_heap_object() appears to be called unconditionally?
On 23.05.23 09:56, Lorenzo Stoakes wrote: > On Tue, May 23, 2023 at 09:46:46AM +0200, Vlastimil Babka wrote: >> On 5/23/23 09:42, Lorenzo Stoakes wrote: >>> On Tue, May 23, 2023 at 09:31:36AM +0200, Vlastimil Babka wrote: >>>> With SLOB removed, both remaining allocators support hardened usercopy, >>>> so remove the config and associated #ifdef. >>>> >>>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >>>> --- >>>> mm/Kconfig | 2 -- >>>> mm/slab.h | 9 --------- >>>> security/Kconfig | 8 -------- >>>> 3 files changed, 19 deletions(-) >>>> >>>> diff --git a/mm/Kconfig b/mm/Kconfig >>>> index 7672a22647b4..041f0da42f2b 100644 >>>> --- a/mm/Kconfig >>>> +++ b/mm/Kconfig >>>> @@ -221,7 +221,6 @@ choice >>>> config SLAB >>>> bool "SLAB" >>>> depends on !PREEMPT_RT >>>> - select HAVE_HARDENED_USERCOPY_ALLOCATOR >>>> help >>>> The regular slab allocator that is established and known to work >>>> well in all environments. It organizes cache hot objects in >>>> @@ -229,7 +228,6 @@ config SLAB >>>> >>>> config SLUB >>>> bool "SLUB (Unqueued Allocator)" >>>> - select HAVE_HARDENED_USERCOPY_ALLOCATOR >>>> help >>>> SLUB is a slab allocator that minimizes cache line usage >>>> instead of managing queues of cached objects (SLAB approach). >>>> diff --git a/mm/slab.h b/mm/slab.h >>>> index f01ac256a8f5..695ef96b4b5b 100644 >>>> --- a/mm/slab.h >>>> +++ b/mm/slab.h >>>> @@ -832,17 +832,8 @@ struct kmem_obj_info { >>>> void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab); >>>> #endif >>>> >>>> -#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR >>>> void __check_heap_object(const void *ptr, unsigned long n, >>>> const struct slab *slab, bool to_user); >>>> -#else >>>> -static inline >>>> -void __check_heap_object(const void *ptr, unsigned long n, >>>> - const struct slab *slab, bool to_user) >>>> -{ >>>> -} >>>> -#endif >>> >>> Hm, this is still defined in slab.c/slub.c and invoked in usercopy.c, do we >>> not want the prototype? >> >> Well I didn't delete the prototype, just the ifdef/else around, so now it's >> there unconditionally. >> >>> Perhaps replacing with #ifdef >>> CONFIG_HARDENED_USERCOPY instead? I may be missing something here :) >> >> Putting it under that #ifdef would work and match that the implementations >> of that function are under that same ifdef, but maybe it's unnecessary noise >> in the header? >> > > Yeah my brain inserted extra '-'s there, sorry! > > Given we only define __check_heap_object() in sl[au]b.c if > CONFIG_HARDENED_USERCOPY wouldn't we need to keep the empty version around > if !CONFIG_HARDENED_USERCOPY since check_heap_object() appears to be called > unconditionally? > The file is only compiled with CONFIG_HARDENED_USERCOPY: mm/Makefile:obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o Reviewed-by: David Hildenbrand <david@redhat.com>
On Tue, May 23, 2023 at 10:14:24AM +0200, David Hildenbrand wrote: > On 23.05.23 09:56, Lorenzo Stoakes wrote: > > On Tue, May 23, 2023 at 09:46:46AM +0200, Vlastimil Babka wrote: > > > On 5/23/23 09:42, Lorenzo Stoakes wrote: > > > > On Tue, May 23, 2023 at 09:31:36AM +0200, Vlastimil Babka wrote: > > > > > With SLOB removed, both remaining allocators support hardened usercopy, > > > > > so remove the config and associated #ifdef. > > > > > > > > > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > > > > > --- > > > > > mm/Kconfig | 2 -- > > > > > mm/slab.h | 9 --------- > > > > > security/Kconfig | 8 -------- > > > > > 3 files changed, 19 deletions(-) > > > > > > > > > > diff --git a/mm/Kconfig b/mm/Kconfig > > > > > index 7672a22647b4..041f0da42f2b 100644 > > > > > --- a/mm/Kconfig > > > > > +++ b/mm/Kconfig > > > > > @@ -221,7 +221,6 @@ choice > > > > > config SLAB > > > > > bool "SLAB" > > > > > depends on !PREEMPT_RT > > > > > - select HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > help > > > > > The regular slab allocator that is established and known to work > > > > > well in all environments. It organizes cache hot objects in > > > > > @@ -229,7 +228,6 @@ config SLAB > > > > > > > > > > config SLUB > > > > > bool "SLUB (Unqueued Allocator)" > > > > > - select HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > help > > > > > SLUB is a slab allocator that minimizes cache line usage > > > > > instead of managing queues of cached objects (SLAB approach). > > > > > diff --git a/mm/slab.h b/mm/slab.h > > > > > index f01ac256a8f5..695ef96b4b5b 100644 > > > > > --- a/mm/slab.h > > > > > +++ b/mm/slab.h > > > > > @@ -832,17 +832,8 @@ struct kmem_obj_info { > > > > > void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab); > > > > > #endif > > > > > > > > > > -#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > void __check_heap_object(const void *ptr, unsigned long n, > > > > > const struct slab *slab, bool to_user); > > > > > -#else > > > > > -static inline > > > > > -void __check_heap_object(const void *ptr, unsigned long n, > > > > > - const struct slab *slab, bool to_user) > > > > > -{ > > > > > -} > > > > > -#endif > > > > > > > > Hm, this is still defined in slab.c/slub.c and invoked in usercopy.c, do we > > > > not want the prototype? > > > > > > Well I didn't delete the prototype, just the ifdef/else around, so now it's > > > there unconditionally. > > > > > > > Perhaps replacing with #ifdef > > > > CONFIG_HARDENED_USERCOPY instead? I may be missing something here :) > > > > > > Putting it under that #ifdef would work and match that the implementations > > > of that function are under that same ifdef, but maybe it's unnecessary noise > > > in the header? > > > > > > > Yeah my brain inserted extra '-'s there, sorry! > > > > Given we only define __check_heap_object() in sl[au]b.c if > > CONFIG_HARDENED_USERCOPY wouldn't we need to keep the empty version around > > if !CONFIG_HARDENED_USERCOPY since check_heap_object() appears to be called > > unconditionally? > > > > The file is only compiled with CONFIG_HARDENED_USERCOPY: > > mm/Makefile:obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o > Yeah ugh at this sort of implicit thing. Anyway it'd be preferable to stick #ifdef CONFIG_HARDENED_USERCOPY around the prototype just so it's abundantly clear this function doesn't exist unless that is set. > > > Reviewed-by: David Hildenbrand <david@redhat.com> > > -- > Thanks, > > David / dhildenb >
On 23.05.23 10:19, Lorenzo Stoakes wrote: > On Tue, May 23, 2023 at 10:14:24AM +0200, David Hildenbrand wrote: >> On 23.05.23 09:56, Lorenzo Stoakes wrote: >>> On Tue, May 23, 2023 at 09:46:46AM +0200, Vlastimil Babka wrote: >>>> On 5/23/23 09:42, Lorenzo Stoakes wrote: >>>>> On Tue, May 23, 2023 at 09:31:36AM +0200, Vlastimil Babka wrote: >>>>>> With SLOB removed, both remaining allocators support hardened usercopy, >>>>>> so remove the config and associated #ifdef. >>>>>> >>>>>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >>>>>> --- >>>>>> mm/Kconfig | 2 -- >>>>>> mm/slab.h | 9 --------- >>>>>> security/Kconfig | 8 -------- >>>>>> 3 files changed, 19 deletions(-) >>>>>> >>>>>> diff --git a/mm/Kconfig b/mm/Kconfig >>>>>> index 7672a22647b4..041f0da42f2b 100644 >>>>>> --- a/mm/Kconfig >>>>>> +++ b/mm/Kconfig >>>>>> @@ -221,7 +221,6 @@ choice >>>>>> config SLAB >>>>>> bool "SLAB" >>>>>> depends on !PREEMPT_RT >>>>>> - select HAVE_HARDENED_USERCOPY_ALLOCATOR >>>>>> help >>>>>> The regular slab allocator that is established and known to work >>>>>> well in all environments. It organizes cache hot objects in >>>>>> @@ -229,7 +228,6 @@ config SLAB >>>>>> >>>>>> config SLUB >>>>>> bool "SLUB (Unqueued Allocator)" >>>>>> - select HAVE_HARDENED_USERCOPY_ALLOCATOR >>>>>> help >>>>>> SLUB is a slab allocator that minimizes cache line usage >>>>>> instead of managing queues of cached objects (SLAB approach). >>>>>> diff --git a/mm/slab.h b/mm/slab.h >>>>>> index f01ac256a8f5..695ef96b4b5b 100644 >>>>>> --- a/mm/slab.h >>>>>> +++ b/mm/slab.h >>>>>> @@ -832,17 +832,8 @@ struct kmem_obj_info { >>>>>> void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab); >>>>>> #endif >>>>>> >>>>>> -#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR >>>>>> void __check_heap_object(const void *ptr, unsigned long n, >>>>>> const struct slab *slab, bool to_user); >>>>>> -#else >>>>>> -static inline >>>>>> -void __check_heap_object(const void *ptr, unsigned long n, >>>>>> - const struct slab *slab, bool to_user) >>>>>> -{ >>>>>> -} >>>>>> -#endif >>>>> >>>>> Hm, this is still defined in slab.c/slub.c and invoked in usercopy.c, do we >>>>> not want the prototype? >>>> >>>> Well I didn't delete the prototype, just the ifdef/else around, so now it's >>>> there unconditionally. >>>> >>>>> Perhaps replacing with #ifdef >>>>> CONFIG_HARDENED_USERCOPY instead? I may be missing something here :) >>>> >>>> Putting it under that #ifdef would work and match that the implementations >>>> of that function are under that same ifdef, but maybe it's unnecessary noise >>>> in the header? >>>> >>> >>> Yeah my brain inserted extra '-'s there, sorry! >>> >>> Given we only define __check_heap_object() in sl[au]b.c if >>> CONFIG_HARDENED_USERCOPY wouldn't we need to keep the empty version around >>> if !CONFIG_HARDENED_USERCOPY since check_heap_object() appears to be called >>> unconditionally? >>> >> >> The file is only compiled with CONFIG_HARDENED_USERCOPY: >> >> mm/Makefile:obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o >> > > Yeah ugh at this sort of implicit thing. Anyway it'd be preferable to stick > #ifdef CONFIG_HARDENED_USERCOPY around the prototype just so it's > abundantly clear this function doesn't exist unless that is set. I recall that it is very common to not use ifdefs unless really required. Because less ifefs are obviously preferable ;) Compilation+linking will fail in any case.
On Tue, May 23, 2023 at 10:14:24AM +0200, David Hildenbrand wrote: > On 23.05.23 09:56, Lorenzo Stoakes wrote: > > On Tue, May 23, 2023 at 09:46:46AM +0200, Vlastimil Babka wrote: > > > On 5/23/23 09:42, Lorenzo Stoakes wrote: > > > > On Tue, May 23, 2023 at 09:31:36AM +0200, Vlastimil Babka wrote: > > > > > With SLOB removed, both remaining allocators support hardened usercopy, > > > > > so remove the config and associated #ifdef. > > > > > > > > > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > > > > > --- > > > > > mm/Kconfig | 2 -- > > > > > mm/slab.h | 9 --------- > > > > > security/Kconfig | 8 -------- > > > > > 3 files changed, 19 deletions(-) > > > > > > > > > > diff --git a/mm/Kconfig b/mm/Kconfig > > > > > index 7672a22647b4..041f0da42f2b 100644 > > > > > --- a/mm/Kconfig > > > > > +++ b/mm/Kconfig > > > > > @@ -221,7 +221,6 @@ choice > > > > > config SLAB > > > > > bool "SLAB" > > > > > depends on !PREEMPT_RT > > > > > - select HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > help > > > > > The regular slab allocator that is established and known to work > > > > > well in all environments. It organizes cache hot objects in > > > > > @@ -229,7 +228,6 @@ config SLAB > > > > > > > > > > config SLUB > > > > > bool "SLUB (Unqueued Allocator)" > > > > > - select HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > help > > > > > SLUB is a slab allocator that minimizes cache line usage > > > > > instead of managing queues of cached objects (SLAB approach). > > > > > diff --git a/mm/slab.h b/mm/slab.h > > > > > index f01ac256a8f5..695ef96b4b5b 100644 > > > > > --- a/mm/slab.h > > > > > +++ b/mm/slab.h > > > > > @@ -832,17 +832,8 @@ struct kmem_obj_info { > > > > > void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab); > > > > > #endif > > > > > > > > > > -#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > void __check_heap_object(const void *ptr, unsigned long n, > > > > > const struct slab *slab, bool to_user); > > > > > -#else > > > > > -static inline > > > > > -void __check_heap_object(const void *ptr, unsigned long n, > > > > > - const struct slab *slab, bool to_user) > > > > > -{ > > > > > -} > > > > > -#endif > > > > > > > > Hm, this is still defined in slab.c/slub.c and invoked in usercopy.c, do we > > > > not want the prototype? > > > > > > Well I didn't delete the prototype, just the ifdef/else around, so now it's > > > there unconditionally. > > > > > > > Perhaps replacing with #ifdef > > > > CONFIG_HARDENED_USERCOPY instead? I may be missing something here :) > > > > > > Putting it under that #ifdef would work and match that the implementations > > > of that function are under that same ifdef, but maybe it's unnecessary noise > > > in the header? > > > > > > > Yeah my brain inserted extra '-'s there, sorry! > > > > Given we only define __check_heap_object() in sl[au]b.c if > > CONFIG_HARDENED_USERCOPY wouldn't we need to keep the empty version around > > if !CONFIG_HARDENED_USERCOPY since check_heap_object() appears to be called > > unconditionally? > > > > The file is only compiled with CONFIG_HARDENED_USERCOPY: > > mm/Makefile:obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o Right. > Reviewed-by: David Hildenbrand <david@redhat.com> Thanks! Reviewed-by: Kees Cook <keescook@chromium.org>
On Tue, 23 May 2023, Kees Cook wrote: > On Tue, May 23, 2023 at 10:14:24AM +0200, David Hildenbrand wrote: > > On 23.05.23 09:56, Lorenzo Stoakes wrote: > > > On Tue, May 23, 2023 at 09:46:46AM +0200, Vlastimil Babka wrote: > > > > On 5/23/23 09:42, Lorenzo Stoakes wrote: > > > > > On Tue, May 23, 2023 at 09:31:36AM +0200, Vlastimil Babka wrote: > > > > > > With SLOB removed, both remaining allocators support hardened usercopy, > > > > > > so remove the config and associated #ifdef. > > > > > > > > > > > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > > > > > > --- > > > > > > mm/Kconfig | 2 -- > > > > > > mm/slab.h | 9 --------- > > > > > > security/Kconfig | 8 -------- > > > > > > 3 files changed, 19 deletions(-) > > > > > > > > > > > > diff --git a/mm/Kconfig b/mm/Kconfig > > > > > > index 7672a22647b4..041f0da42f2b 100644 > > > > > > --- a/mm/Kconfig > > > > > > +++ b/mm/Kconfig > > > > > > @@ -221,7 +221,6 @@ choice > > > > > > config SLAB > > > > > > bool "SLAB" > > > > > > depends on !PREEMPT_RT > > > > > > - select HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > > help > > > > > > The regular slab allocator that is established and known to work > > > > > > well in all environments. It organizes cache hot objects in > > > > > > @@ -229,7 +228,6 @@ config SLAB > > > > > > > > > > > > config SLUB > > > > > > bool "SLUB (Unqueued Allocator)" > > > > > > - select HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > > help > > > > > > SLUB is a slab allocator that minimizes cache line usage > > > > > > instead of managing queues of cached objects (SLAB approach). > > > > > > diff --git a/mm/slab.h b/mm/slab.h > > > > > > index f01ac256a8f5..695ef96b4b5b 100644 > > > > > > --- a/mm/slab.h > > > > > > +++ b/mm/slab.h > > > > > > @@ -832,17 +832,8 @@ struct kmem_obj_info { > > > > > > void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab); > > > > > > #endif > > > > > > > > > > > > -#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > > void __check_heap_object(const void *ptr, unsigned long n, > > > > > > const struct slab *slab, bool to_user); > > > > > > -#else > > > > > > -static inline > > > > > > -void __check_heap_object(const void *ptr, unsigned long n, > > > > > > - const struct slab *slab, bool to_user) > > > > > > -{ > > > > > > -} > > > > > > -#endif > > > > > > > > > > Hm, this is still defined in slab.c/slub.c and invoked in usercopy.c, do we > > > > > not want the prototype? > > > > > > > > Well I didn't delete the prototype, just the ifdef/else around, so now it's > > > > there unconditionally. > > > > > > > > > Perhaps replacing with #ifdef > > > > > CONFIG_HARDENED_USERCOPY instead? I may be missing something here :) > > > > > > > > Putting it under that #ifdef would work and match that the implementations > > > > of that function are under that same ifdef, but maybe it's unnecessary noise > > > > in the header? > > > > > > > > > > Yeah my brain inserted extra '-'s there, sorry! > > > > > > Given we only define __check_heap_object() in sl[au]b.c if > > > CONFIG_HARDENED_USERCOPY wouldn't we need to keep the empty version around > > > if !CONFIG_HARDENED_USERCOPY since check_heap_object() appears to be called > > > unconditionally? > > > > > > > The file is only compiled with CONFIG_HARDENED_USERCOPY: > > > > mm/Makefile:obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o > > Right. > > > Reviewed-by: David Hildenbrand <david@redhat.com> > > Thanks! > > Reviewed-by: Kees Cook <keescook@chromium.org> > Acked-by: David Rientjes <rientjes@google.com>
On Tue, May 23, 2023 at 05:31:47PM -0700, David Rientjes wrote: > On Tue, 23 May 2023, Kees Cook wrote: > > > On Tue, May 23, 2023 at 10:14:24AM +0200, David Hildenbrand wrote: > > > On 23.05.23 09:56, Lorenzo Stoakes wrote: > > > > On Tue, May 23, 2023 at 09:46:46AM +0200, Vlastimil Babka wrote: > > > > > On 5/23/23 09:42, Lorenzo Stoakes wrote: > > > > > > On Tue, May 23, 2023 at 09:31:36AM +0200, Vlastimil Babka wrote: > > > > > > > With SLOB removed, both remaining allocators support hardened usercopy, > > > > > > > so remove the config and associated #ifdef. > > > > > > > > > > > > > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > > > > > > > --- > > > > > > > mm/Kconfig | 2 -- > > > > > > > mm/slab.h | 9 --------- > > > > > > > security/Kconfig | 8 -------- > > > > > > > 3 files changed, 19 deletions(-) > > > > > > > > > > > > > > diff --git a/mm/Kconfig b/mm/Kconfig > > > > > > > index 7672a22647b4..041f0da42f2b 100644 > > > > > > > --- a/mm/Kconfig > > > > > > > +++ b/mm/Kconfig > > > > > > > @@ -221,7 +221,6 @@ choice > > > > > > > config SLAB > > > > > > > bool "SLAB" > > > > > > > depends on !PREEMPT_RT > > > > > > > - select HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > > > help > > > > > > > The regular slab allocator that is established and known to work > > > > > > > well in all environments. It organizes cache hot objects in > > > > > > > @@ -229,7 +228,6 @@ config SLAB > > > > > > > > > > > > > > config SLUB > > > > > > > bool "SLUB (Unqueued Allocator)" > > > > > > > - select HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > > > help > > > > > > > SLUB is a slab allocator that minimizes cache line usage > > > > > > > instead of managing queues of cached objects (SLAB approach). > > > > > > > diff --git a/mm/slab.h b/mm/slab.h > > > > > > > index f01ac256a8f5..695ef96b4b5b 100644 > > > > > > > --- a/mm/slab.h > > > > > > > +++ b/mm/slab.h > > > > > > > @@ -832,17 +832,8 @@ struct kmem_obj_info { > > > > > > > void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab); > > > > > > > #endif > > > > > > > > > > > > > > -#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR > > > > > > > void __check_heap_object(const void *ptr, unsigned long n, > > > > > > > const struct slab *slab, bool to_user); > > > > > > > -#else > > > > > > > -static inline > > > > > > > -void __check_heap_object(const void *ptr, unsigned long n, > > > > > > > - const struct slab *slab, bool to_user) > > > > > > > -{ > > > > > > > -} > > > > > > > -#endif > > > > > > > > > > > > Hm, this is still defined in slab.c/slub.c and invoked in usercopy.c, do we > > > > > > not want the prototype? > > > > > > > > > > Well I didn't delete the prototype, just the ifdef/else around, so now it's > > > > > there unconditionally. > > > > > > > > > > > Perhaps replacing with #ifdef > > > > > > CONFIG_HARDENED_USERCOPY instead? I may be missing something here :) > > > > > > > > > > Putting it under that #ifdef would work and match that the implementations > > > > > of that function are under that same ifdef, but maybe it's unnecessary noise > > > > > in the header? > > > > > > > > > > > > > Yeah my brain inserted extra '-'s there, sorry! > > > > > > > > Given we only define __check_heap_object() in sl[au]b.c if > > > > CONFIG_HARDENED_USERCOPY wouldn't we need to keep the empty version around > > > > if !CONFIG_HARDENED_USERCOPY since check_heap_object() appears to be called > > > > unconditionally? > > > > > > > > > > The file is only compiled with CONFIG_HARDENED_USERCOPY: > > > > > > mm/Makefile:obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o > > > > Right. > > > > > Reviewed-by: David Hildenbrand <david@redhat.com> > > > > Thanks! > > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > Acked-by: David Rientjes <rientjes@google.com> looks fine to me, Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
On Tue, May 23, 2023 at 10:28:32AM +0200, David Hildenbrand wrote: [snip] > > > The file is only compiled with CONFIG_HARDENED_USERCOPY: > > > > > > mm/Makefile:obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o > > > > > > > Yeah ugh at this sort of implicit thing. Anyway it'd be preferable to stick > > #ifdef CONFIG_HARDENED_USERCOPY around the prototype just so it's > > abundantly clear this function doesn't exist unless that is set. > > I recall that it is very common to not use ifdefs unless really required. > Because less ifefs are obviously preferable ;) > > Compilation+linking will fail in any case. > I don't want to insist so hard on something that doesn't really matter, the bike shed can be blue, green or red it's fine :P So, Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>
diff --git a/mm/Kconfig b/mm/Kconfig index 7672a22647b4..041f0da42f2b 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -221,7 +221,6 @@ choice config SLAB bool "SLAB" depends on !PREEMPT_RT - select HAVE_HARDENED_USERCOPY_ALLOCATOR help The regular slab allocator that is established and known to work well in all environments. It organizes cache hot objects in @@ -229,7 +228,6 @@ config SLAB config SLUB bool "SLUB (Unqueued Allocator)" - select HAVE_HARDENED_USERCOPY_ALLOCATOR help SLUB is a slab allocator that minimizes cache line usage instead of managing queues of cached objects (SLAB approach). diff --git a/mm/slab.h b/mm/slab.h index f01ac256a8f5..695ef96b4b5b 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -832,17 +832,8 @@ struct kmem_obj_info { void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab); #endif -#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR void __check_heap_object(const void *ptr, unsigned long n, const struct slab *slab, bool to_user); -#else -static inline -void __check_heap_object(const void *ptr, unsigned long n, - const struct slab *slab, bool to_user) -{ -} -#endif - #ifdef CONFIG_SLUB_DEBUG void skip_orig_size_check(struct kmem_cache *s, const void *object); #endif diff --git a/security/Kconfig b/security/Kconfig index 97abeb9b9a19..52c9af08ad35 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -127,16 +127,8 @@ config LSM_MMAP_MIN_ADDR this low address space will need the permission specific to the systems running LSM. -config HAVE_HARDENED_USERCOPY_ALLOCATOR - bool - help - The heap allocator implements __check_heap_object() for - validating memory ranges against heap object sizes in - support of CONFIG_HARDENED_USERCOPY. - config HARDENED_USERCOPY bool "Harden memory copies between kernel and userspace" - depends on HAVE_HARDENED_USERCOPY_ALLOCATOR imply STRICT_DEVMEM help This option checks for obviously wrong memory regions when