Several other "dup"-style interfaces could use the __realloc_size()
attribute. (As a reminder to myself and others: "realloc" is used here
instead of "alloc" because the "alloc_size" attribute implies that the
memory contents are uninitialized. Since we're copying contents into the
resulting allocation, it must use "realloc_size" to avoid confusing the
compiler's optimization passes.)
Add KUnit test coverage where possible. (KUnit still does not have the
ability to manipulate userspace memory.)
Signed-off-by: Kees Cook <[email protected]>
---
Cc: Andy Shevchenko <[email protected]>
Cc: [email protected]
---
include/linux/string.h | 13 ++++++++-----
lib/fortify_kunit.c | 26 ++++++++++++++++++++++++++
2 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index 86aa6cd35167..10e5177bb49c 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -14,8 +14,8 @@
#include <uapi/linux/string.h>
extern char *strndup_user(const char __user *, long);
-extern void *memdup_user(const void __user *, size_t);
-extern void *vmemdup_user(const void __user *, size_t);
+extern void *memdup_user(const void __user *, size_t) __realloc_size(2);
+extern void *vmemdup_user(const void __user *, size_t) __realloc_size(2);
extern void *memdup_user_nul(const void __user *, size_t);
/**
@@ -27,7 +27,8 @@ extern void *memdup_user_nul(const void __user *, size_t);
* Return: an ERR_PTR() on failure. Result is physically
* contiguous, to be freed by kfree().
*/
-static inline void *memdup_array_user(const void __user *src, size_t n, size_t size)
+static inline __realloc_size(2, 3)
+void *memdup_array_user(const void __user *src, size_t n, size_t size)
{
size_t nbytes;
@@ -46,7 +47,8 @@ static inline void *memdup_array_user(const void __user *src, size_t n, size_t s
* Return: an ERR_PTR() on failure. Result may be not
* physically contiguous. Use kvfree() to free.
*/
-static inline void *vmemdup_array_user(const void __user *src, size_t n, size_t size)
+static inline __realloc_size(2, 3)
+void *vmemdup_array_user(const void __user *src, size_t n, size_t size)
{
size_t nbytes;
@@ -285,7 +287,8 @@ extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
extern void *kmemdup(const void *src, size_t len, gfp_t gfp) __realloc_size(2);
extern void *kvmemdup(const void *src, size_t len, gfp_t gfp) __realloc_size(2);
extern char *kmemdup_nul(const char *s, size_t len, gfp_t gfp);
-extern void *kmemdup_array(const void *src, size_t element_size, size_t count, gfp_t gfp);
+extern void *kmemdup_array(const void *src, size_t element_size, size_t count, gfp_t gfp)
+ __realloc_size(2, 3);
/* lib/argv_split.c */
extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c
index 306522fd0aa2..87b873108350 100644
--- a/lib/fortify_kunit.c
+++ b/lib/fortify_kunit.c
@@ -363,6 +363,31 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)
} while (0)
DEFINE_ALLOC_SIZE_TEST_PAIR(devm_kmalloc)
+static const char * const test_phrases[] = {
+ "",
+ "Hello there",
+ "A longer string, just for variety",
+};
+
+#define TEST_realloc(checker) do { \
+ gfp_t gfp = GFP_KERNEL; \
+ size_t len; \
+ int idx; \
+ \
+ for (idx = 0; idx < ARRAY_SIZE(test_phrases); idx++) { \
+ len = strlen(test_phrases[idx]); \
+ KUNIT_EXPECT_EQ(test, __builtin_constant_p(len), 0); \
+ checker(len, kmemdup_array(test_phrases[idx], len, 1, \
+ gfp), kfree(p)); \
+ checker(len, kmemdup(test_phrases[idx], len, gfp), \
+ kfree(p)); \
+ } \
+} while (0)
+static void fortify_test_realloc_size(struct kunit *test)
+{
+ TEST_realloc(check_dynamic);
+}
+
/*
* We can't have an array at the end of a structure or else
* builds without -fstrict-flex-arrays=3 will report them as
@@ -1046,6 +1071,7 @@ static struct kunit_case fortify_test_cases[] = {
KUNIT_CASE(fortify_test_alloc_size_kvmalloc_dynamic),
KUNIT_CASE(fortify_test_alloc_size_devm_kmalloc_const),
KUNIT_CASE(fortify_test_alloc_size_devm_kmalloc_dynamic),
+ KUNIT_CASE(fortify_test_realloc_size),
KUNIT_CASE(fortify_test_strlen),
KUNIT_CASE(fortify_test_strnlen),
KUNIT_CASE(fortify_test_strcpy),
--
2.34.1
On Thu, May 2, 2024 at 2:32 AM Kees Cook <[email protected]> wrote:
>
> Several other "dup"-style interfaces could use the __realloc_size()
> attribute. (As a reminder to myself and others: "realloc" is used here
> instead of "alloc" because the "alloc_size" attribute implies that the
> memory contents are uninitialized. Since we're copying contents into the
> resulting allocation, it must use "realloc_size" to avoid confusing the
> compiler's optimization passes.)
>
> Add KUnit test coverage where possible. (KUnit still does not have the
> ability to manipulate userspace memory.)
Makes sense to me,
Reviewed-by: Andy Shevchenko <[email protected]>
..
> + checker(len, kmemdup_array(test_phrases[idx], len, 1, \
> + gfp), kfree(p)); \
Despite being longer, I would put gfp on the previous line for the
sake of logical split (and additionally to be consistent with the
below).
> + checker(len, kmemdup(test_phrases[idx], len, gfp), \
> + kfree(p)); \
--
With Best Regards,
Andy Shevchenko
On Thu, May 02, 2024 at 12:45:33PM +0300, Andy Shevchenko wrote:
> On Thu, May 2, 2024 at 2:32 AM Kees Cook <[email protected]> wrote:
> >
> > Several other "dup"-style interfaces could use the __realloc_size()
> > attribute. (As a reminder to myself and others: "realloc" is used here
> > instead of "alloc" because the "alloc_size" attribute implies that the
> > memory contents are uninitialized. Since we're copying contents into the
> > resulting allocation, it must use "realloc_size" to avoid confusing the
> > compiler's optimization passes.)
> >
> > Add KUnit test coverage where possible. (KUnit still does not have the
> > ability to manipulate userspace memory.)
>
> Makes sense to me,
> Reviewed-by: Andy Shevchenko <[email protected]>
>
> ...
>
> > + checker(len, kmemdup_array(test_phrases[idx], len, 1, \
> > + gfp), kfree(p)); \
>
> Despite being longer, I would put gfp on the previous line for the
> sake of logical split (and additionally to be consistent with the
> below).
>
> > + checker(len, kmemdup(test_phrases[idx], len, gfp), \
> > + kfree(p)); \
Oh good! This bothered me to no end too. I will adjust it. :) Thanks!
-Kees
--
Kees Cook