With SLOB removed, both remaining allocators support hardened usercopy,
so remove the config and associated #ifdef.
Signed-off-by: Vlastimil Babka <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
--
Thanks,
David / dhildenb
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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
>
> --
> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
Thanks!
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
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 <[email protected]>
> > > > > > ---
> > > > > > 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 <[email protected]>
>
> Thanks!
>
> Reviewed-by: Kees Cook <[email protected]>
>
Acked-by: David Rientjes <[email protected]>
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 <[email protected]>
> > > > > > > ---
> > > > > > > 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 <[email protected]>
> >
> > Thanks!
> >
> > Reviewed-by: Kees Cook <[email protected]>
> >
>
> Acked-by: David Rientjes <[email protected]>
looks fine to me,
Acked-by: Hyeonggon Yoo <[email protected]>
--
Hyeonggon Yoo
Doing kernel stuff as a hobby
Undergraduate | Chungnam National University
Dept. Computer Science & Engineering
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 <[email protected]>