Annotate allocation functions with alloc_size attribute

PATCH In review Daniel Axtens 5 participants 13 messages Jan 20, 2020 – Feb 26, 2020

Subsystems (8)

Memory management Staging Library code Char and misc drivers +4 more subsystems

lore

Share & tools

lore

Daniel Axtens

author

2020-01-20 07:43 UTC #0 lore raw

cover Annotate allocation functions with alloc_size attribute

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(-)

Signature · 1 line

2.20.1

Daniel Axtens

author

2020-01-20 07:43 UTC #1 lore raw

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

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" <[email protected]>

Signed-off-by Daniel Axtens <[email protected]>

---
 drivers/misc/altera-stapl/altera.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/misc/altera-stapl/altera.c +6 -6

index 25e5f24b3fec..5bdf57472314 100644
@@ -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);
 	}

Signature · 1 line

2.20.1

Daniel Axtens

author

2020-01-20 07:43 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 lib/test_kasan.c | 48 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 13 deletions(-)

lib/test_kasan.c +35 ~~-13~~

index 328d33beae36..dbbecd75f1e3 100644
@@ -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;

Signature · 1 line

2.20.1

Daniel Axtens

author

2020-01-20 07:44 UTC #3 lore raw

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

In reply to Daniel Axtens

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

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 <[email protected]>

---
 drivers/staging/rts5208/rtsx_chip.c | 2 +-
 drivers/staging/rts5208/rtsx_chip.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/staging/rts5208/rtsx_chip.c +1 -1

index 17c4131f5f62..4a8cbf7362f7 100644
@@ -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;

drivers/staging/rts5208/rtsx_chip.h +1 -1

index bac65784d4a1..9b0024557b7e 100644
@@ -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);

Signature · 1 line

2.20.1

Daniel Axtens

author

2020-01-20 07:44 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 include/linux/slab.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/slab.h +6 -0

index 03a389358562..8141c6b1882a 100644
@@ -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) {

Signature · 1 line

2.20.1

Michal Hocko

2020-01-20 11:14 UTC #5 lore raw

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

Quoted 31 lines · 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.

Quoted 4 lines

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?

Quoted 22 lines

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.

Quoted 6 lines

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.

Signature · 2 lines

Michal Hocko
SUSE Labs

Daniel Axtens

author

2020-01-20 22:51 UTC #6 lore raw

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

Hi Michal,

Quoted 28 lines

Quoted 28 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 26 lines

Quoted 26 lines
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.

Quoted 6 lines

Quoted 2 lines
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

Quoted 3 lines

Signature · 2 lines
Michal Hocko
SUSE Labs

Daniel Axtens

author

2020-01-20 07:44 UTC #7 lore raw

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

In reply to 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 <[email protected]>

Cc Daniel Micay <[email protected]>

Signed-off-by Daniel Axtens <[email protected]>

---
 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(-)

include/linux/compiler_attributes.h +6 -0

index cdf016596659..ccacbb2f2c56 100644
@@ -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

include/linux/kasan.h +6 -6

index 5cde9e7c2664..a8da784c98ad 100644
@@ -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;

include/linux/slab.h +24 ~~-14~~

index 8141c6b1882a..fbfc81f37374 100644
@@ -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_)

include/linux/vmalloc.h +13 ~~-13~~

index 0507a162ccd0..a3651bcc62a3 100644
@@ -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);

Signature · 1 line

2.20.1

kbuild test robot

build bot

2020-01-20 11:57 UTC #8 lore raw

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

[if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please…

Hi Daniel,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on next-20200117]
[cannot apply to staging/staging-testing char-misc/char-misc-testing kees/for-next/pstore linus/master v5.5-rc7 v5.5-rc6 v5.5-rc5 v5.5-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Daniel-Axtens/Annotate-allocation-functions-with-alloc_size-attribute/20200120-155420
base:    de970dffa7d19eae1d703c3534825308ef8d5dec
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.5.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=sh

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/media//common/v4l2-tpg/v4l2-tpg-core.c: In function 'tpg_alloc':

>> drivers/media//common/v4l2-tpg/v4l2-tpg-core.c:138:4: warning: argument 1 value '4294967295' exceeds maximum object size 2147483647 [-Walloc-size-larger-than=]
       vzalloc(array_size(pixelsz, max_w));
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/media/tpg/v4l2-tpg.h:15:0,
                    from drivers/media//common/v4l2-tpg/v4l2-tpg-core.c:12:
   include/linux/vmalloc.h:106:14: note: in a call to allocation function 'vzalloc' declared here
    extern void *vzalloc(unsigned long size) __alloc_size(1);
                 ^~~~~~~

vim +/4294967295 +138 drivers/media//common/v4l2-tpg/v4l2-tpg-core.c

63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  111  
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  112  int tpg_alloc(struct tpg_data *tpg, unsigned max_w)
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  113  {
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  114  	unsigned pat;
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  115  	unsigned plane;
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  116  
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  117  	tpg->max_line_width = max_w;
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  118  	for (pat = 0; pat < TPG_MAX_PAT_LINES; pat++) {
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  119  		for (plane = 0; plane < TPG_MAX_PLANES; plane++) {
dde72bd773dfce drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2015-03-13  120  			unsigned pixelsz = plane ? 2 : 4;
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  121  
fad953ce0b22cf drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Kees Cook                 2018-06-12  122  			tpg->lines[pat][plane] =
fad953ce0b22cf drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Kees Cook                 2018-06-12  123  				vzalloc(array3_size(max_w, 2, pixelsz));
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  124  			if (!tpg->lines[pat][plane])
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  125  				return -ENOMEM;
5d7c539e194af5 drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2015-03-07  126  			if (plane == 0)
5d7c539e194af5 drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2015-03-07  127  				continue;
fad953ce0b22cf drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Kees Cook                 2018-06-12  128  			tpg->downsampled_lines[pat][plane] =
fad953ce0b22cf drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Kees Cook                 2018-06-12  129  				vzalloc(array3_size(max_w, 2, pixelsz));
5d7c539e194af5 drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2015-03-07  130  			if (!tpg->downsampled_lines[pat][plane])
5d7c539e194af5 drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2015-03-07  131  				return -ENOMEM;
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  132  		}
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  133  	}
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  134  	for (plane = 0; plane < TPG_MAX_PLANES; plane++) {
dde72bd773dfce drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2015-03-13  135  		unsigned pixelsz = plane ? 2 : 4;
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  136  
fad953ce0b22cf drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Kees Cook                 2018-06-12  137  		tpg->contrast_line[plane] =
fad953ce0b22cf drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Kees Cook                 2018-06-12 @138  			vzalloc(array_size(pixelsz, max_w));
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  139  		if (!tpg->contrast_line[plane])
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  140  			return -ENOMEM;
fad953ce0b22cf drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Kees Cook                 2018-06-12  141  		tpg->black_line[plane] =
fad953ce0b22cf drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Kees Cook                 2018-06-12  142  			vzalloc(array_size(pixelsz, max_w));
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  143  		if (!tpg->black_line[plane])
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  144  			return -ENOMEM;
fad953ce0b22cf drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Kees Cook                 2018-06-12  145  		tpg->random_line[plane] =
fad953ce0b22cf drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Kees Cook                 2018-06-12  146  			vzalloc(array3_size(max_w, 2, pixelsz));
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  147  		if (!tpg->random_line[plane])
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  148  			return -ENOMEM;
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  149  	}
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  150  	return 0;
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c      Hans Verkuil              2014-08-25  151  }

e07d46e7e0da86 drivers/media/common/v4l2-tpg/v4l2-tpg-core.c Helen Mae Koike Fornazier 2016-04-08 152 EXPORT_SYMBOL_GPL(tpg_alloc);
63881df94d3ecb drivers/media/platform/vivid/vivid-tpg.c Hans Verkuil 2014-08-25 153

:::::: The code at line 138 was first introduced by commit
:::::: fad953ce0b22cfd352a9a90b070c34b8791e6868 treewide: Use array_size() in vzalloc()

:::::: TO: Kees Cook <[email protected]>
:::::: CC: Kees Cook <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

Daniel Micay

2020-02-07 20:38 UTC #9 lore raw

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

In reply to Daniel Axtens

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.

Kees Cook

2020-02-25 18:36 UTC #10 lore raw

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

Quoted 8 lines · 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.

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.

Signature · 1 line

Kees Cook

Daniel Axtens

author

2020-02-26 06:07 UTC #11 lore raw

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

Quoted 10 lines · Kees Cook

Quoted 8 lines · 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.

Quoted 3 lines

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

Quoted 3 lines

Signature · 2 lines
-- 
Kees Cook

Kees Cook

2020-02-26 21:57 UTC #12 lore raw

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

Quoted 12 lines · Daniel Axtens

Quoted 10 lines · Kees Cook
Quoted 8 lines · 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.

Quoted 7 lines

Quoted 3 lines
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!

Signature · 1 line

Kees Cook