* [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR @ 2023-05-23 7:31 Vlastimil Babka 2023-05-23 7:42 ` Lorenzo Stoakes 0 siblings, 1 reply; 11+ messages in thread From: Vlastimil Babka @ 2023-05-23 7:31 UTC (permalink / raw) To: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim Cc: Roman Gushchin, Hyeonggon Yoo, Kees Cook, linux-mm, linux-hardening, patches, linux-kernel, Vlastimil Babka 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 - #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 -- 2.40.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR 2023-05-23 7:31 [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR Vlastimil Babka @ 2023-05-23 7:42 ` Lorenzo Stoakes 2023-05-23 7:46 ` Vlastimil Babka 0 siblings, 1 reply; 11+ messages in thread From: Lorenzo Stoakes @ 2023-05-23 7:42 UTC (permalink / raw) To: Vlastimil Babka Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Kees Cook, linux-mm, linux-hardening, patches, linux-kernel 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? Perhaps replacing with #ifdef CONFIG_HARDENED_USERCOPY instead? I may be missing something here :) > - > #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 > -- > 2.40.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR 2023-05-23 7:42 ` Lorenzo Stoakes @ 2023-05-23 7:46 ` Vlastimil Babka 2023-05-23 7:56 ` Lorenzo Stoakes 0 siblings, 1 reply; 11+ messages in thread From: Vlastimil Babka @ 2023-05-23 7:46 UTC (permalink / raw) To: Lorenzo Stoakes Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Kees Cook, linux-mm, linux-hardening, patches, linux-kernel 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR 2023-05-23 7:46 ` Vlastimil Babka @ 2023-05-23 7:56 ` Lorenzo Stoakes 2023-05-23 8:14 ` David Hildenbrand 0 siblings, 1 reply; 11+ messages in thread From: Lorenzo Stoakes @ 2023-05-23 7:56 UTC (permalink / raw) To: Vlastimil Babka Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Kees Cook, linux-mm, linux-hardening, patches, linux-kernel 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR 2023-05-23 7:56 ` Lorenzo Stoakes @ 2023-05-23 8:14 ` David Hildenbrand 2023-05-23 8:19 ` Lorenzo Stoakes 2023-05-23 17:02 ` Kees Cook 0 siblings, 2 replies; 11+ messages in thread From: David Hildenbrand @ 2023-05-23 8:14 UTC (permalink / raw) To: Lorenzo Stoakes, Vlastimil Babka Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Kees Cook, linux-mm, linux-hardening, patches, linux-kernel 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> -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR 2023-05-23 8:14 ` David Hildenbrand @ 2023-05-23 8:19 ` Lorenzo Stoakes 2023-05-23 8:28 ` David Hildenbrand 2023-05-23 17:02 ` Kees Cook 1 sibling, 1 reply; 11+ messages in thread From: Lorenzo Stoakes @ 2023-05-23 8:19 UTC (permalink / raw) To: David Hildenbrand Cc: Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Kees Cook, linux-mm, linux-hardening, patches, linux-kernel 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR 2023-05-23 8:19 ` Lorenzo Stoakes @ 2023-05-23 8:28 ` David Hildenbrand 2023-05-24 7:15 ` Lorenzo Stoakes 0 siblings, 1 reply; 11+ messages in thread From: David Hildenbrand @ 2023-05-23 8:28 UTC (permalink / raw) To: Lorenzo Stoakes Cc: Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Kees Cook, linux-mm, linux-hardening, patches, linux-kernel 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. -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR 2023-05-23 8:28 ` David Hildenbrand @ 2023-05-24 7:15 ` Lorenzo Stoakes 0 siblings, 0 replies; 11+ messages in thread From: Lorenzo Stoakes @ 2023-05-24 7:15 UTC (permalink / raw) To: David Hildenbrand Cc: Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Kees Cook, linux-mm, linux-hardening, patches, linux-kernel 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR 2023-05-23 8:14 ` David Hildenbrand 2023-05-23 8:19 ` Lorenzo Stoakes @ 2023-05-23 17:02 ` Kees Cook 2023-05-24 0:31 ` David Rientjes 1 sibling, 1 reply; 11+ messages in thread From: Kees Cook @ 2023-05-23 17:02 UTC (permalink / raw) To: David Hildenbrand Cc: Lorenzo Stoakes, Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, linux-mm, linux-hardening, patches, linux-kernel 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> -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR 2023-05-23 17:02 ` Kees Cook @ 2023-05-24 0:31 ` David Rientjes 2023-05-24 6:15 ` Hyeonggon Yoo 0 siblings, 1 reply; 11+ messages in thread From: David Rientjes @ 2023-05-24 0:31 UTC (permalink / raw) To: Kees Cook Cc: David Hildenbrand, Lorenzo Stoakes, Vlastimil Babka, Christoph Lameter, Pekka Enberg, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, linux-mm, linux-hardening, patches, linux-kernel 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR 2023-05-24 0:31 ` David Rientjes @ 2023-05-24 6:15 ` Hyeonggon Yoo 0 siblings, 0 replies; 11+ messages in thread From: Hyeonggon Yoo @ 2023-05-24 6:15 UTC (permalink / raw) To: David Rientjes Cc: Kees Cook, David Hildenbrand, Lorenzo Stoakes, Vlastimil Babka, Christoph Lameter, Pekka Enberg, Joonsoo Kim, Roman Gushchin, linux-mm, linux-hardening, patches, linux-kernel 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> -- Hyeonggon Yoo Doing kernel stuff as a hobby Undergraduate | Chungnam National University Dept. Computer Science & Engineering ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-05-24 7:16 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-23 7:31 [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR Vlastimil Babka 2023-05-23 7:42 ` Lorenzo Stoakes 2023-05-23 7:46 ` Vlastimil Babka 2023-05-23 7:56 ` Lorenzo Stoakes 2023-05-23 8:14 ` David Hildenbrand 2023-05-23 8:19 ` Lorenzo Stoakes 2023-05-23 8:28 ` David Hildenbrand 2023-05-24 7:15 ` Lorenzo Stoakes 2023-05-23 17:02 ` Kees Cook 2023-05-24 0:31 ` David Rientjes 2023-05-24 6:15 ` Hyeonggon Yoo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).