All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Axtens <dja@axtens.net>
To: kernel-hardening@lists.openwall.com, linux-mm@kvack.org,
	keescook@chromium.org
Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org,
	Daniel Axtens <dja@axtens.net>
Subject: [PATCH 4/5] [VERY RFC] mm: kmalloc(_node): return NULL immediately for SIZE_MAX
Date: Mon, 20 Jan 2020 18:43:43 +1100	[thread overview]
Message-ID: <20200120074344.504-5-dja@axtens.net> (raw)
In-Reply-To: <20200120074344.504-1-dja@axtens.net>

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


  parent reply	other threads:[~2020-01-20  7:44 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-20  7:43 [PATCH 0/5] Annotate allocation functions with alloc_size attribute Daniel Axtens
2020-01-20  7:43 ` [PATCH 1/5] altera-stapl: altera_get_note: prevent write beyond end of 'key' Daniel Axtens
2020-01-20  7:43 ` [PATCH 2/5] [RFC] kasan: kasan_test: hide allocation sizes from the compiler Daniel Axtens
2020-01-20  7:43 ` [PATCH 3/5] [RFC] staging: rts5208: make len a u16 in rtsx_write_cfg_seq Daniel Axtens
2020-01-20  7:43 ` Daniel Axtens [this message]
2020-01-20 11:14   ` [PATCH 4/5] [VERY RFC] mm: kmalloc(_node): return NULL immediately for SIZE_MAX Michal Hocko
2020-01-20 22:51     ` Daniel Axtens
2020-01-20  7:43 ` [PATCH 5/5] [RFC] mm: annotate memory allocation functions with their sizes Daniel Axtens
2020-01-20 11:57   ` kbuild test robot
2020-02-07 20:38   ` Daniel Micay
2020-02-07 20:38     ` Daniel Micay
2020-02-25 18:35     ` Kees Cook
2020-02-26  6:07       ` Daniel Axtens
2020-02-26 21:56         ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200120074344.504-5-dja@axtens.net \
    --to=dja@axtens.net \
    --cc=akpm@linux-foundation.org \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.