diff mbox series

[v2,6/7] percpu: Add __alloc_size attributes for better bounds checking

Message ID 20210818214021.2476230-7-keescook@chromium.org (mailing list archive)
State New
Headers show
Series Add __alloc_size() for better bounds checking | expand

Commit Message

Kees Cook Aug. 18, 2021, 9:40 p.m. UTC 
As already done in GrapheneOS, add the __alloc_size attribute for
appropriate percpu allocator interfaces, to provide additional hinting
for better bounds checking, assisting CONFIG_FORTIFY_SOURCE and other
compiler optimizations.

Co-developed-by: Daniel Micay <danielmicay@gmail.com>
Signed-off-by: Daniel Micay <danielmicay@gmail.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: linux-mm@kvack.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/percpu.h | 3 +++
 1 file changed, 3 insertions(+)

Comments

Dennis Zhou Aug. 19, 2021, 12:42 a.m. UTC | #1 
Hello,

On Wed, Aug 18, 2021 at 02:40:20PM -0700, Kees Cook wrote:
> As already done in GrapheneOS, add the __alloc_size attribute for
> appropriate percpu allocator interfaces, to provide additional hinting
> for better bounds checking, assisting CONFIG_FORTIFY_SOURCE and other
> compiler optimizations.

Can you elaborate a little bit for me how this works for percpu? In any
case that's not uniprocessor, any modification is done through address
accessors and not on the returned percpu pointer. Is the metadata kept
by gcc/clang able to transpire the percpu pointer accessors?

Thanks,
Dennis

> 
> Co-developed-by: Daniel Micay <danielmicay@gmail.com>
> Signed-off-by: Daniel Micay <danielmicay@gmail.com>
> Cc: Dennis Zhou <dennis@kernel.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: linux-mm@kvack.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/percpu.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/percpu.h b/include/linux/percpu.h
> index 5e76af742c80..119f41815b32 100644
> --- a/include/linux/percpu.h
> +++ b/include/linux/percpu.h
> @@ -123,6 +123,7 @@ extern int __init pcpu_page_first_chunk(size_t reserved_size,
>  				pcpu_fc_populate_pte_fn_t populate_pte_fn);
>  #endif
>  
> +__alloc_size(1)
>  extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align);
>  extern bool __is_kernel_percpu_address(unsigned long addr, unsigned long *can_addr);
>  extern bool is_kernel_percpu_address(unsigned long addr);
> @@ -131,7 +132,9 @@ extern bool is_kernel_percpu_address(unsigned long addr);
>  extern void __init setup_per_cpu_areas(void);
>  #endif
>  
> +__alloc_size(1)
>  extern void __percpu *__alloc_percpu_gfp(size_t size, size_t align, gfp_t gfp);
> +__alloc_size(1)
>  extern void __percpu *__alloc_percpu(size_t size, size_t align);
>  extern void free_percpu(void __percpu *__pdata);
>  extern phys_addr_t per_cpu_ptr_to_phys(void *addr);
> -- 
> 2.30.2
>
Kees Cook Aug. 19, 2021, 3:36 a.m. UTC | #2 
On Wed, Aug 18, 2021 at 08:42:59PM -0400, Dennis Zhou wrote:
> On Wed, Aug 18, 2021 at 02:40:20PM -0700, Kees Cook wrote:
> > As already done in GrapheneOS, add the __alloc_size attribute for
> > appropriate percpu allocator interfaces, to provide additional hinting
> > for better bounds checking, assisting CONFIG_FORTIFY_SOURCE and other
> > compiler optimizations.
> 
> Can you elaborate a little bit for me how this works for percpu? In any
> case that's not uniprocessor, any modification is done through address
> accessors and not on the returned percpu pointer. Is the metadata kept
> by gcc/clang able to transpire the percpu pointer accessors?

That's an excellent point. :P I haven't tested it through the accessors,
but I guess it's possible that this is only useful for UP, and even
then, only where the access is very close to the "allocation", maybe
like:

char __percpu *test_buf;

	char *buf;
	test_var = __alloc_percpu(16, __alignof__(char));
	buf = per_cpu_ptr(test_buf, get_cpu());
	...
	buf[20] = '!';

-Kees

> 
> Thanks,
> Dennis
> 
> > 
> > Co-developed-by: Daniel Micay <danielmicay@gmail.com>
> > Signed-off-by: Daniel Micay <danielmicay@gmail.com>
> > Cc: Dennis Zhou <dennis@kernel.org>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Christoph Lameter <cl@linux.com>
> > Cc: linux-mm@kvack.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  include/linux/percpu.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/linux/percpu.h b/include/linux/percpu.h
> > index 5e76af742c80..119f41815b32 100644
> > --- a/include/linux/percpu.h
> > +++ b/include/linux/percpu.h
> > @@ -123,6 +123,7 @@ extern int __init pcpu_page_first_chunk(size_t reserved_size,
> >  				pcpu_fc_populate_pte_fn_t populate_pte_fn);
> >  #endif
> >  
> > +__alloc_size(1)
> >  extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align);
> >  extern bool __is_kernel_percpu_address(unsigned long addr, unsigned long *can_addr);
> >  extern bool is_kernel_percpu_address(unsigned long addr);
> > @@ -131,7 +132,9 @@ extern bool is_kernel_percpu_address(unsigned long addr);
> >  extern void __init setup_per_cpu_areas(void);
> >  #endif
> >  
> > +__alloc_size(1)
> >  extern void __percpu *__alloc_percpu_gfp(size_t size, size_t align, gfp_t gfp);
> > +__alloc_size(1)
> >  extern void __percpu *__alloc_percpu(size_t size, size_t align);
> >  extern void free_percpu(void __percpu *__pdata);
> >  extern phys_addr_t per_cpu_ptr_to_phys(void *addr);
> > -- 
> > 2.30.2
> >
Dennis Zhou Aug. 19, 2021, 2:12 p.m. UTC | #3 
On Wed, Aug 18, 2021 at 08:36:50PM -0700, Kees Cook wrote:
> On Wed, Aug 18, 2021 at 08:42:59PM -0400, Dennis Zhou wrote:
> > On Wed, Aug 18, 2021 at 02:40:20PM -0700, Kees Cook wrote:
> > > As already done in GrapheneOS, add the __alloc_size attribute for
> > > appropriate percpu allocator interfaces, to provide additional hinting
> > > for better bounds checking, assisting CONFIG_FORTIFY_SOURCE and other
> > > compiler optimizations.
> > 
> > Can you elaborate a little bit for me how this works for percpu? In any
> > case that's not uniprocessor, any modification is done through address
> > accessors and not on the returned percpu pointer. Is the metadata kept
> > by gcc/clang able to transpire the percpu pointer accessors?
> 
> That's an excellent point. :P I haven't tested it through the accessors,
> but I guess it's possible that this is only useful for UP, and even
> then, only where the access is very close to the "allocation", maybe
> like:
> 

I see that this is already pulled by Andrew, but I think it would be
good to modify the commit log to add a short bit about this limitation.
Otherwise, the commit reads as if it's doing way more than it is.

Thanks,
Dennis

> char __percpu *test_buf;
> 
> 	char *buf;
> 	test_var = __alloc_percpu(16, __alignof__(char));
> 	buf = per_cpu_ptr(test_buf, get_cpu());
> 	...
> 	buf[20] = '!';
> 
> -Kees
> 
> > 
> > Thanks,
> > Dennis
> > 
> > > 
> > > Co-developed-by: Daniel Micay <danielmicay@gmail.com>
> > > Signed-off-by: Daniel Micay <danielmicay@gmail.com>
> > > Cc: Dennis Zhou <dennis@kernel.org>
> > > Cc: Tejun Heo <tj@kernel.org>
> > > Cc: Christoph Lameter <cl@linux.com>
> > > Cc: linux-mm@kvack.org
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > >  include/linux/percpu.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/include/linux/percpu.h b/include/linux/percpu.h
> > > index 5e76af742c80..119f41815b32 100644
> > > --- a/include/linux/percpu.h
> > > +++ b/include/linux/percpu.h
> > > @@ -123,6 +123,7 @@ extern int __init pcpu_page_first_chunk(size_t reserved_size,
> > >  				pcpu_fc_populate_pte_fn_t populate_pte_fn);
> > >  #endif
> > >  
> > > +__alloc_size(1)
> > >  extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align);
> > >  extern bool __is_kernel_percpu_address(unsigned long addr, unsigned long *can_addr);
> > >  extern bool is_kernel_percpu_address(unsigned long addr);
> > > @@ -131,7 +132,9 @@ extern bool is_kernel_percpu_address(unsigned long addr);
> > >  extern void __init setup_per_cpu_areas(void);
> > >  #endif
> > >  
> > > +__alloc_size(1)
> > >  extern void __percpu *__alloc_percpu_gfp(size_t size, size_t align, gfp_t gfp);
> > > +__alloc_size(1)
> > >  extern void __percpu *__alloc_percpu(size_t size, size_t align);
> > >  extern void free_percpu(void __percpu *__pdata);
> > >  extern phys_addr_t per_cpu_ptr_to_phys(void *addr);
> > > -- 
> > > 2.30.2
> > > 
> 
> -- 
> Kees Cook
Andrew Morton Aug. 20, 2021, 5:11 a.m. UTC | #4 
On Wed, 18 Aug 2021 14:40:20 -0700 Kees Cook <keescook@chromium.org> wrote:

> As already done in GrapheneOS, add the __alloc_size attribute for
> appropriate percpu allocator interfaces, to provide additional hinting
> for better bounds checking, assisting CONFIG_FORTIFY_SOURCE and other
> compiler optimizations.
> 

Caught one, I assume:

In file included from ./include/linux/string.h:262,
                 from ./include/linux/bitmap.h:11,
                 from ./include/linux/cpumask.h:12,
                 from ./arch/x86/include/asm/cpumask.h:5,
                 from ./arch/x86/include/asm/msr.h:11,
                 from ./arch/x86/include/asm/processor.h:22,
                 from ./arch/x86/include/asm/cpufeature.h:5,
                 from ./arch/x86/include/asm/thread_info.h:53,
                 from ./include/linux/thread_info.h:60,
                 from ./arch/x86/include/asm/preempt.h:7,
                 from ./include/linux/preempt.h:78,
                 from ./include/linux/spinlock.h:55,
                 from ./include/linux/mmzone.h:8,
                 from ./include/linux/gfp.h:6,
                 from ./include/linux/slab.h:15,
                 from drivers/misc/lkdtm/heap.c:7:
In function 'memset',
    inlined from 'lkdtm_VMALLOC_LINEAR_OVERFLOW' at drivers/misc/lkdtm/heap.c:27:2:
./include/linux/fortify-string.h:172:3: error: call to '__write_overflow' declared with attribute error: detected write beyond size of object passed as 1st parameter
  172 |   __write_overflow();
      |   ^~~~~~~~~~~~~~~~~~
make[3]: *** [drivers/misc/lkdtm/heap.o] Error 1
make[2]: *** [drivers/misc/lkdtm] Error 2
make[1]: *** [drivers/misc] Error 2
make: *** [drivers] Error 2

I want to get a kernel release out, so I'll hide
mm-vmalloc-add-__alloc_size-attributes-for-better-bounds-checking.patch
for now.
Kees Cook Aug. 20, 2021, 5:27 a.m. UTC | #5 
On Thu, Aug 19, 2021 at 10:11:15PM -0700, Andrew Morton wrote:
> On Wed, 18 Aug 2021 14:40:20 -0700 Kees Cook <keescook@chromium.org> wrote:
> 
> > As already done in GrapheneOS, add the __alloc_size attribute for
> > appropriate percpu allocator interfaces, to provide additional hinting
> > for better bounds checking, assisting CONFIG_FORTIFY_SOURCE and other
> > compiler optimizations.
> > 
> 
> Caught one, I assume:
> 
> In file included from ./include/linux/string.h:262,
>                  from ./include/linux/bitmap.h:11,
>                  from ./include/linux/cpumask.h:12,
>                  from ./arch/x86/include/asm/cpumask.h:5,
>                  from ./arch/x86/include/asm/msr.h:11,
>                  from ./arch/x86/include/asm/processor.h:22,
>                  from ./arch/x86/include/asm/cpufeature.h:5,
>                  from ./arch/x86/include/asm/thread_info.h:53,
>                  from ./include/linux/thread_info.h:60,
>                  from ./arch/x86/include/asm/preempt.h:7,
>                  from ./include/linux/preempt.h:78,
>                  from ./include/linux/spinlock.h:55,
>                  from ./include/linux/mmzone.h:8,
>                  from ./include/linux/gfp.h:6,
>                  from ./include/linux/slab.h:15,
>                  from drivers/misc/lkdtm/heap.c:7:
> In function 'memset',
>     inlined from 'lkdtm_VMALLOC_LINEAR_OVERFLOW' at drivers/misc/lkdtm/heap.c:27:2:
> ./include/linux/fortify-string.h:172:3: error: call to '__write_overflow' declared with attribute error: detected write beyond size of object passed as 1st parameter
>   172 |   __write_overflow();
>       |   ^~~~~~~~~~~~~~~~~~
> make[3]: *** [drivers/misc/lkdtm/heap.o] Error 1
> make[2]: *** [drivers/misc/lkdtm] Error 2
> make[1]: *** [drivers/misc] Error 2
> make: *** [drivers] Error 2
> 
> I want to get a kernel release out, so I'll hide
> mm-vmalloc-add-__alloc_size-attributes-for-better-bounds-checking.patch
> for now.

In the cover letter[1], I listed the needed fixes that were sent
separately from this series. Quoting here:

> To build without warnings, this series needs a couple small fixes for
> allmodconfig, which I sent separately:
> https://lore.kernel.org/lkml/20210818174855.2307828-5-keescook@chromium.org/
> https://lore.kernel.org/lkml/20210818044252.1533634-1-keescook@chromium.org/
> https://lore.kernel.org/lkml/20210818043912.1466447-1-keescook@chromium.org/

What you hit is the first one, which is already in Greg's tree:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=e6d468d32cd084edd030a8bae76440b17b854b5c

The other two have also been taken:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-next&id=cbe34165cc1b7d1110b268ba8b9f30843c941639
https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=a31e5a4158d03595ca4258b94397d4097be0ebe4

-Kees

[1] https://lore.kernel.org/lkml/20210818214021.2476230-1-keescook@chromium.org/
diff mbox series

Patch

diff --git a/include/linux/percpu.h b/include/linux/percpu.h
index 5e76af742c80..119f41815b32 100644
--- a/include/linux/percpu.h
+++ b/include/linux/percpu.h
@@ -123,6 +123,7 @@  extern int __init pcpu_page_first_chunk(size_t reserved_size,
 				pcpu_fc_populate_pte_fn_t populate_pte_fn);
 #endif
 
+__alloc_size(1)
 extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align);
 extern bool __is_kernel_percpu_address(unsigned long addr, unsigned long *can_addr);
 extern bool is_kernel_percpu_address(unsigned long addr);
@@ -131,7 +132,9 @@  extern bool is_kernel_percpu_address(unsigned long addr);
 extern void __init setup_per_cpu_areas(void);
 #endif
 
+__alloc_size(1)
 extern void __percpu *__alloc_percpu_gfp(size_t size, size_t align, gfp_t gfp);
+__alloc_size(1)
 extern void __percpu *__alloc_percpu(size_t size, size_t align);
 extern void free_percpu(void __percpu *__pdata);
 extern phys_addr_t per_cpu_ptr_to_phys(void *addr);