| Message ID | 20230523073136.4900-1-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR | expand |
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 >
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?
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
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(-)