[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7a16b351-c7f6-b6a3-869b-5163c0d0793f@google.com>
Date: Tue, 23 May 2023 17:31:47 -0700 (PDT)
From: David Rientjes <rientjes@...gle.com>
To: Kees Cook <keescook@...omium.org>
cc: David Hildenbrand <david@...hat.com>,
Lorenzo Stoakes <lstoakes@...il.com>,
Vlastimil Babka <vbabka@...e.cz>,
Christoph Lameter <cl@...ux.com>,
Pekka Enberg <penberg@...nel.org>,
Joonsoo Kim <iamjoonsoo.kim@....com>,
Roman Gushchin <roman.gushchin@...ux.dev>,
Hyeonggon Yoo <42.hyeyoo@...il.com>, linux-mm@...ck.org,
linux-hardening@...r.kernel.org, patches@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR
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@...e.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@...hat.com>
>
> Thanks!
>
> Reviewed-by: Kees Cook <keescook@...omium.org>
>
Acked-by: David Rientjes <rientjes@...gle.com>
Powered by blists - more mailing lists