[PATCH 0/5] Add __alloc_size() for better bounds checking

[PATCH 0/5] Add __alloc_size() for better bounds checking

[Kees Cook]

Hi,

GCC and Clang both use the "alloc_size" attribute to assist with bounds
checking around the use of allocation functions. Add the attribute,
adjust the Makefile to silence needless warnings, and add the hints to
the allocators where possible. These changes have been in use for a
while now in GrapheneOS.

To build without warnings, this series needs a couple small fixes for
allmodconfig, which I sent separately:
https://lore.kernel.org/lkml/20210818044540.1601664-1-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/

I figure I can take this via my "overflow" series, or it could go via
-mm?

-Kees

Kees Cook (5):
  Compiler Attributes: Add __alloc_size() for better bounds checking
  slab: Add __alloc_size attributes for better bounds checking
  mm/page_alloc: Add __alloc_size attributes for better bounds checking
  percpu: Add __alloc_size attributes for better bounds checking
  mm/vmalloc: Add __alloc_size attributes for better bounds checking

 Makefile                            |  6 +++-
 include/linux/compiler_attributes.h |  6 ++++
 include/linux/gfp.h                 |  4 +--
 include/linux/percpu.h              |  6 ++--
 include/linux/slab.h                | 50 ++++++++++++++++++-----------
 include/linux/vmalloc.h             | 22 ++++++-------
 6 files changed, 58 insertions(+), 36 deletions(-)

-- 
2.30.2

[PATCH 1/5] Compiler Attributes: Add __alloc_size() for better bounds checking

[Kees Cook]

GCC and Clang can use the alloc_size attribute to better inform the
results of __builtin_object_size() (for compile-time constant values).
Clang can additionally use alloc_size to informt the results of
__builtin_dynamic_object_size() (for run-time values).

Additionally disables -Wno-alloc-size-larger-than since the allocators
already reject SIZE_MAX, and the compile-time warnings aren't helpful.

Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: clang-built-linux@googlegroups.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Makefile                            | 6 +++++-
 include/linux/compiler_attributes.h | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1b238ce86ed4..3b6fb740584e 100644
--- a/Makefile
+++ b/Makefile
@@ -1076,9 +1076,13 @@ KBUILD_CFLAGS += $(call cc-disable-warning, stringop-overflow)
 # Another good warning that we'll want to enable eventually
 KBUILD_CFLAGS += $(call cc-disable-warning, restrict)
 
-# Enabled with W=2, disabled by default as noisy
 ifdef CONFIG_CC_IS_GCC
+# Enabled with W=2, disabled by default as noisy
 KBUILD_CFLAGS += -Wno-maybe-uninitialized
+
+# The allocators already balk at large sizes, so silence the compiler
+# warnings for bounds checks involving those possible values.
+KBUILD_CFLAGS += -Wno-alloc-size-larger-than
 endif
 
 # disable invalid "can't wrap" optimizations for signed / pointers
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 67c5667f8042..203b0ac62d15 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -54,6 +54,12 @@
 #define __aligned(x)                    __attribute__((__aligned__(x)))
 #define __aligned_largest               __attribute__((__aligned__))
 
+/*
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
+ */
+#define __alloc_size(x, ...)		__attribute__((__alloc_size__(x, ## __VA_ARGS__)))
+
 /*
  * Note: users of __always_inline currently do not write "inline" themselves,
  * which seems to be required by gcc to apply the attribute according
-- 
2.30.2

[PATCH 2/5] slab: Add __alloc_size attributes for better bounds checking

[Kees Cook]

As already done in GrapheneOS, add the __alloc_size attribute for
regular kmalloc 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: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: linux-mm@kvack.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/slab.h | 50 +++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index c0d46b6fa12a..b2181c176999 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -181,7 +181,7 @@ int kmem_cache_shrink(struct kmem_cache *);
 /*
  * Common kmalloc functions provided by all allocators
  */
-void * __must_check krealloc(const void *, size_t, gfp_t);
+void * __must_check krealloc(const void *, size_t, gfp_t) __alloc_size(2);
 void kfree(const void *);
 void kfree_sensitive(const void *);
 size_t __ksize(const void *);
@@ -425,7 +425,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
 #define kmalloc_index(s) __kmalloc_index(s, true)
 #endif /* !CONFIG_SLOB */
 
-void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __malloc;
+void *__kmalloc(size_t size, gfp_t flags) __alloc_size(1) __assume_kmalloc_alignment __malloc;
 void *kmem_cache_alloc(struct kmem_cache *, gfp_t flags) __assume_slab_alignment __malloc;
 void kmem_cache_free(struct kmem_cache *, void *);
 
@@ -449,7 +449,8 @@ static __always_inline void kfree_bulk(size_t size, void **p)
 }
 
 #ifdef CONFIG_NUMA
-void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_kmalloc_alignment __malloc;
+void *__kmalloc_node(size_t size, gfp_t flags, int node) __alloc_size(1)
+							 __assume_kmalloc_alignment __malloc;
 void *kmem_cache_alloc_node(struct kmem_cache *, gfp_t flags, int node) __assume_slab_alignment __malloc;
 #else
 static __always_inline void *__kmalloc_node(size_t size, gfp_t flags, int node)
@@ -574,7 +575,7 @@ static __always_inline void *kmalloc_large(size_t size, gfp_t flags)
  *	Try really hard to succeed the allocation but fail
  *	eventually.
  */
-static __always_inline void *kmalloc(size_t size, gfp_t flags)
+static __always_inline __alloc_size(1) void *kmalloc(size_t size, gfp_t flags)
 {
 	if (__builtin_constant_p(size)) {
 #ifndef CONFIG_SLOB
@@ -596,7 +597,8 @@ static __always_inline void *kmalloc(size_t size, gfp_t flags)
 	return __kmalloc(size, flags);
 }
 
-static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
+static __always_inline __alloc_size(1) void *
+kmalloc_node(size_t size, gfp_t flags, int node)
 {
 #ifndef CONFIG_SLOB
 	if (__builtin_constant_p(size) &&
@@ -620,7 +622,8 @@ static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
  * @size: element size.
  * @flags: the type of memory to allocate (see kmalloc).
  */
-static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags)
+static inline __alloc_size(1, 2) void *
+kmalloc_array(size_t n, size_t size, gfp_t flags)
 {
 	size_t bytes;
 
@@ -638,7 +641,7 @@ static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags)
  * @new_size: new size of a single member of the array
  * @flags: the type of memory to allocate (see kmalloc)
  */
-static __must_check inline void *
+static __must_check inline __alloc_size(2, 3) void *
 krealloc_array(void *p, size_t new_n, size_t new_size, gfp_t flags)
 {
 	size_t bytes;
@@ -655,7 +658,8 @@ krealloc_array(void *p, size_t new_n, size_t new_size, gfp_t flags)
  * @size: element size.
  * @flags: the type of memory to allocate (see kmalloc).
  */
-static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
+static inline __alloc_size(1, 2) void *
+kcalloc(size_t n, size_t size, gfp_t flags)
 {
 	return kmalloc_array(n, size, flags | __GFP_ZERO);
 }
@@ -684,7 +688,8 @@ static inline void *kmalloc_array_node(size_t n, size_t size, gfp_t flags,
 	return __kmalloc_node(bytes, flags, node);
 }
 
-static inline void *kcalloc_node(size_t n, size_t size, gfp_t flags, int node)
+static inline __alloc_size(1, 2) void *
+kcalloc_node(size_t n, size_t size, gfp_t flags, int node)
 {
 	return kmalloc_array_node(n, size, flags | __GFP_ZERO, node);
 }
@@ -716,7 +721,8 @@ static inline void *kmem_cache_zalloc(struct kmem_cache *k, gfp_t flags)
  * @size: how many bytes of memory are required.
  * @flags: the type of memory to allocate (see kmalloc).
  */
-static inline void *kzalloc(size_t size, gfp_t flags)
+static inline __alloc_size(1) void *
+kzalloc(size_t size, gfp_t flags)
 {
 	return kmalloc(size, flags | __GFP_ZERO);
 }
@@ -727,26 +733,31 @@ static inline void *kzalloc(size_t size, gfp_t flags)
  * @flags: the type of memory to allocate (see kmalloc).
  * @node: memory node from which to allocate
  */
-static inline void *kzalloc_node(size_t size, gfp_t flags, int node)
+static inline __alloc_size(1) void *
+kzalloc_node(size_t size, gfp_t flags, int node)
 {
 	return kmalloc_node(size, flags | __GFP_ZERO, node);
 }
 
-extern void *kvmalloc_node(size_t size, gfp_t flags, int node);
-static inline void *kvmalloc(size_t size, gfp_t flags)
+extern __alloc_size(1) void *
+kvmalloc_node(size_t size, gfp_t flags, int node);
+static inline __alloc_size(1) void *kvmalloc(size_t size, gfp_t flags)
 {
 	return kvmalloc_node(size, flags, NUMA_NO_NODE);
 }
-static inline void *kvzalloc_node(size_t size, gfp_t flags, int node)
+static inline __alloc_size(1) void *
+kvzalloc_node(size_t size, gfp_t flags, int node)
 {
 	return kvmalloc_node(size, flags | __GFP_ZERO, node);
 }
-static inline void *kvzalloc(size_t size, gfp_t flags)
+static inline __alloc_size(1) void *
+kvzalloc(size_t size, gfp_t flags)
 {
 	return kvmalloc(size, flags | __GFP_ZERO);
 }
 
-static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
+static inline __alloc_size(1, 2) void *
+kvmalloc_array(size_t n, size_t size, gfp_t flags)
 {
 	size_t bytes;
 
@@ -756,13 +767,14 @@ static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
 	return kvmalloc(bytes, flags);
 }
 
-static inline void *kvcalloc(size_t n, size_t size, gfp_t flags)
+static inline __alloc_size(1, 2) void *
+kvcalloc(size_t n, size_t size, gfp_t flags)
 {
 	return kvmalloc_array(n, size, flags | __GFP_ZERO);
 }
 
-extern void *kvrealloc(const void *p, size_t oldsize, size_t newsize,
-		gfp_t flags);
+extern __alloc_size(3) void *
+kvrealloc(const void *p, size_t oldsize, size_t newsize, gfp_t flags);
 extern void kvfree(const void *addr);
 extern void kvfree_sensitive(const void *addr, size_t len);
 
-- 
2.30.2

[PATCH 3/5] mm/page_alloc: Add __alloc_size attributes for better bounds checking

[Kees Cook]

As already done in GrapheneOS, add the __alloc_size attribute for
appropriate page 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: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/gfp.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 3745efd21cf6..94e57c752308 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -618,9 +618,9 @@ static inline struct folio *folio_alloc(gfp_t gfp, unsigned int order)
 extern unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order);
 extern unsigned long get_zeroed_page(gfp_t gfp_mask);
 
-void *alloc_pages_exact(size_t size, gfp_t gfp_mask);
+void *alloc_pages_exact(size_t size, gfp_t gfp_mask) __alloc_size(1);
 void free_pages_exact(void *virt, size_t size);
-void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask);
+void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask) __alloc_size(1);
 
 #define __get_free_page(gfp_mask) \
 		__get_free_pages((gfp_mask), 0)
-- 
2.30.2

[PATCH 4/5] percpu: Add __alloc_size attributes for better bounds checking

[Kees Cook]

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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/percpu.h b/include/linux/percpu.h
index 5e76af742c80..98a9371133f8 100644
--- a/include/linux/percpu.h
+++ b/include/linux/percpu.h
@@ -123,7 +123,7 @@ extern int __init pcpu_page_first_chunk(size_t reserved_size,
 				pcpu_fc_populate_pte_fn_t populate_pte_fn);
 #endif
 
-extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align);
+extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align) __alloc_size(1);
 extern bool __is_kernel_percpu_address(unsigned long addr, unsigned long *can_addr);
 extern bool is_kernel_percpu_address(unsigned long addr);
 
@@ -131,8 +131,8 @@ extern bool is_kernel_percpu_address(unsigned long addr);
 extern void __init setup_per_cpu_areas(void);
 #endif
 
-extern void __percpu *__alloc_percpu_gfp(size_t size, size_t align, gfp_t gfp);
-extern void __percpu *__alloc_percpu(size_t size, size_t align);
+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) __alloc_size(1);
 extern void free_percpu(void __percpu *__pdata);
 extern phys_addr_t per_cpu_ptr_to_phys(void *addr);
 
-- 
2.30.2

[PATCH 5/5] mm/vmalloc: Add __alloc_size attributes for better bounds checking

[Kees Cook]

As already done in GrapheneOS, add the __alloc_size attribute for
appropriate vmalloc 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: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/vmalloc.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 2644425b6dce..f4ede07e1dae 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -136,21 +136,21 @@ static inline void vmalloc_init(void)
 static inline unsigned long vmalloc_nr_pages(void) { return 0; }
 #endif
 
-extern void *vmalloc(unsigned long size);
-extern void *vzalloc(unsigned long size);
-extern void *vmalloc_user(unsigned long size);
-extern void *vmalloc_node(unsigned long size, int node);
-extern void *vzalloc_node(unsigned long size, int node);
-extern void *vmalloc_32(unsigned long size);
-extern void *vmalloc_32_user(unsigned long size);
-extern void *__vmalloc(unsigned long size, gfp_t gfp_mask);
+extern void *vmalloc(unsigned long size) __alloc_size(1);
+extern void *vzalloc(unsigned long size) __alloc_size(1);
+extern void *vmalloc_user(unsigned long size) __alloc_size(1);
+extern void *vmalloc_node(unsigned long size, int node) __alloc_size(1);
+extern void *vzalloc_node(unsigned long size, int node) __alloc_size(1);
+extern void *vmalloc_32(unsigned long size) __alloc_size(1);
+extern void *vmalloc_32_user(unsigned long size) __alloc_size(1);
+extern void *__vmalloc(unsigned long size, gfp_t gfp_mask) __alloc_size(1);
 extern void *__vmalloc_node_range(unsigned long size, unsigned long align,
 			unsigned long start, unsigned long end, gfp_t gfp_mask,
 			pgprot_t prot, unsigned long vm_flags, int node,
-			const void *caller);
+			const void *caller) __alloc_size(1);
 void *__vmalloc_node(unsigned long size, unsigned long align, gfp_t gfp_mask,
-		int node, const void *caller);
-void *vmalloc_no_huge(unsigned long size);
+		int node, const void *caller) __alloc_size(1);
+void *vmalloc_no_huge(unsigned long size) __alloc_size(1);
 
 extern void vfree(const void *addr);
 extern void vfree_atomic(const void *addr);
-- 
2.30.2

Re: [PATCH 2/5] slab: Add __alloc_size attributes for better bounds checking

[Joe Perches]

On Tue, 2021-08-17 at 22:08 -0700, Kees Cook wrote:
> As already done in GrapheneOS, add the __alloc_size attribute for
> regular kmalloc interfaces, to provide additional hinting for better
> bounds checking, assisting CONFIG_FORTIFY_SOURCE and other compiler
> optimizations.
[]
> diff --git a/include/linux/slab.h b/include/linux/slab.h
[]
> @@ -181,7 +181,7 @@ int kmem_cache_shrink(struct kmem_cache *);
>  /*
>   * Common kmalloc functions provided by all allocators
>   */
> -void * __must_check krealloc(const void *, size_t, gfp_t);
> +void * __must_check krealloc(const void *, size_t, gfp_t) __alloc_size(2);

I suggest the __alloc_size attribute be placed at the beginning of the
function declaration to be more similar to the common __printf attribute
location uses.

__alloc_size(2)
void * __must_check krealloc(const void *, size_t, gfp_t);

I really prefer the __must_check to be with the other attribute and that
function declarations have argument names too like:

__alloc_size(2) __must_check
void *krealloc(const void *ptr, size_t size, gfp_t gfp);

but there are a _lot_ of placement of __must_check after the return type

Lastly __alloc_size should probably be added to checkpatch

Maybe:
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 161ce7fe5d1e5..1a166b5cf3447 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -489,7 +489,8 @@ our $Attribute	= qr{
 			____cacheline_aligned|
 			____cacheline_aligned_in_smp|
 			____cacheline_internodealigned_in_smp|
-			__weak
+			__weak|
+			__alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\)
 		  }x;
 our $Modifier;
 our $Inline	= qr{inline|__always_inline|noinline|__inline|__inline__};

Re: [PATCH 2/5] slab: Add __alloc_size attributes for better bounds checking

[Kees Cook]

On Tue, Aug 17, 2021 at 10:31:32PM -0700, Joe Perches wrote:
> On Tue, 2021-08-17 at 22:08 -0700, Kees Cook wrote:
> > As already done in GrapheneOS, add the __alloc_size attribute for
> > regular kmalloc interfaces, to provide additional hinting for better
> > bounds checking, assisting CONFIG_FORTIFY_SOURCE and other compiler
> > optimizations.
> []
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> []
> > @@ -181,7 +181,7 @@ int kmem_cache_shrink(struct kmem_cache *);
> >  /*
> >   * Common kmalloc functions provided by all allocators
> >   */
> > -void * __must_check krealloc(const void *, size_t, gfp_t);
> > +void * __must_check krealloc(const void *, size_t, gfp_t) __alloc_size(2);
> 
> I suggest the __alloc_size attribute be placed at the beginning of the
> function declaration to be more similar to the common __printf attribute
> location uses.

Yeah, any consistent ordering suggestions are welcome here; thank you!
These declarations were all over the place, and trying to follow each
slightly different existing style made my eyes hurt. :)

> __alloc_size(2)
> void * __must_check krealloc(const void *, size_t, gfp_t);
> 
> I really prefer the __must_check to be with the other attribute and that
> function declarations have argument names too like:
> 
> __alloc_size(2) __must_check
> void *krealloc(const void *ptr, size_t size, gfp_t gfp);

I'm happy with whatever makes the most sense.

> but there are a _lot_ of placement of __must_check after the return type
> 
> Lastly __alloc_size should probably be added to checkpatch

Oh, yes! Thanks for the reminder.

> Maybe:
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 161ce7fe5d1e5..1a166b5cf3447 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -489,7 +489,8 @@ our $Attribute	= qr{
>  			____cacheline_aligned|
>  			____cacheline_aligned_in_smp|
>  			____cacheline_internodealigned_in_smp|
> -			__weak
> +			__weak|
> +			__alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\)

Why the "{0,5}" bit here? I was expecting just "?". (i.e. it can have
either 1 or 2 arguments.)

>  		  }x;
>  our $Modifier;
>  our $Inline	= qr{inline|__always_inline|noinline|__inline|__inline__};
> 
> 

-- 
Kees Cook

Re: [PATCH 2/5] slab: Add __alloc_size attributes for better bounds checking

[Joe Perches]

On Tue, 2021-08-17 at 23:16 -0700, Kees Cook wrote:
> On Tue, Aug 17, 2021 at 10:31:32PM -0700, Joe Perches wrote:
> > On Tue, 2021-08-17 at 22:08 -0700, Kees Cook wrote:
> > > As already done in GrapheneOS, add the __alloc_size attribute for
> > > regular kmalloc interfaces, to provide additional hinting for better
> > > bounds checking, assisting CONFIG_FORTIFY_SOURCE and other compiler
> > > optimizations.
[]
> > Lastly __alloc_size should probably be added to checkpatch
> 
> Oh, yes! Thanks for the reminder.
> 
> > Maybe:
> > ---
> >  scripts/checkpatch.pl | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 161ce7fe5d1e5..1a166b5cf3447 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -489,7 +489,8 @@ our $Attribute	= qr{
> >  			____cacheline_aligned|
> >  			____cacheline_aligned_in_smp|
> >  			____cacheline_internodealigned_in_smp|
> > -			__weak
> > +			__weak|
> > +			__alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\)
> 
> Why the "{0,5}" bit here? I was expecting just "?". (i.e. it can have
> either 1 or 2 arguments.)

You are right.  I misread the doc.  I also missed a \ before the last d.

So that last added line should maybe be: (totally untested btw)

+			__alloc_size\s*\(\s*\d+\s*(?:,\s*\d+\s*)?\)

Re: [PATCH 1/5] Compiler Attributes: Add __alloc_size() for better bounds checking

[Miguel Ojeda]

On Wed, Aug 18, 2021 at 7:08 AM Kees Cook <keescook@chromium.org> wrote:
>
> Clang can additionally use alloc_size to informt the results of

Typo.

> Additionally disables -Wno-alloc-size-larger-than since the allocators

Disables -Walloc-size-larger-than?

> already reject SIZE_MAX, and the compile-time warnings aren't helpful.

Perhaps a bit more context here (and/or in the comment in the
Makefile) would be nice: i.e. why are they not helpful (even if
rejected by the allocators).

Cheers,
Miguel

Re: [PATCH 1/5] Compiler Attributes: Add __alloc_size() for better bounds checking

[Kees Cook]

On Wed, Aug 18, 2021 at 03:07:48PM +0200, Miguel Ojeda wrote:
> On Wed, Aug 18, 2021 at 7:08 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > Clang can additionally use alloc_size to informt the results of
> 
> Typo.
> 
> > Additionally disables -Wno-alloc-size-larger-than since the allocators
> 
> Disables -Walloc-size-larger-than?
> 
> > already reject SIZE_MAX, and the compile-time warnings aren't helpful.
> 
> Perhaps a bit more context here (and/or in the comment in the
> Makefile) would be nice: i.e. why are they not helpful (even if
> rejected by the allocators).

Thanks for the review! I'll get this all fixed for v2.

-- 
Kees Cook

Re: [PATCH 1/5] Compiler Attributes: Add __alloc_size() for better bounds checking

[Nathan Chancellor]

On 8/17/2021 10:08 PM, Kees Cook wrote:
> GCC and Clang can use the alloc_size attribute to better inform the
> results of __builtin_object_size() (for compile-time constant values).
> Clang can additionally use alloc_size to informt the results of
> __builtin_dynamic_object_size() (for run-time values).
> 
> Additionally disables -Wno-alloc-size-larger-than since the allocators
> already reject SIZE_MAX, and the compile-time warnings aren't helpful.

In addition to what Miguel said, it might be helpful to mention that 
this warning is GCC specific, I was a little confused at first as to why 
it was just being added in the GCC only block :)

Otherwise, the attribute addition looks good to me. I will add my tag on v2.

> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: clang-built-linux@googlegroups.com
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   Makefile                            | 6 +++++-
>   include/linux/compiler_attributes.h | 6 ++++++
>   2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 1b238ce86ed4..3b6fb740584e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1076,9 +1076,13 @@ KBUILD_CFLAGS += $(call cc-disable-warning, stringop-overflow)
>   # Another good warning that we'll want to enable eventually
>   KBUILD_CFLAGS += $(call cc-disable-warning, restrict)
>   
> -# Enabled with W=2, disabled by default as noisy
>   ifdef CONFIG_CC_IS_GCC
> +# Enabled with W=2, disabled by default as noisy
>   KBUILD_CFLAGS += -Wno-maybe-uninitialized
> +
> +# The allocators already balk at large sizes, so silence the compiler
> +# warnings for bounds checks involving those possible values.
> +KBUILD_CFLAGS += -Wno-alloc-size-larger-than
>   endif
>   
>   # disable invalid "can't wrap" optimizations for signed / pointers
> diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> index 67c5667f8042..203b0ac62d15 100644
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -54,6 +54,12 @@
>   #define __aligned(x)                    __attribute__((__aligned__(x)))
>   #define __aligned_largest               __attribute__((__aligned__))
>   
> +/*
> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
> + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
> + */
> +#define __alloc_size(x, ...)		__attribute__((__alloc_size__(x, ## __VA_ARGS__)))
> +
>   /*
>    * Note: users of __always_inline currently do not write "inline" themselves,
>    * which seems to be required by gcc to apply the attribute according
> 

Re: [PATCH 1/5] Compiler Attributes: Add __alloc_size() for better bounds checking

[Kees Cook]

On Wed, Aug 18, 2021 at 11:04:32AM -0700, Nathan Chancellor wrote:
> On 8/17/2021 10:08 PM, Kees Cook wrote:
> > GCC and Clang can use the alloc_size attribute to better inform the
> > results of __builtin_object_size() (for compile-time constant values).
> > Clang can additionally use alloc_size to informt the results of
> > __builtin_dynamic_object_size() (for run-time values).
> > 
> > Additionally disables -Wno-alloc-size-larger-than since the allocators
> > already reject SIZE_MAX, and the compile-time warnings aren't helpful.
> 
> In addition to what Miguel said, it might be helpful to mention that this
> warning is GCC specific, I was a little confused at first as to why it was
> just being added in the GCC only block :)

Yes, good point. I'll call it out in particular.

> Otherwise, the attribute addition looks good to me. I will add my tag on v2.

Thanks!

-- 
Kees Cook

Re: [PATCH 2/5] slab: Add __alloc_size attributes for better bounds checking

[Matthew Wilcox]

On Tue, Aug 17, 2021 at 10:31:32PM -0700, Joe Perches wrote:
> Lastly __alloc_size should probably be added to checkpatch
> 
> Maybe:
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 161ce7fe5d1e5..1a166b5cf3447 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -489,7 +489,8 @@ our $Attribute	= qr{
>  			____cacheline_aligned|
>  			____cacheline_aligned_in_smp|
>  			____cacheline_internodealigned_in_smp|
> -			__weak
> +			__weak|
> +			__alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\)

Should probably be added to kernel-doc as well.  Any other awful regexes
that need to be changed to understand it?  And can we commonise the
regexes that do exist into a perl helper library?

Re: [PATCH 2/5] slab: Add __alloc_size attributes for better bounds checking

[Joe Perches]

On Thu, 2021-08-19 at 01:27 +0100, Matthew Wilcox wrote:
> On Tue, Aug 17, 2021 at 10:31:32PM -0700, Joe Perches wrote:
> > Lastly __alloc_size should probably be added to checkpatch
> > 
> > Maybe:
> > ---
> >  scripts/checkpatch.pl | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > @@ -489,7 +489,8 @@ our $Attribute	= qr{
> >  			____cacheline_aligned|
> >  			____cacheline_aligned_in_smp|
> >  			____cacheline_internodealigned_in_smp|
> > -			__weak
> > +			__weak|
> > +			__alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\)
> 
> Should probably be added to kernel-doc as well.  Any other awful regexes
> that need to be changed to understand it?  And can we commonise the
> regexes that do exist into a perl helper library?

probably, but there would need to be some library work done and
changes made to both utilities so they could use the same $helpers.

And there are several nominally incomplete regexes already in
kernel-doc and I'm not at all familiar with kernel-doc.

e.g.: kernel-doc has:

my $attribute = qr{__attribute__\s*\(\([a-z0-9,_\*\s\(\)]*\)\)}i;

but __attribute__ can have quotes like:

__attribute__((section("foo")))

and spaces around and and I believe between (( and )) like:

__attribute__ ((packed))

so those wouldn't match.

The use of parentheses internal to attributes like __align__(8) may
not work particularly well either given greedy matching.

Re: [PATCH 2/5] slab: Add __alloc_size attributes for better bounds checking

[Matthew Wilcox]

On Wed, Aug 18, 2021 at 06:10:57PM -0700, Joe Perches wrote:
> On Thu, 2021-08-19 at 01:27 +0100, Matthew Wilcox wrote:
> > On Tue, Aug 17, 2021 at 10:31:32PM -0700, Joe Perches wrote:
> > > Lastly __alloc_size should probably be added to checkpatch
> > > 
> > > Maybe:
> > > ---
> > >  scripts/checkpatch.pl | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > > @@ -489,7 +489,8 @@ our $Attribute	= qr{
> > >  			____cacheline_aligned|
> > >  			____cacheline_aligned_in_smp|
> > >  			____cacheline_internodealigned_in_smp|
> > > -			__weak
> > > +			__weak|
> > > +			__alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\)
> > 
> > Should probably be added to kernel-doc as well.  Any other awful regexes
> > that need to be changed to understand it?  And can we commonise the
> > regexes that do exist into a perl helper library?
> 
> probably, but there would need to be some library work done and
> changes made to both utilities so they could use the same $helpers.
> 
> And there are several nominally incomplete regexes already in
> kernel-doc and I'm not at all familiar with kernel-doc.

Yes, kernel-doc is an awful example of perl gone wild.

Re: [PATCH 2/5] slab: Add __alloc_size attributes for better bounds checking

[Joe Perches]

On Thu, 2021-08-19 at 03:16 +0100, Matthew Wilcox wrote:

> kernel-doc is an awful example of perl gone wild.

And checkpatch isn't?

Re: [PATCH 0/5] Add __alloc_size() for better bounds checking

[Christoph Hellwig]

On Tue, Aug 17, 2021 at 10:08:36PM -0700, Kees Cook wrote:
> Hi,
> 
> GCC and Clang both use the "alloc_size" attribute to assist with bounds
> checking around the use of allocation functions. Add the attribute,
> adjust the Makefile to silence needless warnings, and add the hints to
> the allocators where possible. These changes have been in use for a
> while now in GrapheneOS.

Can you explain how this attribute helps?  Should we flow it through
other allocating functions?

Re: [PATCH 0/5] Add __alloc_size() for better bounds checking

[Daniel Micay]

It tells the compiler the function will either return NULL or an
allocation of the size specific by the parameter referenced by
alloc_size. It could also be used for functions resembling allocation
functions which aren't actually allocating. The compiler will use it
for optimization so it's extremely important that it's only used
correctly. It only really has a use on the top-level API used
externally.

The compiler uses it for __builtin_object_size which is primarily used
by FORTIFY_SOURCE and also internally by -fsanitize=object-size which
will be available for the kernel via UBSan to find bugs or as
hardening in the trapping mode. There are currently compatibility
issues (undefined out-of-bounds accesses) blocking using
-fsanitize=object-size beyond fixing those relatively benign issues to
allow using it elsewhere.

For example, it will know that kmalloc(n) returns either NULL or an
allocation of size n. A simple sample program with calloc in
userspace:

    #include <stdlib.h>
    #include <stdio.h>

    int main(void) {
        char *p = calloc(64, 1);
        if (!p) {
            return 1;
        }
        printf("%zu\n", __builtin_object_size(p, 1));
        return 0;
    }

It will also detect an out-of-bounds access via the allocation with
-fsanitize=object-size including with a runtime value as the index.

It's not as useful as it should be yet because __builtin_object_size
must return a compile-time constant. Clang has a new
__builtin_dynamic_object_size that's allowed to return a value that's
not a compile-time constant so it can work for kmalloc(n) where n is a
runtime value. It might not be quite ready for use yet but it should
be able to make it a lot more useful. GCC also seems open to adding it
too.

Re: [PATCH 0/5] Add __alloc_size() for better bounds checking

[Christoph Lameter]

On Thu, 19 Aug 2021, Daniel Micay wrote:

> For example, it will know that kmalloc(n) returns either NULL or an
> allocation of size n. A simple sample program with calloc in
> userspace:
>
>     #include <stdlib.h>
>     #include <stdio.h>
>
>     int main(void) {
>         char *p = calloc(64, 1);
>         if (!p) {
>             return 1;
>         }
>         printf("%zu\n", __builtin_object_size(p, 1));
>         return 0;
>     }
>
> It will also detect an out-of-bounds access via the allocation with
> -fsanitize=object-size including with a runtime value as the index.
>
> It's not as useful as it should be yet because __builtin_object_size
> must return a compile-time constant. Clang has a new
> __builtin_dynamic_object_size that's allowed to return a value that's
> not a compile-time constant so it can work for kmalloc(n) where n is a
> runtime value. It might not be quite ready for use yet but it should
> be able to make it a lot more useful. GCC also seems open to adding it
> too.

The other complication with kmalloc etc is that the slab allocators may
decided to allocate more bytes than needed because it does not support
that particular allocation size. Some functions check the allocated true
size and make use of that. See ksize().

Re: [PATCH 0/5] Add __alloc_size() for better bounds checking

[Kees Cook]

On Wed, Aug 25, 2021 at 12:01:42PM +0200, Christoph Lameter wrote:
> On Thu, 19 Aug 2021, Daniel Micay wrote:
> 
> > For example, it will know that kmalloc(n) returns either NULL or an
> > allocation of size n. A simple sample program with calloc in
> > userspace:
> >
> >     #include <stdlib.h>
> >     #include <stdio.h>
> >
> >     int main(void) {
> >         char *p = calloc(64, 1);
> >         if (!p) {
> >             return 1;
> >         }
> >         printf("%zu\n", __builtin_object_size(p, 1));
> >         return 0;
> >     }
> >
> > It will also detect an out-of-bounds access via the allocation with
> > -fsanitize=object-size including with a runtime value as the index.
> >
> > It's not as useful as it should be yet because __builtin_object_size
> > must return a compile-time constant. Clang has a new
> > __builtin_dynamic_object_size that's allowed to return a value that's
> > not a compile-time constant so it can work for kmalloc(n) where n is a
> > runtime value. It might not be quite ready for use yet but it should
> > be able to make it a lot more useful. GCC also seems open to adding it
> > too.
> 
> The other complication with kmalloc etc is that the slab allocators may
> decided to allocate more bytes than needed because it does not support
> that particular allocation size. Some functions check the allocated true
> size and make use of that. See ksize().

Yup, this is known. For the current iteration, this doesn't pose a
problem since the compile-time checking has very limited scope.

-- 
Kees Cook