[PATCH 0/5] Annotate allocation functions with alloc_size attribute

[PATCH 0/5] Annotate allocation functions with alloc_size attribute

[Daniel Axtens]

Both gcc and clang support the 'alloc_size' function attribute. It tells
the compiler that a function returns a pointer to a certain amount of
memory.

This series tries applying that attribute to a number of our memory
allocation functions. This provides much more information to things that
use __builtin_object_size() (FORTIFY_SOURCE and some copy_to/from_user
stuff), as well as enhancing inlining opportunities where __builtin_mem* or
__builtin_str* are used.

With this series, FORTIFY_SOURCE picks up a bug in altera-stapl, which is
fixed in patch 1.

It also generates a bunch of warnings about times memory allocation
functions can be called with SIZE_MAX as the parameter. For example, from
patch 3:

drivers/staging/rts5208/rtsx_chip.c: In function ‘rtsx_write_cfg_seq’:
drivers/staging/rts5208/rtsx_chip.c:1453:7: warning: argument 1 value ‘18446744073709551615’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
  data = vzalloc(array_size(dw_len, 4));
  ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The parameter to array_size is a size_t, but it is called with a signed
integer argument. If the argument is a negative integer, it will become a
very large positive number when cast to size_t. This could cause an
overflow, so array_size() will return SIZE_MAX _at compile time_. gcc then
notices that this value is too large for an allocation and throws a
warning.

I propose two ways to deal with this:

 - Individually go through and address these warnings, usualy by
   catching when struct_size/array_size etc are called with a signed
   type, and insert bounds checks or change the type where
   appropriate. Patch 3 is an example.

 - Patch 4: make kmalloc(_node) catch SIZE_MAX and return NULL early,
   preventing an annotated kmalloc-family allocation function from seeing
   SIZE_MAX as a parameter.

I'm not sure whether I like the idea of catching SIZE_MAX in the inlined
functions. Here are some pros and cons, and I'd be really interested to
hear feedback:

 * Making kmalloc return NULL early doesn't change _runtime_ behaviour:
   obviously no SIZE_MAX allocation will ever succeed. And it means we
   could have this feature earlier, which will help to catch issues like
   what we catch in altera-stapl.

 * However, it does mean we don't audit callsites where perhaps we should
   have stricter types or bounds-checking. It also doesn't cover any of the
   v*alloc functions.

Overall I think this is a meaningful strengthening of FORTIFY_SOURCE
and worth pursuing.

Daniel Axtens (5):
  altera-stapl: altera_get_note: prevent write beyond end of 'key'
  [RFC] kasan: kasan_test: hide allocation sizes from the compiler
  [RFC] staging: rts5208: make len a u16 in rtsx_write_cfg_seq
  [VERY RFC] mm: kmalloc(_node): return NULL immediately for SIZE_MAX
  [RFC] mm: annotate memory allocation functions with their sizes

 drivers/misc/altera-stapl/altera.c  | 12 ++++----
 drivers/staging/rts5208/rtsx_chip.c |  2 +-
 drivers/staging/rts5208/rtsx_chip.h |  2 +-
 include/linux/compiler_attributes.h |  6 ++++
 include/linux/kasan.h               | 12 ++++----
 include/linux/slab.h                | 44 +++++++++++++++++---------
 include/linux/vmalloc.h             | 26 ++++++++--------
 lib/test_kasan.c                    | 48 +++++++++++++++++++++--------
 8 files changed, 98 insertions(+), 54 deletions(-)

-- 
2.20.1

[PATCH 1/5] altera-stapl: altera_get_note: prevent write beyond end of 'key'

[Daniel Axtens]

altera_get_note is called from altera_init, where key is kzalloc(33).

When the allocation functions are annotated to allow the compiler to see
the sizes of objects, and with FORTIFY_SOURCE, we see:

In file included from drivers/misc/altera-stapl/altera.c:14:0:
In function ‘strlcpy’,
    inlined from ‘altera_init’ at drivers/misc/altera-stapl/altera.c:2189:5:
include/linux/string.h:378:4: error: call to ‘__write_overflow’ declared with attribute error: detected write beyond size of object passed as 1st parameter
    __write_overflow();
    ^~~~~~~~~~~~~~~~~~

That refers to this code in altera_get_note:

    if (key != NULL)
            strlcpy(key, &p[note_strings +
                            get_unaligned_be32(
                            &p[note_table + (8 * i)])],
                    length);

The error triggers because the length of 'key' is 33, but the copy
uses length supplied as the 'length' parameter, which is always
256. Split the size parameter into key_len and val_len, and use the
appropriate length depending on what is being copied.

Detected by compiler error, only compile-tested.

Cc: "Igor M. Liplianin" <liplianin@netup.ru>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 drivers/misc/altera-stapl/altera.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/altera-stapl/altera.c b/drivers/misc/altera-stapl/altera.c
index 25e5f24b3fec..5bdf57472314 100644
--- a/drivers/misc/altera-stapl/altera.c
+++ b/drivers/misc/altera-stapl/altera.c
@@ -2112,8 +2112,8 @@ static int altera_execute(struct altera_state *astate,
 	return status;
 }
 
-static int altera_get_note(u8 *p, s32 program_size,
-			s32 *offset, char *key, char *value, int length)
+static int altera_get_note(u8 *p, s32 program_size, s32 *offset,
+			   char *key, char *value, int keylen, int vallen)
 /*
  * Gets key and value of NOTE fields in the JBC file.
  * Can be called in two modes:  if offset pointer is NULL,
@@ -2170,7 +2170,7 @@ static int altera_get_note(u8 *p, s32 program_size,
 						&p[note_table + (8 * i) + 4])];
 
 				if (value != NULL)
-					strlcpy(value, value_ptr, length);
+					strlcpy(value, value_ptr, vallen);
 
 			}
 		}
@@ -2189,13 +2189,13 @@ static int altera_get_note(u8 *p, s32 program_size,
 				strlcpy(key, &p[note_strings +
 						get_unaligned_be32(
 						&p[note_table + (8 * i)])],
-					length);
+					keylen);
 
 			if (value != NULL)
 				strlcpy(value, &p[note_strings +
 						get_unaligned_be32(
 						&p[note_table + (8 * i) + 4])],
-					length);
+					vallen);
 
 			*offset = i + 1;
 		}
@@ -2449,7 +2449,7 @@ int altera_init(struct altera_config *config, const struct firmware *fw)
 			__func__, (format_version == 2) ? "Jam STAPL" :
 						"pre-standardized Jam 1.1");
 		while (altera_get_note((u8 *)fw->data, fw->size,
-					&offset, key, value, 256) == 0)
+					&offset, key, value, 32, 256) == 0)
 			printk(KERN_INFO "%s: NOTE \"%s\" = \"%s\"\n",
 					__func__, key, value);
 	}
-- 
2.20.1

[PATCH 2/5] [RFC] kasan: kasan_test: hide allocation sizes from the compiler

[Daniel Axtens]

We're about to annotate the allocation functions so that the complier will
know the sizes of the allocated objects. This is then caught at compile
time by both the testing in copy_to/from_user, and the testing in fortify.

The simplest way I can find to obscure the size is to pass the memory
through a WRITE_ONCE/READ_ONCE pair.

Create a macro to obscure an object's size, and a kmalloc wrapper to return
an object with an obscured size. Using these is sufficient to compile without
error.

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 lib/test_kasan.c | 48 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 328d33beae36..dbbecd75f1e3 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -20,9 +20,28 @@
 #include <linux/uaccess.h>
 #include <linux/io.h>
 #include <linux/vmalloc.h>
+#include <linux/compiler.h>
 
 #include <asm/page.h>
 
+/*
+ * obscure origin of a pointer, so we can test things that check
+ * the size of the underlying object
+ */
+#define OBSCURE_ORIGINAL_OBJECT(x) {	\
+	void *bounce;			\
+	WRITE_ONCE(bounce, x);		\
+	x = READ_ONCE(bounce);		\
+	}
+
+static inline void *obscured_kmalloc(size_t size, gfp_t flags)
+{
+	void *result, *bounce;
+	result = kmalloc(size, flags);
+	WRITE_ONCE(bounce, result);
+	return READ_ONCE(bounce);
+}
+
 /*
  * Note: test functions are marked noinline so that their names appear in
  * reports.
@@ -34,7 +53,7 @@ static noinline void __init kmalloc_oob_right(void)
 	size_t size = 123;
 
 	pr_info("out-of-bounds to right\n");
-	ptr = kmalloc(size, GFP_KERNEL);
+	ptr = obscured_kmalloc(size, GFP_KERNEL);
 	if (!ptr) {
 		pr_err("Allocation failed\n");
 		return;
@@ -50,7 +69,7 @@ static noinline void __init kmalloc_oob_left(void)
 	size_t size = 15;
 
 	pr_info("out-of-bounds to left\n");
-	ptr = kmalloc(size, GFP_KERNEL);
+	ptr = obscured_kmalloc(size, GFP_KERNEL);
 	if (!ptr) {
 		pr_err("Allocation failed\n");
 		return;
@@ -67,6 +86,7 @@ static noinline void __init kmalloc_node_oob_right(void)
 
 	pr_info("kmalloc_node(): out-of-bounds to right\n");
 	ptr = kmalloc_node(size, GFP_KERNEL, 0);
+	OBSCURE_ORIGINAL_OBJECT(ptr);
 	if (!ptr) {
 		pr_err("Allocation failed\n");
 		return;
@@ -86,7 +106,7 @@ static noinline void __init kmalloc_pagealloc_oob_right(void)
 	 * the page allocator fallback.
 	 */
 	pr_info("kmalloc pagealloc allocation: out-of-bounds to right\n");
-	ptr = kmalloc(size, GFP_KERNEL);
+	ptr = obscured_kmalloc(size, GFP_KERNEL);
 	if (!ptr) {
 		pr_err("Allocation failed\n");
 		return;
@@ -136,7 +156,7 @@ static noinline void __init kmalloc_large_oob_right(void)
 	 * and does not trigger the page allocator fallback in SLUB.
 	 */
 	pr_info("kmalloc large allocation: out-of-bounds to right\n");
-	ptr = kmalloc(size, GFP_KERNEL);
+	ptr = obscured_kmalloc(size, GFP_KERNEL);
 	if (!ptr) {
 		pr_err("Allocation failed\n");
 		return;
@@ -155,6 +175,7 @@ static noinline void __init kmalloc_oob_krealloc_more(void)
 	pr_info("out-of-bounds after krealloc more\n");
 	ptr1 = kmalloc(size1, GFP_KERNEL);
 	ptr2 = krealloc(ptr1, size2, GFP_KERNEL);
+	OBSCURE_ORIGINAL_OBJECT(ptr2);
 	if (!ptr1 || !ptr2) {
 		pr_err("Allocation failed\n");
 		kfree(ptr1);
@@ -174,6 +195,7 @@ static noinline void __init kmalloc_oob_krealloc_less(void)
 	pr_info("out-of-bounds after krealloc less\n");
 	ptr1 = kmalloc(size1, GFP_KERNEL);
 	ptr2 = krealloc(ptr1, size2, GFP_KERNEL);
+	OBSCURE_ORIGINAL_OBJECT(ptr2);
 	if (!ptr1 || !ptr2) {
 		pr_err("Allocation failed\n");
 		kfree(ptr1);
@@ -190,7 +212,7 @@ static noinline void __init kmalloc_oob_16(void)
 	} *ptr1, *ptr2;
 
 	pr_info("kmalloc out-of-bounds for 16-bytes access\n");
-	ptr1 = kmalloc(sizeof(*ptr1) - 3, GFP_KERNEL);
+	ptr1 = obscured_kmalloc(sizeof(*ptr1) - 3, GFP_KERNEL);
 	ptr2 = kmalloc(sizeof(*ptr2), GFP_KERNEL);
 	if (!ptr1 || !ptr2) {
 		pr_err("Allocation failed\n");
@@ -209,7 +231,7 @@ static noinline void __init kmalloc_oob_memset_2(void)
 	size_t size = 8;
 
 	pr_info("out-of-bounds in memset2\n");
-	ptr = kmalloc(size, GFP_KERNEL);
+	ptr = obscured_kmalloc(size, GFP_KERNEL);
 	if (!ptr) {
 		pr_err("Allocation failed\n");
 		return;
@@ -225,7 +247,7 @@ static noinline void __init kmalloc_oob_memset_4(void)
 	size_t size = 8;
 
 	pr_info("out-of-bounds in memset4\n");
-	ptr = kmalloc(size, GFP_KERNEL);
+	ptr = obscured_kmalloc(size, GFP_KERNEL);
 	if (!ptr) {
 		pr_err("Allocation failed\n");
 		return;
@@ -242,7 +264,7 @@ static noinline void __init kmalloc_oob_memset_8(void)
 	size_t size = 8;
 
 	pr_info("out-of-bounds in memset8\n");
-	ptr = kmalloc(size, GFP_KERNEL);
+	ptr = obscured_kmalloc(size, GFP_KERNEL);
 	if (!ptr) {
 		pr_err("Allocation failed\n");
 		return;
@@ -258,7 +280,7 @@ static noinline void __init kmalloc_oob_memset_16(void)
 	size_t size = 16;
 
 	pr_info("out-of-bounds in memset16\n");
-	ptr = kmalloc(size, GFP_KERNEL);
+	ptr = obscured_kmalloc(size, GFP_KERNEL);
 	if (!ptr) {
 		pr_err("Allocation failed\n");
 		return;
@@ -274,7 +296,7 @@ static noinline void __init kmalloc_oob_in_memset(void)
 	size_t size = 666;
 
 	pr_info("out-of-bounds in memset\n");
-	ptr = kmalloc(size, GFP_KERNEL);
+	ptr = obscured_kmalloc(size, GFP_KERNEL);
 	if (!ptr) {
 		pr_err("Allocation failed\n");
 		return;
@@ -479,7 +501,7 @@ static noinline void __init copy_user_test(void)
 	size_t size = 10;
 	int unused;
 
-	kmem = kmalloc(size, GFP_KERNEL);
+	kmem = obscured_kmalloc(size, GFP_KERNEL);
 	if (!kmem)
 		return;
 
@@ -599,7 +621,7 @@ static noinline void __init kasan_memchr(void)
 	size_t size = 24;
 
 	pr_info("out-of-bounds in memchr\n");
-	ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
+	ptr = obscured_kmalloc(size, GFP_KERNEL | __GFP_ZERO);
 	if (!ptr)
 		return;
 
@@ -614,7 +636,7 @@ static noinline void __init kasan_memcmp(void)
 	int arr[9];
 
 	pr_info("out-of-bounds in memcmp\n");
-	ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);
+	ptr = obscured_kmalloc(size, GFP_KERNEL | __GFP_ZERO);
 	if (!ptr)
 		return;
 
-- 
2.20.1

[PATCH 3/5] [RFC] staging: rts5208: make len a u16 in rtsx_write_cfg_seq

[Daniel Axtens]

A warning occurs when vzalloc is annotated in a subsequent patch to tell
the compiler that its parameter is an allocation size:

drivers/staging/rts5208/rtsx_chip.c: In function ‘rtsx_write_cfg_seq’:
drivers/staging/rts5208/rtsx_chip.c:1453:7: warning: argument 1 value ‘18446744073709551615’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=]
  data = vzalloc(array_size(dw_len, 4));
  ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This occurs because len and dw_len are signed integers and the parameter to
array_size is a size_t. If dw_len is a negative integer, it will become a
very large positive number when cast to size_t. This could cause an
overflow, so array_size(), will return SIZE_MAX _at compile time_. gcc then
notices that this value is too large for an allocation and throws a
warning.

rtsx_write_cfg_seq is only called from write_cfg_byte in rtsx_scsi.c.
There, len is a u16. So make len a u16 in rtsx_write_cfg_seq too. This
means dw_len can never be negative, avoiding the potential overflow and the
warning.

This should not cause a functional change, but was compile tested only.

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 drivers/staging/rts5208/rtsx_chip.c | 2 +-
 drivers/staging/rts5208/rtsx_chip.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx_chip.c b/drivers/staging/rts5208/rtsx_chip.c
index 17c4131f5f62..4a8cbf7362f7 100644
--- a/drivers/staging/rts5208/rtsx_chip.c
+++ b/drivers/staging/rts5208/rtsx_chip.c
@@ -1432,7 +1432,7 @@ int rtsx_read_cfg_dw(struct rtsx_chip *chip, u8 func_no, u16 addr, u32 *val)
 }
 
 int rtsx_write_cfg_seq(struct rtsx_chip *chip, u8 func, u16 addr, u8 *buf,
-		       int len)
+		       u16 len)
 {
 	u32 *data, *mask;
 	u16 offset = addr % 4;
diff --git a/drivers/staging/rts5208/rtsx_chip.h b/drivers/staging/rts5208/rtsx_chip.h
index bac65784d4a1..9b0024557b7e 100644
--- a/drivers/staging/rts5208/rtsx_chip.h
+++ b/drivers/staging/rts5208/rtsx_chip.h
@@ -963,7 +963,7 @@ int rtsx_write_cfg_dw(struct rtsx_chip *chip,
 		      u8 func_no, u16 addr, u32 mask, u32 val);
 int rtsx_read_cfg_dw(struct rtsx_chip *chip, u8 func_no, u16 addr, u32 *val);
 int rtsx_write_cfg_seq(struct rtsx_chip *chip,
-		       u8 func, u16 addr, u8 *buf, int len);
+		       u8 func, u16 addr, u8 *buf, u16 len);
 int rtsx_read_cfg_seq(struct rtsx_chip *chip,
 		      u8 func, u16 addr, u8 *buf, int len);
 int rtsx_write_phy_register(struct rtsx_chip *chip, u8 addr, u16 val);
-- 
2.20.1

[PATCH 4/5] [VERY RFC] mm: kmalloc(_node): return NULL immediately for SIZE_MAX

[Daniel Axtens]

kmalloc is sometimes compiled with an size that at compile time may be
equal to SIZE_MAX.

For example, struct_size(struct, array member, array elements) returns the
size of a structure that has an array as the last element, containing a
given number of elements, or SIZE_MAX on overflow.

However, struct_size operates in (arguably) unintuitive ways at compile time.
Consider the following snippet:

struct foo {
	int a;
	int b[0];
};

struct foo *alloc_foo(int elems)
{
	struct foo *result;
	size_t size = struct_size(result, b, elems);
	if (__builtin_constant_p(size)) {
		BUILD_BUG_ON(size == SIZE_MAX);
	}
	result = kmalloc(size, GFP_KERNEL);
	return result;
}

I expected that size would only be constant if alloc_foo() was called
within that translation unit with a constant number of elements, and the
compiler had decided to inline it. I'd therefore expect that 'size' is only
SIZE_MAX if the constant provided was a huge number.

However, instead, this function hits the BUILD_BUG_ON, even if never
called.

include/linux/compiler.h:394:38: error: call to ‘__compiletime_assert_32’ declared with attribute error: BUILD_BUG_ON failed: size == SIZE_MAX

This is with gcc 9.2.1, and I've also observed it with an gcc 8 series
compiler.

My best explanation of this is:

 - elems is a signed int, so a small negative number will become a very
   large unsigned number when cast to a size_t, leading to overflow.

 - Then, the only way in which size can be a constant is if we hit the
   overflow case, in which 'size' will be 'SIZE_MAX'.

 - So the compiler takes that value into the body of the if statement and
   blows up.

But I could be totally wrong.

Anyway, this is relevant to slab.h because kmalloc() and kmalloc_node()
check if the supplied size is a constant and take a faster path if so. A
number of callers of those functions use struct_size to determine the size
of a memory allocation. Therefore, at compile time, those functions will go
down the constant path, specialising for the overflow case.

When my next patch is applied, gcc will then throw a warning any time
kmalloc_large could be called with a SIZE_MAX size, as gcc deems SIZE_MAX
to be too big an allocation.

So, make functions that check __builtin_constant_p check also against
SIZE_MAX in the constant path, and immediately return NULL if we hit it.

This brings kmalloc() and kmalloc_node() into line with the array functions
kmalloc_array() and kmalloc_array_node() for the overflow case. The overall
compiled size change per bloat-o-meter is in the noise (a reduction of
<0.01%).

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 include/linux/slab.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 03a389358562..8141c6b1882a 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -544,6 +544,9 @@ static __always_inline void *kmalloc(size_t size, gfp_t flags)
 #ifndef CONFIG_SLOB
 		unsigned int index;
 #endif
+		if (unlikely(size == SIZE_MAX))
+			return NULL;
+
 		if (size > KMALLOC_MAX_CACHE_SIZE)
 			return kmalloc_large(size, flags);
 #ifndef CONFIG_SLOB
@@ -562,6 +565,9 @@ static __always_inline void *kmalloc(size_t size, gfp_t flags)
 
 static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
 {
+	if (__builtin_constant_p(size) && size == SIZE_MAX)
+		return NULL;
+
 #ifndef CONFIG_SLOB
 	if (__builtin_constant_p(size) &&
 		size <= KMALLOC_MAX_CACHE_SIZE) {
-- 
2.20.1

Re: [PATCH 4/5] [VERY RFC] mm: kmalloc(_node): return NULL immediately for SIZE_MAX

[Michal Hocko]

On Mon 20-01-20 18:43:43, Daniel Axtens wrote:
> kmalloc is sometimes compiled with an size that at compile time may be
> equal to SIZE_MAX.
> 
> For example, struct_size(struct, array member, array elements) returns the
> size of a structure that has an array as the last element, containing a
> given number of elements, or SIZE_MAX on overflow.
> 
> However, struct_size operates in (arguably) unintuitive ways at compile time.
> Consider the following snippet:
> 
> struct foo {
> 	int a;
> 	int b[0];
> };
> 
> struct foo *alloc_foo(int elems)
> {
> 	struct foo *result;
> 	size_t size = struct_size(result, b, elems);
> 	if (__builtin_constant_p(size)) {
> 		BUILD_BUG_ON(size == SIZE_MAX);
> 	}
> 	result = kmalloc(size, GFP_KERNEL);
> 	return result;
> }
> 
> I expected that size would only be constant if alloc_foo() was called
> within that translation unit with a constant number of elements, and the
> compiler had decided to inline it. I'd therefore expect that 'size' is only
> SIZE_MAX if the constant provided was a huge number.
> 
> However, instead, this function hits the BUILD_BUG_ON, even if never
> called.
> 
> include/linux/compiler.h:394:38: error: call to ‘__compiletime_assert_32’ declared with attribute error: BUILD_BUG_ON failed: size == SIZE_MAX

This sounds more like a bug to me. Have you tried to talk to compiler
guys?

> This is with gcc 9.2.1, and I've also observed it with an gcc 8 series
> compiler.
> 
> My best explanation of this is:
> 
>  - elems is a signed int, so a small negative number will become a very
>    large unsigned number when cast to a size_t, leading to overflow.
> 
>  - Then, the only way in which size can be a constant is if we hit the
>    overflow case, in which 'size' will be 'SIZE_MAX'.
> 
>  - So the compiler takes that value into the body of the if statement and
>    blows up.
> 
> But I could be totally wrong.
> 
> Anyway, this is relevant to slab.h because kmalloc() and kmalloc_node()
> check if the supplied size is a constant and take a faster path if so. A
> number of callers of those functions use struct_size to determine the size
> of a memory allocation. Therefore, at compile time, those functions will go
> down the constant path, specialising for the overflow case.
> 
> When my next patch is applied, gcc will then throw a warning any time
> kmalloc_large could be called with a SIZE_MAX size, as gcc deems SIZE_MAX
> to be too big an allocation.
> 
> So, make functions that check __builtin_constant_p check also against
> SIZE_MAX in the constant path, and immediately return NULL if we hit it.

I am not sure I am happy about an additional conditional path in the hot
path of the allocator. Especially when we already have a check for
KMALLOC_MAX_CACHE_SIZE.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/5] [VERY RFC] mm: kmalloc(_node): return NULL immediately for SIZE_MAX

[Daniel Axtens]

Hi Michal,

>> For example, struct_size(struct, array member, array elements) returns the
>> size of a structure that has an array as the last element, containing a
>> given number of elements, or SIZE_MAX on overflow.
>> 
>> However, struct_size operates in (arguably) unintuitive ways at compile time.
>> Consider the following snippet:
>> 
>> struct foo {
>> 	int a;
>> 	int b[0];
>> };
>> 
>> struct foo *alloc_foo(int elems)
>> {
>> 	struct foo *result;
>> 	size_t size = struct_size(result, b, elems);
>> 	if (__builtin_constant_p(size)) {
>> 		BUILD_BUG_ON(size == SIZE_MAX);
>> 	}
>> 	result = kmalloc(size, GFP_KERNEL);
>> 	return result;
>> }
>> 
>> I expected that size would only be constant if alloc_foo() was called
>> within that translation unit with a constant number of elements, and the
>> compiler had decided to inline it. I'd therefore expect that 'size' is only
>> SIZE_MAX if the constant provided was a huge number.
>> 
>> However, instead, this function hits the BUILD_BUG_ON, even if never
>> called.
>> 
>> include/linux/compiler.h:394:38: error: call to ‘__compiletime_assert_32’ declared with attribute error: BUILD_BUG_ON failed: size == SIZE_MAX
>
> This sounds more like a bug to me. Have you tried to talk to compiler
> guys?

You're now the second person to suggest this to me, so I will do that
today.

>> This is with gcc 9.2.1, and I've also observed it with an gcc 8 series
>> compiler.
>> 
>> My best explanation of this is:
>> 
>>  - elems is a signed int, so a small negative number will become a very
>>    large unsigned number when cast to a size_t, leading to overflow.
>> 
>>  - Then, the only way in which size can be a constant is if we hit the
>>    overflow case, in which 'size' will be 'SIZE_MAX'.
>> 
>>  - So the compiler takes that value into the body of the if statement and
>>    blows up.
>> 
>> But I could be totally wrong.
>> 
>> Anyway, this is relevant to slab.h because kmalloc() and kmalloc_node()
>> check if the supplied size is a constant and take a faster path if so. A
>> number of callers of those functions use struct_size to determine the size
>> of a memory allocation. Therefore, at compile time, those functions will go
>> down the constant path, specialising for the overflow case.
>> 
>> When my next patch is applied, gcc will then throw a warning any time
>> kmalloc_large could be called with a SIZE_MAX size, as gcc deems SIZE_MAX
>> to be too big an allocation.
>> 
>> So, make functions that check __builtin_constant_p check also against
>> SIZE_MAX in the constant path, and immediately return NULL if we hit it.
>
> I am not sure I am happy about an additional conditional path in the hot
> path of the allocator. Especially when we already have a check for
> KMALLOC_MAX_CACHE_SIZE.

It is guarded by __builtin_constant_p in both cases, so it should not
cause an additional runtime branch. But I'll check in with our friendly
local compiler folks and see where that leads first.

Regards,
Daniel

> -- 
> Michal Hocko
> SUSE Labs

[PATCH 5/5] [RFC] mm: annotate memory allocation functions with their sizes

[Daniel Axtens]

gcc and clang support the alloc_size attribute. Quoting
<https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes>:

  alloc_size (position)
  alloc_size (position-1, position-2)

    The alloc_size attribute may be applied to a function that returns a
    pointer and takes at least one argument of an integer or enumerated
    type. It indicates that the returned pointer points to memory whose
    size is given by the function argument at position-1, or by the product
    of the arguments at position-1 and position-2. Meaningful sizes are
    positive values less than PTRDIFF_MAX. GCC uses this information to
    improve the results of __builtin_object_size.

gcc supports this back to at least 4.3.6 [1], and clang has supported it
since December 2016 [2]. I think this is sufficent to make it always-on.

Annotate the kmalloc and vmalloc family: where a memory allocation has a
size knowable at compile time, allow the compiler to use that for
__builtin_object_size() calculations.

There are a couple of limitations:

 * only functions that return a single pointer can be directly annotated

 * only functions that take the size as a parameter (or as the product of
   two parameters) can be directly annotated.

These could possibly be addressed in future with some hackery.

This is useful for two things:

  * __builtin_object_size() is used in fortify and copy_to/from_user to
    find bugs at compile time and run time.

  * knowing the size allows the compiler to inline things when using
    __builtin_* functions. With my config with FORTIFY_SOURCE enabled
    I see a number of strlcpys being converted into a strlen and inline
    memcpy. This leads to an overall size increase of 0.04% (per
    bloat-o-meter) when compiled with -O2.

[1]: https://gcc.gnu.org/onlinedocs/gcc-4.3.6/gcc/Function-Attributes.html#Function-Attributes
[2]: https://reviews.llvm.org/D14274

Cc: Kees Cook <keescook@chromium.org>
Cc: Daniel Micay <danielmicay@gmail.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 include/linux/compiler_attributes.h |  6 +++++
 include/linux/kasan.h               | 12 ++++-----
 include/linux/slab.h                | 38 ++++++++++++++++++-----------
 include/linux/vmalloc.h             | 26 ++++++++++----------
 4 files changed, 49 insertions(+), 33 deletions(-)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index cdf016596659..ccacbb2f2c56 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -56,6 +56,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(a, ...)		__attribute__((alloc_size(a, ## __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
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 5cde9e7c2664..a8da784c98ad 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -53,13 +53,13 @@ void * __must_check kasan_init_slab_obj(struct kmem_cache *cache,
 					const void *object);
 
 void * __must_check kasan_kmalloc_large(const void *ptr, size_t size,
-						gfp_t flags);
+						gfp_t flags) __alloc_size(2);
 void kasan_kfree_large(void *ptr, unsigned long ip);
 void kasan_poison_kfree(void *ptr, unsigned long ip);
 void * __must_check kasan_kmalloc(struct kmem_cache *s, const void *object,
-					size_t size, gfp_t flags);
+					size_t size, gfp_t flags) __alloc_size(3);
 void * __must_check kasan_krealloc(const void *object, size_t new_size,
-					gfp_t flags);
+					gfp_t flags) __alloc_size(2);
 
 void * __must_check kasan_slab_alloc(struct kmem_cache *s, void *object,
 					gfp_t flags);
@@ -124,18 +124,18 @@ static inline void *kasan_init_slab_obj(struct kmem_cache *cache,
 	return (void *)object;
 }
 
-static inline void *kasan_kmalloc_large(void *ptr, size_t size, gfp_t flags)
+static inline __alloc_size(2) void *kasan_kmalloc_large(void *ptr, size_t size, gfp_t flags)
 {
 	return ptr;
 }
 static inline void kasan_kfree_large(void *ptr, unsigned long ip) {}
 static inline void kasan_poison_kfree(void *ptr, unsigned long ip) {}
-static inline void *kasan_kmalloc(struct kmem_cache *s, const void *object,
+static inline __alloc_size(3) void *kasan_kmalloc(struct kmem_cache *s, const void *object,
 				size_t size, gfp_t flags)
 {
 	return (void *)object;
 }
-static inline void *kasan_krealloc(const void *object, size_t new_size,
+static inline __alloc_size(2) void *kasan_krealloc(const void *object, size_t new_size,
 				 gfp_t flags)
 {
 	return (void *)object;
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8141c6b1882a..fbfc81f37374 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -184,7 +184,7 @@ void memcg_deactivate_kmem_caches(struct mem_cgroup *, struct mem_cgroup *);
 /*
  * 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 kzfree(const void *);
 size_t __ksize(const void *);
@@ -389,7 +389,9 @@ static __always_inline unsigned int kmalloc_index(size_t size)
 }
 #endif /* !CONFIG_SLOB */
 
-void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __malloc;
+__assume_kmalloc_alignment __malloc __alloc_size(1) void *
+__kmalloc(size_t size, gfp_t flags);
+
 void *kmem_cache_alloc(struct kmem_cache *, gfp_t flags) __assume_slab_alignment __malloc;
 void kmem_cache_free(struct kmem_cache *, void *);
 
@@ -413,8 +415,11 @@ 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 *kmem_cache_alloc_node(struct kmem_cache *, gfp_t flags, int node) __assume_slab_alignment __malloc;
+__assume_kmalloc_alignment __malloc __alloc_size(1) void *
+__kmalloc_node(size_t size, gfp_t flags, int node);
+
+__assume_slab_alignment __malloc void *
+kmem_cache_alloc_node(struct kmem_cache *, gfp_t flags, int node);
 #else
 static __always_inline void *__kmalloc_node(size_t size, gfp_t flags, int node)
 {
@@ -428,12 +433,14 @@ static __always_inline void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t f
 #endif
 
 #ifdef CONFIG_TRACING
-extern void *kmem_cache_alloc_trace(struct kmem_cache *, gfp_t, size_t) __assume_slab_alignment __malloc;
+extern __alloc_size(3) void *
+kmem_cache_alloc_trace(struct kmem_cache *, gfp_t, size_t);
 
 #ifdef CONFIG_NUMA
-extern void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
-					   gfp_t gfpflags,
-					   int node, size_t size) __assume_slab_alignment __malloc;
+extern  __assume_slab_alignment __malloc __alloc_size(4) void *
+kmem_cache_alloc_node_trace(struct kmem_cache *s,
+			    gfp_t gfpflags,
+			    int node, size_t size);
 #else
 static __always_inline void *
 kmem_cache_alloc_node_trace(struct kmem_cache *s,
@@ -445,8 +452,8 @@ kmem_cache_alloc_node_trace(struct kmem_cache *s,
 #endif /* CONFIG_NUMA */
 
 #else /* CONFIG_TRACING */
-static __always_inline void *kmem_cache_alloc_trace(struct kmem_cache *s,
-		gfp_t flags, size_t size)
+static __always_inline __alloc_size(3) void *
+kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
 {
 	void *ret = kmem_cache_alloc(s, flags);
 
@@ -454,7 +461,7 @@ static __always_inline void *kmem_cache_alloc_trace(struct kmem_cache *s,
 	return ret;
 }
 
-static __always_inline void *
+static __always_inline __alloc_size(4) void *
 kmem_cache_alloc_node_trace(struct kmem_cache *s,
 			      gfp_t gfpflags,
 			      int node, size_t size)
@@ -466,10 +473,12 @@ kmem_cache_alloc_node_trace(struct kmem_cache *s,
 }
 #endif /* CONFIG_TRACING */
 
-extern void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) __assume_page_alignment __malloc;
+extern __assume_page_alignment __malloc __alloc_size(1) void *
+kmalloc_order(size_t size, gfp_t flags, unsigned int order);
 
 #ifdef CONFIG_TRACING
-extern void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) __assume_page_alignment __malloc;
+extern __assume_page_alignment __malloc __alloc_size(1) void *
+kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order);
 #else
 static __always_inline void *
 kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order)
@@ -645,7 +654,8 @@ static inline void *kcalloc_node(size_t n, size_t size, gfp_t flags, int node)
 
 
 #ifdef CONFIG_NUMA
-extern void *__kmalloc_node_track_caller(size_t, gfp_t, int, unsigned long);
+extern __alloc_size(1) void *
+__kmalloc_node_track_caller(size_t, gfp_t, int, unsigned long);
 #define kmalloc_node_track_caller(size, flags, node) \
 	__kmalloc_node_track_caller(size, flags, node, \
 			_RET_IP_)
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 0507a162ccd0..a3651bcc62a3 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -102,22 +102,22 @@ 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_user_node_flags(unsigned long size, int node, gfp_t flags);
-extern void *vmalloc_exec(unsigned long size);
-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, pgprot_t prot);
+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_user_node_flags(unsigned long size, int node, gfp_t flags) __alloc_size(1);
+extern void *vmalloc_exec(unsigned long size) __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, pgprot_t prot) __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);
 #ifndef CONFIG_MMU
-extern void *__vmalloc_node_flags(unsigned long size, int node, gfp_t flags);
+extern void *__vmalloc_node_flags(unsigned long size, int node, gfp_t flags) __alloc_size(1);
 static inline void *__vmalloc_node_flags_caller(unsigned long size, int node,
 						gfp_t flags, void *caller)
 {
@@ -125,7 +125,7 @@ static inline void *__vmalloc_node_flags_caller(unsigned long size, int node,
 }
 #else
 extern void *__vmalloc_node_flags_caller(unsigned long size,
-					 int node, gfp_t flags, void *caller);
+					 int node, gfp_t flags, void *caller) __alloc_size(1);
 #endif
 
 extern void vfree(const void *addr);
-- 
2.20.1

Re: [PATCH 5/5] [RFC] mm: annotate memory allocation functions with their sizes

[Daniel Micay]

There are some uses of ksize in the kernel making use of the real
usable size of memory allocations rather than only the requested
amount. It's incorrect when mixed with alloc_size markers, since if a
number like 14 is passed that's used as the upper bound, rather than a
rounded size like 16 returned by ksize. It's unlikely to trigger any
issues with only CONFIG_FORTIFY_SOURCE, but it becomes more likely
with -fsanitize=object-size or other library-based usage of
__builtin_object_size.

Re: [PATCH 5/5] [RFC] mm: annotate memory allocation functions with their sizes

[Kees Cook]

On Fri, Feb 07, 2020 at 03:38:22PM -0500, Daniel Micay wrote:
> There are some uses of ksize in the kernel making use of the real
> usable size of memory allocations rather than only the requested
> amount. It's incorrect when mixed with alloc_size markers, since if a
> number like 14 is passed that's used as the upper bound, rather than a
> rounded size like 16 returned by ksize. It's unlikely to trigger any
> issues with only CONFIG_FORTIFY_SOURCE, but it becomes more likely
> with -fsanitize=object-size or other library-based usage of
> __builtin_object_size.

I think the solution here is to use a macro that does the per-bucket
rounding and applies them to the attributes. Keep the bucket size lists
in sync will likely need some BUILD_BUG_ON()s or similar.

-- 
Kees Cook

Re: [PATCH 5/5] [RFC] mm: annotate memory allocation functions with their sizes

[Daniel Axtens]

Kees Cook <keescook@chromium.org> writes:

> On Fri, Feb 07, 2020 at 03:38:22PM -0500, Daniel Micay wrote:
>> There are some uses of ksize in the kernel making use of the real
>> usable size of memory allocations rather than only the requested
>> amount. It's incorrect when mixed with alloc_size markers, since if a
>> number like 14 is passed that's used as the upper bound, rather than a
>> rounded size like 16 returned by ksize. It's unlikely to trigger any
>> issues with only CONFIG_FORTIFY_SOURCE, but it becomes more likely
>> with -fsanitize=object-size or other library-based usage of
>> __builtin_object_size.
>
> I think the solution here is to use a macro that does the per-bucket
> rounding and applies them to the attributes. Keep the bucket size lists
> in sync will likely need some BUILD_BUG_ON()s or similar.

I can have a go at this but with various other work projects it has
unfortunately slipped way down the to-do list. So I've very happy for
anyone else to take this and run with it.

Regards,
Daniel

>
> -- 
> Kees Cook

Re: [PATCH 5/5] [RFC] mm: annotate memory allocation functions with their sizes

[Kees Cook]

On Wed, Feb 26, 2020 at 05:07:18PM +1100, Daniel Axtens wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
> > On Fri, Feb 07, 2020 at 03:38:22PM -0500, Daniel Micay wrote:
> >> There are some uses of ksize in the kernel making use of the real
> >> usable size of memory allocations rather than only the requested
> >> amount. It's incorrect when mixed with alloc_size markers, since if a
> >> number like 14 is passed that's used as the upper bound, rather than a
> >> rounded size like 16 returned by ksize. It's unlikely to trigger any
> >> issues with only CONFIG_FORTIFY_SOURCE, but it becomes more likely
> >> with -fsanitize=object-size or other library-based usage of
> >> __builtin_object_size.
> >
> > I think the solution here is to use a macro that does the per-bucket
> > rounding and applies them to the attributes. Keep the bucket size lists
> > in sync will likely need some BUILD_BUG_ON()s or similar.
> 
> I can have a go at this but with various other work projects it has
> unfortunately slipped way down the to-do list. So I've very happy for
> anyone else to take this and run with it.

Sounds good. I've added the above note from Micay to the KSPP bug tracker:
https://github.com/KSPP/linux/issues/5

Thanks for bringing this topic back up!

-- 
Kees Cook