diff mbox series

mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR

Message ID 20230523073136.4900-1-vbabka@suse.cz
State New
Headers
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

Lorenzo Stoakes May 23, 2023, 7:56 a.m. UTC | #1 
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?
David Hildenbrand May 23, 2023, 8:14 a.m. UTC | #2 
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>
Lorenzo Stoakes May 23, 2023, 8:19 a.m. UTC | #3 
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
>
David Hildenbrand May 23, 2023, 8:28 a.m. UTC | #4 
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.
Kees Cook May 23, 2023, 5:02 p.m. UTC | #5 
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>
David Rientjes May 24, 2023, 12:31 a.m. UTC | #6 
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>
Hyeonggon Yoo May 24, 2023, 6:15 a.m. UTC | #7 
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>
Lorenzo Stoakes May 24, 2023, 7:15 a.m. UTC | #8 
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 mbox series

Patch

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