integer overflows in kernel/relay.c

My static checker is warning about integer overflows in kernel/relay.c

relay_create_buf()
170

   171          buf->padding = kmalloc(chan->n_subbufs * sizeof(size_t *), GFP_KERNEL);
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This can only overflow on 32bit systems.

   172          if (!buf->padding)
   173                  goto free_buf;
   174  

relay_open()

   582          chan->version = RELAYFS_CHANNEL_VERSION;
   583          chan->n_subbufs = n_subbufs;
   584          chan->subbuf_size = subbuf_size;
   585          chan->alloc_size = FIX_SIZE(subbuf_size * n_subbufs);
                                            ^^^^^^^^^^^^^^^^^^^^^^^

586 chan->parent = parent;

These come from the user in blk_trace_setup() and they aren't capped. I'm not sure what the maximum size to use is.

Quoted 7 lines · Dan Carpenter

My static checker is warning about integer overflows in kernel/relay.c

relay_create_buf()
170

   171          buf->padding = kmalloc(chan->n_subbufs * sizeof(size_t *), GFP_KERNEL);
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This can only overflow on 32bit systems.

Correct

This patch introduces knalloc/kxnalloc wrappers that perform integer overflow checks without zeroing the memory.

knalloc(n, size, flags) is the non-zeroing version of kcalloc(), which allocates n * size bytes.

kxnalloc(xsize, n, size, flags) allocates xsize + n * size bytes. It is useful to allocate a structure ending with a zero-length array, which is a commonly used pattern.  For example, in posix_acl_alloc() to allocate a posix_acl object one could call

   kxnalloc(sizeof(struct posix_acl),
            count, sizeof(struct posix_acl_entry), flags);

to avoid overflowing the allocation size.

Suggested-by Andrew Morton <[email protected]>

Signed-off-by Xi Wang <[email protected]>

---
I coined the name knalloc() for Andrew's wtf_do_i_call_this() in the previous email.  A better name is welcome.

I additionally added kxnalloc() since it's a common idiom.
---
 include/linux/slab.h |   31 +++++++++++++++++++++++++++----

 1 files changed, 27 insertions(+), 4 deletions(-)

include/linux/slab.h +26 -4

index 573c809..e3a1e81 100644
@@ -190,7 +190,8 @@ size_t ksize(const void *);
  #endif
 
  /**
- * kcalloc - allocate memory for an array. The memory is set to zero.
+ * kxnalloc - allocate memory for an array with an extra header.
+ * @xsize: extra header size.
  * @n: number of elements.
  * @size: element size.
  * @flags: the type of memory to allocate.

@@ -240,11 +241,33 @@ size_t ksize(const void *);
  * for general use, and so are not documented here. For a full list of
  * potential flags, always refer to linux/gfp.h.
  */
-static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
+static inline void *kxnalloc(size_t xsize, size_t n, size_t size, gfp_t flags)
  {
-       if (size != 0 && n > ULONG_MAX / size)
+       if (size != 0 && n > (ULONG_MAX - xsize) / size)
                return NULL;
-       return __kmalloc(n * size, flags | __GFP_ZERO);
+       return __kmalloc(xsize + n * size, flags);
+}
+
+/**
+ * knalloc - allocate memory for an array.
+ * @n: number of elements.
+ * @size: element size.
+ * @flags: the type of memory to allocate (see kmalloc).
+ */
+static inline void *knalloc(size_t n, size_t size, gfp_t flags)
+{
+       return kxnalloc(0, n, size, flags);
+}
 
+/**
+ * kcalloc - allocate memory for an array. The memory is set to zero.
+ * @n: number of elements.
+ * @size: element size.
+ * @flags: the type of memory to allocate (see kmalloc).
+ */
+static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
+{
+       return knalloc(n, size, flags | __GFP_ZERO);
  }
 

 #if !defined(CONFIG_NUMA) && !defined(CONFIG_SLOB)

Quoted 14 lines · Xi Wang

This patch introduces knalloc/kxnalloc wrappers that perform integer overflow checks without zeroing the memory.

knalloc(n, size, flags) is the non-zeroing version of kcalloc(), which allocates n * size bytes.

kxnalloc(xsize, n, size, flags) allocates xsize + n * size bytes. It is useful to allocate a structure ending with a zero-length array, which is a commonly used pattern. For example, in posix_acl_alloc() to allocate a posix_acl object one could call

   kxnalloc(sizeof(struct posix_acl),
            count, sizeof(struct posix_acl_entry), flags);

Quoted 4 lines

to avoid overflowing the allocation size.

Suggested-by Andrew Morton <[email protected]>

Signed-off-by Xi Wang <[email protected]>

Are there really enough potential users to justify adding both?

Agree, lets not overdesign. kmalloc_array() sounds good to me, fwiw.

This patch introduces a kmalloc_array() wrapper that performs integer overflow checking without zeroing the memory.

Suggested-by Andrew Morton <[email protected]>

Suggested-by Jens Axboe <[email protected]>

Signed-off-by Xi Wang <[email protected]>

Quoted 6 lines · Xi Wang

This patch introduces a kmalloc_array() wrapper that performs integer overflow checking without zeroing the memory.

Suggested-by Andrew Morton <[email protected]>

Suggested-by Jens Axboe <[email protected]>

Signed-off-by Xi Wang <[email protected]>

Acked-by David Rientjes <[email protected]>

assuming there's at least one new user or one existing user that can be modified to use the new interface and this won't just sit around not being used.

This patch introduces a kmalloc_array() wrapper that performs integer overflow checking without zeroing the memory.

Suggested-by Andrew Morton <[email protected]>

Suggested-by Jens Axboe <[email protected]>

Signed-off-by Xi Wang <[email protected]>

---
Let's take "kxnalloc" off for now and keep the patch simple.

---
 include/linux/slab.h |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

include/linux/slab.h +7 -3

index 573c809..a595dce 100644
@@ -190,7 +190,7 @@ size_t ksize(const void *);
 #endif
 
 /**
- * kcalloc - allocate memory for an array. The memory is set to zero.
+ * kmalloc_array - allocate memory for an array.
  * @n: number of elements.
  * @size: element size.
  * @flags: the type of memory to allocate.

@@ -240,11 +240,22 @@ size_t ksize(const void *);
  * for general use, and so are not documented here. For a full list of
  * potential flags, always refer to linux/gfp.h.
  */
-static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
+static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags)
 {
 	if (size != 0 && n > ULONG_MAX / size)
 		return NULL;
-	return __kmalloc(n * size, flags | __GFP_ZERO);
+	return __kmalloc(n * size, flags);
+}
+
+/**
+ * kcalloc - allocate memory for an array. The memory is set to zero.

Quoted 20 lines

include/linux/slab.h +13 -2

-static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
+static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags)
 {
 	if (size != 0 && n > ULONG_MAX / size)
 		return NULL;
-	return __kmalloc(n * size, flags | __GFP_ZERO);
+	return __kmalloc(n * size, flags);
+}
+
+/**
+ * kcalloc - allocate memory for an array. The memory is set to zero.
+ * @n: number of elements.
+ * @size: element size.
+ * @flags: the type of memory to allocate (see kmalloc).
+ */
+static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
+{
+	return kmalloc_array(n, size, flags | __GFP_ZERO);
 }

Quoted 2 lines

#if !defined(CONFIG_NUMA) && !defined(CONFIG_SLOB)

Does this want adding to Documentation/CodingStyle "Chapter 14: Allocating memory" ?

Quoted 2 lines · On Thu, 9 Feb 2012 23:47:42 +0100 (CET) Jesper Juhl <[email protected]> wrote:

include/linux/slab.h size_t ksize(const void *); · line 243

Quoted 2 lines

include/linux/slab.h +1 -1

-static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
+static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags)

[]

Quoted 2 lines

Does this want adding to Documentation/CodingStyle "Chapter 14: Allocating memory" ?

Spose so. We could possibly cook up a checkpatch rule too ;) <gad, CodingStyle has 18 chapters and an appendix??>

Well I think the best is just not do any of this. One can already do k[zm]alloc(x * sizeof(struct whatever)). Do x*x for 2 dimensions etc etc. No need to change the API.

If you add these variants then please think
about the necessity to add other variants (like the kmalloc_node() NUMA call) etc in the future.

The point was that there are a bunch of places where we have had integer overflows caused by doing kmalloc(x * sizeof(struct whatever)). For kzalloc(x * sizeof(struct whatever)), you just write it like kcalloc(x, sizeof(struct whatever)) and avoid the overflow, but we don't have a non-zeroing version of kcalloc() to do that.

Probably once we have the kmalloc_array() and people start using it, we get a bunch of overflow checking automatically and it's a kernel hardenning thing. As well we could remove the duplicative checking so it's a cleanup.

Quoted 4 lines

If you add these variants then please think
about the necessity to add other variants (like the kmalloc_node() NUMA call) etc in the future.

We don't have a kcalloc_node(), so I don't think this is likely to be a big issue.

SAFE_ARRAY_SIZE() would return the size if there were no overflow and -1 on errors? We can't return zero on errors because there are a lot of places which do zero size allocations and it's valid. It seems ugly.

I really think that's over thinking things. Let's just match kcalloc() exactly except without zeroing. The BUILD_BUG_ON() thing is an over complication as well. We haven't needed it for kcalloc().

I really think that's over thinking things. Let's just match kcalloc() exactly except without zeroing. The BUILD_BUG_ON() thing is an over complication as well. We haven't needed it for kcalloc().

SAFE_ARRAY_SIZE() would return the size if there were no overflow and -1 on errors? We can't return zero on errors because there are a lot of places which do zero size allocations and it's valid. It seems ugly.

I really think that's over thinking things. Let's just match kcalloc() exactly except without zeroing. The BUILD_BUG_ON() thing is an over complication as well. We haven't needed it for kcalloc().

We could also catch these issues with BUG() or WARN_ON() and then return zero.

Quoted 2 lines · Christoph Lameter

We could also catch these issues with BUG() or WARN_ON() and then return zero.

You cannot have SAFE_ARRAY_SIZE return 0 when an integer overflow occurs.

Besides, BUG() still allows DoS attacks and WARN_ON() would flood the log, especially if n is controlled from user space. Neither seems appropriate here.

You can if you check the results later. A zero size return would be an indication of an error. No need to pass it on to kmalloc.

Quoted 2 lines · Christoph Lameter

You can if you check the results later. A zero size return would be an indication of an error. No need to pass it on to kmalloc.

Maybe I misunderstood something here. How do you not pass it to kmalloc() with kmalloc(SAFE_ARRAY_SIZE(n, size), ...)?

This patch still preserves kcalloc. But note that if kcalloc returns NULL then multiple conditions may have caused it. One is that the array is simply too large. The other may be that such an allocation is not possible due to fragmentation.

Subject: Introduce calculate_array_size

calculate_array_size calculates the size of an array while checking for errors. Returns 0 if the size is too large.

Signed-off-by Christoph Lameter <[email protected]>

---
 include/linux/slab.h |   15 +++++++++++++++
 mm/util.c            |    9 ++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/slab.h
===================================================================

include/linux/slab.h +15 -0

@@ -242,6 +242,21 @@ size_t ksize(const void *);
  */
 void *kcalloc(size_t n, size_t size, gfp_t flags);
 
+/*
+ * calculate_array_size - Calculate an array size given the size of a
+ * particular element with checking for overflow.
+ *
+ * Return 0 if there is an overflow.
+ */
+static inline long calculate_array_size(size_t n, size_t size)
+{
+	if (size != 0 && n > ULONG_MAX / size)
+
+		return 0;
+
+	return n * size;
+}
+
 #if !defined(CONFIG_NUMA) && !defined(CONFIG_SLOB)
 /**
  * kmalloc_node - allocate memory from a specific node

Index: linux-2.6/mm/util.c
===================================================================

mm/util.c +0 -1

@@ -83,9 +83,12 @@ EXPORT_SYMBOL(kmemdup);
  */
 void *kcalloc(size_t n, size_t size, gfp_t flags)
 {
-	if (size != 0 && n > ULONG_MAX / size)

This patch still preserves kcalloc. But note that if kcalloc returns NULL then multiple conditions may have caused it. One is that the array is simply too large. The other may be that such an allocation is not possible due to fragmentation.

+}

Quoted 4 lines · Christoph Lameter

This patch still preserves kcalloc. But note that if kcalloc returns NULL then multiple conditions may have caused it. One is that the array is simply too large. The other may be that such an allocation is not possible due to fragmentation.

[...]

Review

Quoted 5 lines

(quoted diff) +5 -0

+static inline long calculate_array_size(size_t n, size_t size)
+{
+	if (size != 0 && n > ULONG_MAX / size)
+
+		return 0;

This isn't right. The above tests whether or not the result of the multiplication will not be representable in an 'unsigned long'...

Review

Quoted 1 line

(quoted diff) +1 -0

+	return n * size;

but then the result is assigned to a (signed) long, which may overflow if it's greater than LONG_MAX.

Quoted 1 line

+}

[...]

Review

Quoted 12 lines

void *kcalloc(size_t n, size_t size, gfp_t flags) {

(quoted diff) +6 -3

-	if (size != 0 && n > ULONG_MAX / size)
-		return NULL;
-	return __kmalloc(n * size, flags | __GFP_ZERO);
+	size_t s = calculate_array_size(n, size);
+
+	if (s)
+		return kzalloc(s, flags);
+
+	return NULL;
 }

This hunk changes the behaviour of kcalloc if either of the two size parameters is 0.

Quoted 6 lines · Nick Bowler

Quoted 4 lines · Christoph Lameter

This patch still preserves kcalloc. But note that if kcalloc returns NULL then multiple conditions may have caused it. One is that the array is simply too large. The other may be that such an allocation is not possible due to fragmentation.

[...]

Quoted 8 lines

include/linux/slab.h size_t ksize(const void *); · line 255

Quoted 5 lines

include/linux/slab.h +5 -0

+static inline long calculate_array_size(size_t n, size_t size)
+{
+	if (size != 0 && n > ULONG_MAX / size)
+
+		return 0;

This isn't right. The above tests whether or not the result of the multiplication will not be representable in an 'unsigned long'...

Yes and so does the current kcalloc.

Quoted 4 lines

include/linux/slab.h size_t ksize(const void *); · line 257

Quoted 1 line

include/linux/slab.h +1 -0

+	return n * size;

but then the result is assigned to a (signed) long, which may overflow if it's greater than LONG_MAX.

That can happen?

Quoted 16 lines

[...]

mm/util.c EXPORT_SYMBOL(kmemdup); · line 92

Quoted 12 lines

void *kcalloc(size_t n, size_t size, gfp_t flags) {

mm/util.c +6 -3

-	if (size != 0 && n > ULONG_MAX / size)
-		return NULL;
-	return __kmalloc(n * size, flags | __GFP_ZERO);
+	size_t s = calculate_array_size(n, size);
+
+	if (s)
+		return kzalloc(s, flags);
+
+	return NULL;
 }

This hunk changes the behaviour of kcalloc if either of the two size parameters is 0.

You want ZERO_PTR returns?

NULL is one permissible return value of calloc() if size == 0. So we are now deviating from user space conventions.

Quoted 17 lines · Christoph Lameter

Quoted 14 lines · Nick Bowler

Quoted 4 lines · Christoph Lameter

This patch still preserves kcalloc. But note that if kcalloc returns NULL then multiple conditions may have caused it. One is that the array is simply too large. The other may be that such an allocation is not possible due to fragmentation.

[...]

include/linux/slab.h size_t ksize(const void *); · line 255

Quoted 5 lines

include/linux/slab.h +5 -0

+static inline long calculate_array_size(size_t n, size_t size)
+{
+	if (size != 0 && n > ULONG_MAX / size)
+
+		return 0;

This isn't right. The above tests whether or not the result of the multiplication will not be representable in an 'unsigned long'...

Quoted 1 line

Yes and so does the current kcalloc.

Well, the current kcalloc doesn't assign the result to a signed long. However, it does assign the result to a size_t, which makes one wonder why it's not testing against SIZE_MAX.

If size_t has the same range as unsigned long on all architectures, then this confusion doesn't matter, but is that actually the case?

Quoted 6 lines

Quoted 4 lines

include/linux/slab.h size_t ksize(const void *); · line 257

Quoted 1 line

include/linux/slab.h +1 -0

+	return n * size;

but then the result is assigned to a (signed) long, which may overflow if it's greater than LONG_MAX.

That can happen?

Yes, because LONG_MAX (the maximum value of your return type) is strictly less than ULONG_MAX (what you test against). It's not hard to pick input numbers that multiply to something between LONG_MAX and ULONG_MAX, which will cause your function to return a negative value (standard C leaves the result of such a conversion implementation- defined, but I'll assume for now that it works this way for everything that compiles Linux).

Admittedly, your kcalloc change then assigns this negative value to a size_t, which will result in the correct positive value assuming SIZE_MAX == ULONG_MAX, but that's gratuitously roundabout.

[...]

Quoted 17 lines

Quoted 16 lines

[...]

mm/util.c EXPORT_SYMBOL(kmemdup); · line 92

Quoted 12 lines

void *kcalloc(size_t n, size_t size, gfp_t flags) {

mm/util.c +6 -3

-	if (size != 0 && n > ULONG_MAX / size)
-		return NULL;
-	return __kmalloc(n * size, flags | __GFP_ZERO);
+	size_t s = calculate_array_size(n, size);
+
+	if (s)
+		return kzalloc(s, flags);
+
+	return NULL;
 }

This hunk changes the behaviour of kcalloc if either of the two size parameters is 0.

Quoted 4 lines

You want ZERO_PTR returns?

NULL is one permissible return value of calloc() if size == 0. So we are now deviating from user space conventions.

Sort of. While standard C leaves it implementation-defined whether successful zero-sized allocations are possible, all sane implementations let them succeed. Hence, portable C apps need to handle 0 as a special case, because there are insane implementations out there. There's no reason for the kernel to be one of them.

Regardless, this was still a (presumably unintentional) change from the previous behaviour.

Cheers,

Subject: Uninline kcalloc

kcalloc is not used in performance critical ways. So it does not need to be inline. If we would add diagnostics to track the overflow occurrences then such code would be replicated at all call sites in the kernel.

Quoted 5 lines · On Tue, 14 Feb 2012 13:33:40 -0600 (CST) Christoph Lameter <[email protected]> wrote:

Subject: Uninline kcalloc

kcalloc is not used in performance critical ways. So it does not need to be inline. If we would add diagnostics to track the overflow occurrences then such code would be replicated at all call sites in the kernel.

Uninlining kcalloc() seems reasonable.  But if we're going to uninline kcalloc() then we also should uninline kmalloc_array().

And yes, it's still called kmalloc_array() in my tree.  I've been following this discussion for N days waiting for a reason for changing the original patch and I ain't seen one yet.

Me neither. I don't think Christoph's SAFE_ARRAY_SIZE() suggestion makes much sense, really. It's more verbose, less obvious API, and doesn't really deal with the overflow case cleanly.

Quoted 3 lines

Me neither. I don't think Christoph's SAFE_ARRAY_SIZE() suggestion makes much sense, really. It's more verbose, less obvious API, and doesn't really deal with the overflow case cleanly.

IMHO Having a function to deal with the overflow of a multiplication and then do an allocation based on the result is a conflation of two different things that need to be separate. kcalloc only exists because there is an ancient user space function that somehow got a second parameter instead of just using the same as malloc().

Quoted 3 lines · Pekka Enberg

Quoted 3 lines

Me neither. I don't think Christoph's SAFE_ARRAY_SIZE() suggestion makes much sense, really. It's more verbose, less obvious API, and doesn't really deal with the overflow case cleanly.

Quoted 5 lines · Christoph Lameter

IMHO Having a function to deal with the overflow of a multiplication and then do an allocation based on the result is a conflation of two different things that need to be separate. kcalloc only exists because there is an ancient user space function that somehow got a second parameter instead of just using the same as malloc().

It's an ancient userspace function that everybody knows how to use. There really isn't big enough gains from SAFE_ARRAY_SIZE() to justify a new API. And it's not as if the macro is an elegant way to solve the problem at hand either.

IMHO Having a function to deal with the overflow of a multiplication and then do an allocation based on the result is a conflation of two different things that need to be separate. kcalloc only exists because there is an ancient user space function that somehow got a second parameter instead of just using the same as malloc().

Quoted 5 lines · Christoph Lameter

IMHO Having a function to deal with the overflow of a multiplication and then do an allocation based on the result is a conflation of two different things that need to be separate. kcalloc only exists because there is an ancient user space function that somehow got a second parameter instead of just using the same as malloc().

I don't understand why these kcalloc patches have anything to do with kmalloc(SAFE_ARRAY_SIZE(...), ...) you proposed.

It also doesn't make much sense to force the caller to check the result of SAFE_ARRAY_SIZE() or calculate_array_size() before passing it to kmalloc(). This is too verbose.

kcalloc is still there. Certainly useful for legacy purposes. But I'd feel better if I had fine grained control over the size of my allocation rather than rely on the slab allocators to check up on my multiplication.

With these patches both is possible. And if you want the check of an allocation that is not zeroed then you can do so because you have a function that will perform the size check for you without calling into the slab allocator.

for overflow, one has to write:

        size_t s = calculate_array_size(n, size);
        if (s)
                p = kmalloc(s, ...);

This "if" thing is just too verbose --- you need three lines to allocate an array.

We could change calculate_array_size() to return ULONG_MAX or some large number with which kmalloc() would fail. Then one would write:

Any allocation larger than MAX_ORDER << PAGE_SHIFT will fail since the page allocator cannot serve larger contigous pages.

Quoted 2 lines · Christoph Lameter

Any allocation larger than MAX_ORDER << PAGE_SHIFT will fail since the page allocator cannot serve larger contigous pages.

Yes, any number larger than KMALLOC_MAX_SIZE would do the trick. The question is whether people would like to adopt

kmalloc(ARRAY_BYTES(n, size), ...);

Then there is

vzalloc
kzalloc
vmalloc_32
alloc_bootmem (MAXORDER limit may not work)
alloc_remap

...

This would also work for special subsystem allocations like

usb_alloc_coherent
dm_vcalloc
devres_alloc

....

The use of a function or macro makes the overflow check much more universal and allows these array allocations to occur with different allocation functions throughout the kernel.

Quoted 3 lines

The use of a function or macro makes the overflow check much more universal and allows these array allocations to occur with different allocation functions throughout the kernel.

No, it does NOT. It can be easily misued to introduce more bugs.

1) Should calculate_array_size() return 0 on overflow, as you orginally proposed?

No, as Dan, Pekka, and some others already pointed out.

2) Should calculate_array_size() return something like KMALLOC_MAX_SIZE + 1?

No, because you intended to use it with other allocators such as vmalloc().

3) Should calculate_array_size() return ULONG_MAX/SIZE_MAX/-1?

No! Consider devres_alloc() you mentioned. Then you do

devres_alloc(..., calculate_array_size(n, size), ...).

It internally invokes kmalloc() with allocation size:

sizeof(struct devres) + calculate_array_size(n, size).

include/linux/slab.h +1 -1

-static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
+static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags)

 {
       if (size != 0 && n > ULONG_MAX / size) return NULL;
-       return __kmalloc(n * size, flags | __GFP_ZERO);
+       return __kmalloc(n * size, flags);
+}

Quoted 8 lines · Xi Wang

include/linux/slab.h +1 -1

-static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
+static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags)

 {
       if (size != 0 && n > ULONG_MAX / size) return NULL;
-       return __kmalloc(n * size, flags | __GFP_ZERO);
+       return __kmalloc(n * size, flags);
+}

It should be named kaalloc(), I think.
Why it is ULONG_MAX, when size_t is used?

Quoted 1 line

It should be named kaalloc(), I think.

Quoted 1 line

Why it is ULONG_MAX, when size_t is used?

Also, it could be written more "robust" against people who will make sizeof() the first argument with __builtin_constant_p().

Quoted 2 lines · Alexey Dobriyan

Also, it could be written more "robust" against people who will make sizeof() the first argument with __builtin_constant_p().

Do you mean something like this?

 BUILD_BUG_ON(__builtin_constant_p(n));

or

 BUILD_BUG_ON(__builtin_constant_p(n) && !__builtin_constant_p(size));

- xi

No,

If one dimension is constant, limit should be divided by it, so compiler would have less chance
to screw up compile time evaluation.

Quoted 5 lines · Alexey Dobriyan

No,

If one dimension is constant, limit should be divided by it, so compiler would have less chance
to screw up compile time evaluation.

I think the BUILD_BUG_ON macro can prevent that misuse.

But there is no misuse.

Both kaalloc(non-const, const) and kaalloc(const, non-const) are OK. It's just first case happens more often.

Quoted 10 lines · On Wed, 08 Feb 2012 09:34:16 +0100 Jens Axboe <[email protected]> wrote:

Quoted 7 lines · Dan Carpenter

My static checker is warning about integer overflows in kernel/relay.c

relay_create_buf()
170

   171          buf->padding = kmalloc(chan->n_subbufs * sizeof(size_t *), GFP_KERNEL);
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This can only overflow on 32bit systems.

Correct

This is a relatively common problem. Converting a kzalloc() call to kcalloc() fixes it. But we do not have a non-zeroing version of kcalloc().

Please, someone complete this patch and send it to me!

Quoted 21 lines · Andrew Morton

Quoted 10 lines · On Wed, 08 Feb 2012 09:34:16 +0100 Jens Axboe <[email protected]> wrote:

Quoted 7 lines · Dan Carpenter

My static checker is warning about integer overflows in kernel/relay.c

relay_create_buf()
170

   171          buf->padding = kmalloc(chan->n_subbufs * sizeof(size_t *), GFP_KERNEL);
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This can only overflow on 32bit systems.

Correct

This is a relatively common problem. Converting a kzalloc() call to kcalloc() fixes it. But we do not have a non-zeroing version of kcalloc().

Please, someone complete this patch and send it to me!

include/linux/slab.h size_t ksize(const void *); · line 243

Quoted 8 lines

include/linux/slab.h renamed from include/linux/slab.h~a +1 -1

@@ -240,11 +240,16 @@ size_t ksize(const void *);
  * for general use, and so are not documented here. For a full list of
  * potential flags, always refer to linux/gfp.h.
  */
-static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
+static inline void *wtf_do_i_call_this(size_t n, size_t size, gfp_t flags)

kzcalloc()? Either that, or long_and_verbose().

Quoted 10 lines · On Wed, 08 Feb 2012 09:34:16 +0100 Jens Axboe <[email protected]> wrote:

Quoted 7 lines · Dan Carpenter

My static checker is warning about integer overflows in kernel/relay.c

relay_create_buf()
   170
   171          buf->padding = kmalloc(chan->n_subbufs * sizeof(size_t *), GFP_KERNEL); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This can only overflow on 32bit systems.

Correct

This is a relatively common problem.  Converting a kzalloc() call to kcalloc() fixes it.  But we do not have a non-zeroing version of kcalloc().

Please, someone complete this patch and send it to me!

include/linux/slab.h renamed from include/linux/slab.h~a +2 -1

  {
        if (size != 0 && n > ULONG_MAX / size)
                return NULL;
-       return __kmalloc(n * size, flags | __GFP_ZERO);
+       return __kmalloc(n * size, flags);
+}

"subbuf_size" and "n_subbufs" come from the user and they need to be capped to prevent an integer overflow.

Signed-off-by Dan Carpenter <[email protected]>

kernel/relay.c +6 -2

index 4335e1d..ab56a17 100644
@@ -164,10 +164,14 @@ depopulate:
  */
 static struct rchan_buf *relay_create_buf(struct rchan *chan)
 {
-	struct rchan_buf *buf = kzalloc(sizeof(struct rchan_buf), GFP_KERNEL);
-	if (!buf)
+	struct rchan_buf *buf;
+
+	if (chan->n_subbufs > UINT_MAX / sizeof(size_t *))
 		return NULL;
 
+	buf = kzalloc(sizeof(struct rchan_buf), GFP_KERNEL);
+	if (!buf)
+		return NULL;
 	buf->padding = kmalloc(chan->n_subbufs * sizeof(size_t *), GFP_KERNEL);
 	if (!buf->padding)
 		goto free_buf;

numerical this is ok, but ...
maybe it is better to cap the chan->n_subbufs at a useful number ?

The user can still allocate an insane number of bytes.
Restricting subbuf_size*n_subbufs seems more logical (otherwise is this a real problem ?)