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
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(-)
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
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 <[email protected]>
---
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
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(+)
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
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(-)
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
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
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
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.
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
Kees Cook <[email protected]> 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
On Wed, Feb 26, 2020 at 05:07:18PM +1100, Daniel Axtens wrote:
> Kees Cook <[email protected]> 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