[Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc

Discussion:

[Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

Kees Cook

2018-02-14 19:22:38 UTC

We have kvmalloc_array in order to safely allocate an array with a
number of elements specified by userspace (avoiding arithmetic overflow
leading to a buffer overrun). But it's fairly common to have a header
in front of that array (eg specifying the length of the array), so we
need a helper function for that situation.
kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
of our best efforts to name the arguments, it's really hard to remember
which order to put the arguments in. kvzalloc_struct() eliminates that
effort; you tell it about the struct you're allocating, and it puts the
arguments in the right order for you (and checks that the arguments
you've given are at least plausible).
sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
GFP_KERNEL);
sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
GFP_KERNEL);
sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
---
include/linux/mm.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 81bd7f0be286..ddf929c5aaee 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -557,6 +557,57 @@ static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
return kvmalloc(n * size, flags);
}
+/**
+ * kvmalloc_ab_c() - Allocate memory.

Longer description, maybe? "Allocate a *b + c bytes of memory"?
If these should be constant, should we mark them as "const"? Or WARN
if __builtin_constant_p() isn't true?

+ *
+ * return %NULL if the required amount of memory cannot be allocated.
+ * Use kvfree() to free the allocated memory.
+ *
+ * The kvzalloc_hdr_arr() function is easier to use as it has typechecking

renaming typo? Should this be "kvzalloc_struct()"?

+ * and you do not need to remember which of the arguments should be constants.
+ *
+ * %GFP_KERNEL.
+ * Return: A pointer to the allocated memory or %NULL.
+ */
+static inline __must_check
+void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
+{
+ if (size != 0 && n > (SIZE_MAX - c) / size)
+ return NULL;
+
+ return kvmalloc(n * size + c, gfp);
+}
+#define kvzalloc_ab_c(a, b, c, gfp) kvmalloc_ab_c(a, b, c, gfp | __GFP_ZERO)

Nit: "(gfp) | __GFP_ZERO" just in case of insane usage.

+
+/**
+ * kvzalloc_struct() - Allocate and zero-fill a structure containing a
+ * variable length array.
+ *
+ * Allocate (and zero-fill) enough memory for a structure with an array
+ * userspace as the arithmetic will not overflow.
+ * Use kvfree() to free the allocated memory.
+ *
+ * %GFP_KERNEL.
+ * Return: Zero-filled memory or a NULL pointer.
+ */
+#define kvzalloc_struct(p, member, n, gfp) \
+ (typeof(p))kvzalloc_ab_c(n, \
+ sizeof(*(p)->member) + __must_be_array((p)->member), \
+ offsetof(typeof(*(p)), member), gfp)
+
extern void kvfree(const void *addr);
static inline atomic_t *compound_mapcount_ptr(struct page *page)

It might be nice to include another patch that replaces some of the
existing/common uses of a*b+c with the new function...

Otherwise, yes, please. We could build a coccinelle rule for
additional replacements...

-Kees

--
Kees Cook
Pixel Security

Julia Lawall

2018-02-14 19:27:46 UTC