[PATCH v3] lib: Convert test_user_copy to KUnit test

[PATCH v3] lib: Convert test_user_copy to KUnit test

[Vitor Massaru Iha]

This adds the conversion of the runtime tests of test_user_copy fuctions,
from `lib/test_user_copy.c`to KUnit tests.

Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
---
v2:
    * splitted patch in 3:
        - Allows to install and load modules in root filesystem;
        - Provides an userspace memory context when tests are compiled
          as module;
        - Convert test_user_copy to KUnit test;
    * removed entry for CONFIG_TEST_USER_COPY;
    * replaced pr_warn to KUNIT_EXPECT_FALSE_MSG in test macro to
      decrease the diff;
v3:
    * rebased with last kunit branch
    * Please apply this commit from kunit-fixes:
        3f37d14b8a3152441f36b6bc74000996679f0998
      And these from patchwork:
        https://patchwork.kernel.org/patch/11676331/
        https://patchwork.kernel.org/patch/11676335/
---
 lib/Kconfig.debug                           | 28 ++++++++------
 lib/Makefile                                |  2 +-
 lib/{test_user_copy.c => user_copy_kunit.c} | 42 +++++++++------------
 3 files changed, 35 insertions(+), 37 deletions(-)
 rename lib/{test_user_copy.c => user_copy_kunit.c} (91%)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9ad9210d70a1..f699a3624ae7 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2078,18 +2078,6 @@ config TEST_VMALLOC

 	  If unsure, say N.

-config TEST_USER_COPY
-	tristate "Test user/kernel boundary protections"
-	depends on m
-	help
-	  This builds the "test_user_copy" module that runs sanity checks
-	  on the copy_to/from_user infrastructure, making sure basic
-	  user/kernel boundary testing is working. If it fails to load,
-	  a regression has been detected in the user/kernel memory boundary
-	  protections.
-
-	  If unsure, say N.
-
 config TEST_BPF
 	tristate "Test BPF filter functionality"
 	depends on m && NET
@@ -2154,6 +2142,22 @@ config SYSCTL_KUNIT_TEST

 	  If unsure, say N.

+config USER_COPY_KUNIT
+	tristate "KUnit Test for user/kernel boundary protections"
+	depends on KUNIT
+	depends on m
+	help
+	  This builds the "user_copy_kunit" module that runs sanity checks
+	  on the copy_to/from_user infrastructure, making sure basic
+	  user/kernel boundary testing is working. If it fails to load,
+	  a regression has been detected in the user/kernel memory boundary
+	  protections.
+
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
+
 config LIST_KUNIT_TEST
 	tristate "KUnit Test for Kernel Linked-list structures" if !KUNIT_ALL_TESTS
 	depends on KUNIT
diff --git a/lib/Makefile b/lib/Makefile
index b1c42c10073b..8c145f85accc 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
 obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
 obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
 obj-$(CONFIG_TEST_SORT) += test_sort.o
-obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
 obj-$(CONFIG_TEST_PRINTF) += test_printf.o
@@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
 # KUnit tests
 obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
 obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
+obj-$(CONFIG_USER_COPY_KUNIT) += user_copy_kunit.o
diff --git a/lib/test_user_copy.c b/lib/user_copy_kunit.c
similarity index 91%
rename from lib/test_user_copy.c
rename to lib/user_copy_kunit.c
index 5ff04d8fe971..a10ddd15b4cd 100644
--- a/lib/test_user_copy.c
+++ b/lib/user_copy_kunit.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
+#include <kunit/test.h>

 /*
  * Several 32-bit architectures support 64-bit {get,put}_user() calls.
@@ -35,7 +36,7 @@
 ({									\
 	int cond = (condition);						\
 	if (cond)							\
-		pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__);	\
+		KUNIT_EXPECT_FALSE_MSG(test, cond, msg, ##__VA_ARGS__);	\
 	cond;								\
 })

@@ -44,7 +45,7 @@ static bool is_zeroed(void *from, size_t size)
 	return memchr_inv(from, 0x0, size) == NULL;
 }

-static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
+static int test_check_nonzero_user(struct kunit *test, char *kmem, char __user *umem, size_t size)
 {
 	int ret = 0;
 	size_t start, end, i, zero_start, zero_end;
@@ -102,7 +103,7 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
 	return ret;
 }

-static int test_copy_struct_from_user(char *kmem, char __user *umem,
+static int test_copy_struct_from_user(struct kunit *test, char *kmem, char __user *umem,
 				      size_t size)
 {
 	int ret = 0;
@@ -177,7 +178,7 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
 	return ret;
 }

-static int __init test_user_copy_init(void)
+static void user_copy_test(struct kunit *test)
 {
 	int ret = 0;
 	char *kmem;
@@ -192,16 +193,14 @@ static int __init test_user_copy_init(void)
 #endif

 	kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
-	if (!kmem)
-		return -ENOMEM;
+	KUNIT_EXPECT_FALSE_MSG(test, kmem == NULL, "kmalloc failed");

 	user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
 			    PROT_READ | PROT_WRITE | PROT_EXEC,
 			    MAP_ANONYMOUS | MAP_PRIVATE, 0);
 	if (user_addr >= (unsigned long)(TASK_SIZE)) {
-		pr_warn("Failed to allocate user memory\n");
 		kfree(kmem);
-		return -ENOMEM;
+		KUNIT_FAIL(test, "Failed to allocate user memory");
 	}

 	usermem = (char __user *)user_addr;
@@ -245,9 +244,9 @@ static int __init test_user_copy_init(void)
 #undef test_legit

 	/* Test usage of check_nonzero_user(). */
-	ret |= test_check_nonzero_user(kmem, usermem, 2 * PAGE_SIZE);
+	ret |= test_check_nonzero_user(test, kmem, usermem, 2 * PAGE_SIZE);
 	/* Test usage of copy_struct_from_user(). */
-	ret |= test_copy_struct_from_user(kmem, usermem, 2 * PAGE_SIZE);
+	ret |= test_copy_struct_from_user(test, kmem, usermem, 2 * PAGE_SIZE);

 	/*
 	 * Invalid usage: none of these copies should succeed.
@@ -309,23 +308,18 @@ static int __init test_user_copy_init(void)

 	vm_munmap(user_addr, PAGE_SIZE * 2);
 	kfree(kmem);
-
-	if (ret == 0) {
-		pr_info("tests passed.\n");
-		return 0;
-	}
-
-	return -EINVAL;
 }

-module_init(test_user_copy_init);
-
-static void __exit test_user_copy_exit(void)
-{
-	pr_info("unloaded.\n");
-}
+static struct kunit_case user_copy_test_cases[] = {
+	KUNIT_CASE(user_copy_test),
+	{}
+};

-module_exit(test_user_copy_exit);
+static struct kunit_suite user_copy_test_suite = {
+	.name = "user_copy",
+	.test_cases = user_copy_test_cases,
+};

+kunit_test_suites(&user_copy_test_suite);
 MODULE_AUTHOR("Kees Cook <keescook@chromium.org>");
 MODULE_LICENSE("GPL");

base-commit: d43c7fb05765152d4d4a39a8ef957c4ea14d8847
--
2.26.2

Re: [PATCH v3] lib: Convert test_user_copy to KUnit test

[Kees Cook]

On Tue, Jul 21, 2020 at 02:46:54PM -0300, Vitor Massaru Iha wrote:
> This adds the conversion of the runtime tests of test_user_copy fuctions,
> from `lib/test_user_copy.c`to KUnit tests.
> 
> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
> ---
> v2:
>     * splitted patch in 3:
>         - Allows to install and load modules in root filesystem;
>         - Provides an userspace memory context when tests are compiled
>           as module;
>         - Convert test_user_copy to KUnit test;
>     * removed entry for CONFIG_TEST_USER_COPY;
>     * replaced pr_warn to KUNIT_EXPECT_FALSE_MSG in test macro to
>       decrease the diff;
> v3:
>     * rebased with last kunit branch
>     * Please apply this commit from kunit-fixes:
>         3f37d14b8a3152441f36b6bc74000996679f0998
>       And these from patchwork:
>         https://patchwork.kernel.org/patch/11676331/
>         https://patchwork.kernel.org/patch/11676335/
> ---
>  lib/Kconfig.debug                           | 28 ++++++++------
>  lib/Makefile                                |  2 +-
>  lib/{test_user_copy.c => user_copy_kunit.c} | 42 +++++++++------------
>  3 files changed, 35 insertions(+), 37 deletions(-)
>  rename lib/{test_user_copy.c => user_copy_kunit.c} (91%)
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 9ad9210d70a1..f699a3624ae7 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2078,18 +2078,6 @@ config TEST_VMALLOC
> 
>  	  If unsure, say N.
> 
> -config TEST_USER_COPY
> -	tristate "Test user/kernel boundary protections"
> -	depends on m
> -	help
> -	  This builds the "test_user_copy" module that runs sanity checks
> -	  on the copy_to/from_user infrastructure, making sure basic
> -	  user/kernel boundary testing is working. If it fails to load,
> -	  a regression has been detected in the user/kernel memory boundary
> -	  protections.
> -
> -	  If unsure, say N.
> -
>  config TEST_BPF
>  	tristate "Test BPF filter functionality"
>  	depends on m && NET
> @@ -2154,6 +2142,22 @@ config SYSCTL_KUNIT_TEST
> 
>  	  If unsure, say N.
> 
> +config USER_COPY_KUNIT
> +	tristate "KUnit Test for user/kernel boundary protections"
> +	depends on KUNIT
> +	depends on m
> +	help
> +	  This builds the "user_copy_kunit" module that runs sanity checks
> +	  on the copy_to/from_user infrastructure, making sure basic
> +	  user/kernel boundary testing is working. If it fails to load,
> +	  a regression has been detected in the user/kernel memory boundary
> +	  protections.
> +
> +	  For more information on KUnit and unit tests in general please refer
> +	  to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> +	  If unsure, say N.
> +
>  config LIST_KUNIT_TEST
>  	tristate "KUnit Test for Kernel Linked-list structures" if !KUNIT_ALL_TESTS
>  	depends on KUNIT
> diff --git a/lib/Makefile b/lib/Makefile
> index b1c42c10073b..8c145f85accc 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
>  obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
>  obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
>  obj-$(CONFIG_TEST_SORT) += test_sort.o
> -obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
>  obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
>  obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
>  obj-$(CONFIG_TEST_PRINTF) += test_printf.o
> @@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
>  # KUnit tests
>  obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
>  obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> +obj-$(CONFIG_USER_COPY_KUNIT) += user_copy_kunit.o
> diff --git a/lib/test_user_copy.c b/lib/user_copy_kunit.c
> similarity index 91%
> rename from lib/test_user_copy.c
> rename to lib/user_copy_kunit.c
> index 5ff04d8fe971..a10ddd15b4cd 100644
> --- a/lib/test_user_copy.c
> +++ b/lib/user_copy_kunit.c
> @@ -16,6 +16,7 @@
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/vmalloc.h>
> +#include <kunit/test.h>
> 
>  /*
>   * Several 32-bit architectures support 64-bit {get,put}_user() calls.
> @@ -35,7 +36,7 @@
>  ({									\
>  	int cond = (condition);						\
>  	if (cond)							\
> -		pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__);	\
> +		KUNIT_EXPECT_FALSE_MSG(test, cond, msg, ##__VA_ARGS__);	\

I'm surprised any of this compiles with both a macro and arg named
"test". :) Can you change the arg to something with more clarity?
"context" or "kunit" seems better.

>  	cond;								\
>  })
> 
> @@ -44,7 +45,7 @@ static bool is_zeroed(void *from, size_t size)
>  	return memchr_inv(from, 0x0, size) == NULL;
>  }
> 
> -static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
> +static int test_check_nonzero_user(struct kunit *test, char *kmem, char __user *umem, size_t size)
>  {
>  	int ret = 0;
>  	size_t start, end, i, zero_start, zero_end;
> @@ -102,7 +103,7 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
>  	return ret;
>  }
> 
> -static int test_copy_struct_from_user(char *kmem, char __user *umem,
> +static int test_copy_struct_from_user(struct kunit *test, char *kmem, char __user *umem,
>  				      size_t size)
>  {
>  	int ret = 0;
> @@ -177,7 +178,7 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
>  	return ret;
>  }
> 
> -static int __init test_user_copy_init(void)
> +static void user_copy_test(struct kunit *test)
>  {
>  	int ret = 0;
>  	char *kmem;
> @@ -192,16 +193,14 @@ static int __init test_user_copy_init(void)
>  #endif
> 
>  	kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> -	if (!kmem)
> -		return -ENOMEM;
> +	KUNIT_EXPECT_FALSE_MSG(test, kmem == NULL, "kmalloc failed");

This would need to be an ASSERT, yes?

> 
>  	user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
>  			    PROT_READ | PROT_WRITE | PROT_EXEC,
>  			    MAP_ANONYMOUS | MAP_PRIVATE, 0);
>  	if (user_addr >= (unsigned long)(TASK_SIZE)) {
> -		pr_warn("Failed to allocate user memory\n");
>  		kfree(kmem);
> -		return -ENOMEM;
> +		KUNIT_FAIL(test, "Failed to allocate user memory");
>  	}

Why FAIL instead of ASSERT?

> 
>  	usermem = (char __user *)user_addr;
> @@ -245,9 +244,9 @@ static int __init test_user_copy_init(void)
>  #undef test_legit
> 
>  	/* Test usage of check_nonzero_user(). */
> -	ret |= test_check_nonzero_user(kmem, usermem, 2 * PAGE_SIZE);
> +	ret |= test_check_nonzero_user(test, kmem, usermem, 2 * PAGE_SIZE);
>  	/* Test usage of copy_struct_from_user(). */
> -	ret |= test_copy_struct_from_user(kmem, usermem, 2 * PAGE_SIZE);
> +	ret |= test_copy_struct_from_user(test, kmem, usermem, 2 * PAGE_SIZE);
> 
>  	/*
>  	 * Invalid usage: none of these copies should succeed.
> @@ -309,23 +308,18 @@ static int __init test_user_copy_init(void)
> 
>  	vm_munmap(user_addr, PAGE_SIZE * 2);
>  	kfree(kmem);
> -
> -	if (ret == 0) {
> -		pr_info("tests passed.\n");
> -		return 0;
> -	}
> -
> -	return -EINVAL;

Does KUnit provide a end-of-test summary now?

>  }
> 
> -module_init(test_user_copy_init);
> -
> -static void __exit test_user_copy_exit(void)
> -{
> -	pr_info("unloaded.\n");
> -}
> +static struct kunit_case user_copy_test_cases[] = {
> +	KUNIT_CASE(user_copy_test),
> +	{}
> +};
> 
> -module_exit(test_user_copy_exit);
> +static struct kunit_suite user_copy_test_suite = {
> +	.name = "user_copy",
> +	.test_cases = user_copy_test_cases,
> +};
> 
> +kunit_test_suites(&user_copy_test_suite);
>  MODULE_AUTHOR("Kees Cook <keescook@chromium.org>");
>  MODULE_LICENSE("GPL");
> 
> base-commit: d43c7fb05765152d4d4a39a8ef957c4ea14d8847
> --
> 2.26.2
> 

Otherwise, yes, looking good.

-- 
Kees Cook

Re: [PATCH v3] lib: Convert test_user_copy to KUnit test

[Vitor Massaru Iha]

On Tue, Jul 21, 2020 at 4:09 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jul 21, 2020 at 02:46:54PM -0300, Vitor Massaru Iha wrote:
> > This adds the conversion of the runtime tests of test_user_copy fuctions,
> > from `lib/test_user_copy.c`to KUnit tests.
> >
> > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
> > ---
> > v2:
> >     * splitted patch in 3:
> >         - Allows to install and load modules in root filesystem;
> >         - Provides an userspace memory context when tests are compiled
> >           as module;
> >         - Convert test_user_copy to KUnit test;
> >     * removed entry for CONFIG_TEST_USER_COPY;
> >     * replaced pr_warn to KUNIT_EXPECT_FALSE_MSG in test macro to
> >       decrease the diff;
> > v3:
> >     * rebased with last kunit branch
> >     * Please apply this commit from kunit-fixes:
> >         3f37d14b8a3152441f36b6bc74000996679f0998
> >       And these from patchwork:
> >         https://patchwork.kernel.org/patch/11676331/
> >         https://patchwork.kernel.org/patch/11676335/
> > ---
> >  lib/Kconfig.debug                           | 28 ++++++++------
> >  lib/Makefile                                |  2 +-
> >  lib/{test_user_copy.c => user_copy_kunit.c} | 42 +++++++++------------
> >  3 files changed, 35 insertions(+), 37 deletions(-)
> >  rename lib/{test_user_copy.c => user_copy_kunit.c} (91%)
> >
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 9ad9210d70a1..f699a3624ae7 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2078,18 +2078,6 @@ config TEST_VMALLOC
> >
> >         If unsure, say N.
> >
> > -config TEST_USER_COPY
> > -     tristate "Test user/kernel boundary protections"
> > -     depends on m
> > -     help
> > -       This builds the "test_user_copy" module that runs sanity checks
> > -       on the copy_to/from_user infrastructure, making sure basic
> > -       user/kernel boundary testing is working. If it fails to load,
> > -       a regression has been detected in the user/kernel memory boundary
> > -       protections.
> > -
> > -       If unsure, say N.
> > -
> >  config TEST_BPF
> >       tristate "Test BPF filter functionality"
> >       depends on m && NET
> > @@ -2154,6 +2142,22 @@ config SYSCTL_KUNIT_TEST
> >
> >         If unsure, say N.
> >
> > +config USER_COPY_KUNIT
> > +     tristate "KUnit Test for user/kernel boundary protections"
> > +     depends on KUNIT
> > +     depends on m
> > +     help
> > +       This builds the "user_copy_kunit" module that runs sanity checks
> > +       on the copy_to/from_user infrastructure, making sure basic
> > +       user/kernel boundary testing is working. If it fails to load,
> > +       a regression has been detected in the user/kernel memory boundary
> > +       protections.
> > +
> > +       For more information on KUnit and unit tests in general please refer
> > +       to the KUnit documentation in Documentation/dev-tools/kunit/.
> > +
> > +       If unsure, say N.
> > +
> >  config LIST_KUNIT_TEST
> >       tristate "KUnit Test for Kernel Linked-list structures" if !KUNIT_ALL_TESTS
> >       depends on KUNIT
> > diff --git a/lib/Makefile b/lib/Makefile
> > index b1c42c10073b..8c145f85accc 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
> >  obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
> >  obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
> >  obj-$(CONFIG_TEST_SORT) += test_sort.o
> > -obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> >  obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> >  obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> >  obj-$(CONFIG_TEST_PRINTF) += test_printf.o
> > @@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
> >  # KUnit tests
> >  obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> >  obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> > +obj-$(CONFIG_USER_COPY_KUNIT) += user_copy_kunit.o
> > diff --git a/lib/test_user_copy.c b/lib/user_copy_kunit.c
> > similarity index 91%
> > rename from lib/test_user_copy.c
> > rename to lib/user_copy_kunit.c
> > index 5ff04d8fe971..a10ddd15b4cd 100644
> > --- a/lib/test_user_copy.c
> > +++ b/lib/user_copy_kunit.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/vmalloc.h>
> > +#include <kunit/test.h>
> >
> >  /*
> >   * Several 32-bit architectures support 64-bit {get,put}_user() calls.
> > @@ -35,7 +36,7 @@
> >  ({                                                                   \
> >       int cond = (condition);                                         \
> >       if (cond)                                                       \
> > -             pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__);     \
> > +             KUNIT_EXPECT_FALSE_MSG(test, cond, msg, ##__VA_ARGS__); \
>
> I'm surprised any of this compiles with both a macro and arg named
> "test". :) Can you change the arg to something with more clarity?
> "context" or "kunit" seems better.

It will be out of the standard of the other tests in KUnit, but I agree that
I should not use the same name "test" in the argument and in the name
of the macro.
I'll replace it with "context" instead of "test" in arg.

>
> >       cond;                                                           \
> >  })
> >
> > @@ -44,7 +45,7 @@ static bool is_zeroed(void *from, size_t size)
> >       return memchr_inv(from, 0x0, size) == NULL;
> >  }
> >
> > -static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
> > +static int test_check_nonzero_user(struct kunit *test, char *kmem, char __user *umem, size_t size)
> >  {
> >       int ret = 0;
> >       size_t start, end, i, zero_start, zero_end;
> > @@ -102,7 +103,7 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
> >       return ret;
> >  }
> >
> > -static int test_copy_struct_from_user(char *kmem, char __user *umem,
> > +static int test_copy_struct_from_user(struct kunit *test, char *kmem, char __user *umem,
> >                                     size_t size)
> >  {
> >       int ret = 0;
> > @@ -177,7 +178,7 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
> >       return ret;
> >  }
> >
> > -static int __init test_user_copy_init(void)
> > +static void user_copy_test(struct kunit *test)
> >  {
> >       int ret = 0;
> >       char *kmem;
> > @@ -192,16 +193,14 @@ static int __init test_user_copy_init(void)
> >  #endif
> >
> >       kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> > -     if (!kmem)
> > -             return -ENOMEM;
> > +     KUNIT_EXPECT_FALSE_MSG(test, kmem == NULL, "kmalloc failed");
>
> This would need to be an ASSERT, yes?

Yep, I'll fix it.

>
> >
> >       user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
> >                           PROT_READ | PROT_WRITE | PROT_EXEC,
> >                           MAP_ANONYMOUS | MAP_PRIVATE, 0);
> >       if (user_addr >= (unsigned long)(TASK_SIZE)) {
> > -             pr_warn("Failed to allocate user memory\n");
> >               kfree(kmem);
> > -             return -ENOMEM;
> > +             KUNIT_FAIL(test, "Failed to allocate user memory");
> >       }
>
> Why FAIL instead of ASSERT?

I did it this way so I wouldn't have to test twice if I had a memory
allocation problem,
once in the "if" and once in the ASSERT, so the memory of the other
kmalloc is freed
in case of memory allocation error in this memory allocation.

>
> >
> >       usermem = (char __user *)user_addr;
> > @@ -245,9 +244,9 @@ static int __init test_user_copy_init(void)
> >  #undef test_legit
> >
> >       /* Test usage of check_nonzero_user(). */
> > -     ret |= test_check_nonzero_user(kmem, usermem, 2 * PAGE_SIZE);
> > +     ret |= test_check_nonzero_user(test, kmem, usermem, 2 * PAGE_SIZE);
> >       /* Test usage of copy_struct_from_user(). */
> > -     ret |= test_copy_struct_from_user(kmem, usermem, 2 * PAGE_SIZE);
> > +     ret |= test_copy_struct_from_user(test, kmem, usermem, 2 * PAGE_SIZE);
> >
> >       /*
> >        * Invalid usage: none of these copies should succeed.
> > @@ -309,23 +308,18 @@ static int __init test_user_copy_init(void)
> >
> >       vm_munmap(user_addr, PAGE_SIZE * 2);
> >       kfree(kmem);
> > -
> > -     if (ret == 0) {
> > -             pr_info("tests passed.\n");
> > -             return 0;
> > -     }
> > -
> > -     return -EINVAL;
>
> Does KUnit provide a end-of-test summary now?

When you talk about end-of-test summary, is it what is written in
dmesg and not the kunit-tool?

>
> >  }
> >
> > -module_init(test_user_copy_init);
> > -
> > -static void __exit test_user_copy_exit(void)
> > -{
> > -     pr_info("unloaded.\n");
> > -}
> > +static struct kunit_case user_copy_test_cases[] = {
> > +     KUNIT_CASE(user_copy_test),
> > +     {}
> > +};
> >
> > -module_exit(test_user_copy_exit);
> > +static struct kunit_suite user_copy_test_suite = {
> > +     .name = "user_copy",
> > +     .test_cases = user_copy_test_cases,
> > +};
> >
> > +kunit_test_suites(&user_copy_test_suite);
> >  MODULE_AUTHOR("Kees Cook <keescook@chromium.org>");
> >  MODULE_LICENSE("GPL");
> >
> > base-commit: d43c7fb05765152d4d4a39a8ef957c4ea14d8847
> > --
> > 2.26.2
> >
>
> Otherwise, yes, looking good.
>
> --
> Kees Cook

Thanks for the review!

Re: [PATCH v3] lib: Convert test_user_copy to KUnit test

[Vitor Massaru Iha]

On Tue, Jul 21, 2020 at 7:19 PM Vitor Massaru Iha <vitor@massaru.org> wrote:
>
> On Tue, Jul 21, 2020 at 4:09 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Tue, Jul 21, 2020 at 02:46:54PM -0300, Vitor Massaru Iha wrote:
> > > This adds the conversion of the runtime tests of test_user_copy fuctions,
> > > from `lib/test_user_copy.c`to KUnit tests.
> > >
> > > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
> > > ---
> > > v2:
> > >     * splitted patch in 3:
> > >         - Allows to install and load modules in root filesystem;
> > >         - Provides an userspace memory context when tests are compiled
> > >           as module;
> > >         - Convert test_user_copy to KUnit test;
> > >     * removed entry for CONFIG_TEST_USER_COPY;
> > >     * replaced pr_warn to KUNIT_EXPECT_FALSE_MSG in test macro to
> > >       decrease the diff;
> > > v3:
> > >     * rebased with last kunit branch
> > >     * Please apply this commit from kunit-fixes:
> > >         3f37d14b8a3152441f36b6bc74000996679f0998
> > >       And these from patchwork:
> > >         https://patchwork.kernel.org/patch/11676331/
> > >         https://patchwork.kernel.org/patch/11676335/
> > > ---
> > >  lib/Kconfig.debug                           | 28 ++++++++------
> > >  lib/Makefile                                |  2 +-
> > >  lib/{test_user_copy.c => user_copy_kunit.c} | 42 +++++++++------------
> > >  3 files changed, 35 insertions(+), 37 deletions(-)
> > >  rename lib/{test_user_copy.c => user_copy_kunit.c} (91%)
> > >
> > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > > index 9ad9210d70a1..f699a3624ae7 100644
> > > --- a/lib/Kconfig.debug
> > > +++ b/lib/Kconfig.debug
> > > @@ -2078,18 +2078,6 @@ config TEST_VMALLOC
> > >
> > >         If unsure, say N.
> > >
> > > -config TEST_USER_COPY
> > > -     tristate "Test user/kernel boundary protections"
> > > -     depends on m
> > > -     help
> > > -       This builds the "test_user_copy" module that runs sanity checks
> > > -       on the copy_to/from_user infrastructure, making sure basic
> > > -       user/kernel boundary testing is working. If it fails to load,
> > > -       a regression has been detected in the user/kernel memory boundary
> > > -       protections.
> > > -
> > > -       If unsure, say N.
> > > -
> > >  config TEST_BPF
> > >       tristate "Test BPF filter functionality"
> > >       depends on m && NET
> > > @@ -2154,6 +2142,22 @@ config SYSCTL_KUNIT_TEST
> > >
> > >         If unsure, say N.
> > >
> > > +config USER_COPY_KUNIT
> > > +     tristate "KUnit Test for user/kernel boundary protections"
> > > +     depends on KUNIT
> > > +     depends on m
> > > +     help
> > > +       This builds the "user_copy_kunit" module that runs sanity checks
> > > +       on the copy_to/from_user infrastructure, making sure basic
> > > +       user/kernel boundary testing is working. If it fails to load,
> > > +       a regression has been detected in the user/kernel memory boundary
> > > +       protections.
> > > +
> > > +       For more information on KUnit and unit tests in general please refer
> > > +       to the KUnit documentation in Documentation/dev-tools/kunit/.
> > > +
> > > +       If unsure, say N.
> > > +
> > >  config LIST_KUNIT_TEST
> > >       tristate "KUnit Test for Kernel Linked-list structures" if !KUNIT_ALL_TESTS
> > >       depends on KUNIT
> > > diff --git a/lib/Makefile b/lib/Makefile
> > > index b1c42c10073b..8c145f85accc 100644
> > > --- a/lib/Makefile
> > > +++ b/lib/Makefile
> > > @@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
> > >  obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
> > >  obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
> > >  obj-$(CONFIG_TEST_SORT) += test_sort.o
> > > -obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> > >  obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> > >  obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> > >  obj-$(CONFIG_TEST_PRINTF) += test_printf.o
> > > @@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
> > >  # KUnit tests
> > >  obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> > >  obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> > > +obj-$(CONFIG_USER_COPY_KUNIT) += user_copy_kunit.o
> > > diff --git a/lib/test_user_copy.c b/lib/user_copy_kunit.c
> > > similarity index 91%
> > > rename from lib/test_user_copy.c
> > > rename to lib/user_copy_kunit.c
> > > index 5ff04d8fe971..a10ddd15b4cd 100644
> > > --- a/lib/test_user_copy.c
> > > +++ b/lib/user_copy_kunit.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/uaccess.h>
> > >  #include <linux/vmalloc.h>
> > > +#include <kunit/test.h>
> > >
> > >  /*
> > >   * Several 32-bit architectures support 64-bit {get,put}_user() calls.
> > > @@ -35,7 +36,7 @@
> > >  ({                                                                   \
> > >       int cond = (condition);                                         \
> > >       if (cond)                                                       \
> > > -             pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__);     \
> > > +             KUNIT_EXPECT_FALSE_MSG(test, cond, msg, ##__VA_ARGS__); \
> >
> > I'm surprised any of this compiles with both a macro and arg named
> > "test". :) Can you change the arg to something with more clarity?
> > "context" or "kunit" seems better.
>
> It will be out of the standard of the other tests in KUnit, but I agree that
> I should not use the same name "test" in the argument and in the name
> of the macro.
> I'll replace it with "context" instead of "test" in arg.
>
> >
> > >       cond;                                                           \
> > >  })
> > >
> > > @@ -44,7 +45,7 @@ static bool is_zeroed(void *from, size_t size)
> > >       return memchr_inv(from, 0x0, size) == NULL;
> > >  }
> > >
> > > -static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
> > > +static int test_check_nonzero_user(struct kunit *test, char *kmem, char __user *umem, size_t size)
> > >  {
> > >       int ret = 0;
> > >       size_t start, end, i, zero_start, zero_end;
> > > @@ -102,7 +103,7 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
> > >       return ret;
> > >  }
> > >
> > > -static int test_copy_struct_from_user(char *kmem, char __user *umem,
> > > +static int test_copy_struct_from_user(struct kunit *test, char *kmem, char __user *umem,
> > >                                     size_t size)
> > >  {
> > >       int ret = 0;
> > > @@ -177,7 +178,7 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
> > >       return ret;
> > >  }
> > >
> > > -static int __init test_user_copy_init(void)
> > > +static void user_copy_test(struct kunit *test)
> > >  {
> > >       int ret = 0;
> > >       char *kmem;
> > > @@ -192,16 +193,14 @@ static int __init test_user_copy_init(void)
> > >  #endif
> > >
> > >       kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> > > -     if (!kmem)
> > > -             return -ENOMEM;
> > > +     KUNIT_EXPECT_FALSE_MSG(test, kmem == NULL, "kmalloc failed");
> >
> > This would need to be an ASSERT, yes?
>
> Yep, I'll fix it.
>
> >
> > >
> > >       user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
> > >                           PROT_READ | PROT_WRITE | PROT_EXEC,
> > >                           MAP_ANONYMOUS | MAP_PRIVATE, 0);
> > >       if (user_addr >= (unsigned long)(TASK_SIZE)) {
> > > -             pr_warn("Failed to allocate user memory\n");
> > >               kfree(kmem);
> > > -             return -ENOMEM;
> > > +             KUNIT_FAIL(test, "Failed to allocate user memory");
> > >       }
> >
> > Why FAIL instead of ASSERT?
>
> I did it this way so I wouldn't have to test twice if I had a memory
> allocation problem,
> once in the "if" and once in the ASSERT, so the memory of the other
> kmalloc is freed
> in case of memory allocation error in this memory allocation.

Hm, In this case the test needs to stop, I'll fix it.

>
> >
> > >
> > >       usermem = (char __user *)user_addr;
> > > @@ -245,9 +244,9 @@ static int __init test_user_copy_init(void)
> > >  #undef test_legit
> > >
> > >       /* Test usage of check_nonzero_user(). */
> > > -     ret |= test_check_nonzero_user(kmem, usermem, 2 * PAGE_SIZE);
> > > +     ret |= test_check_nonzero_user(test, kmem, usermem, 2 * PAGE_SIZE);
> > >       /* Test usage of copy_struct_from_user(). */
> > > -     ret |= test_copy_struct_from_user(kmem, usermem, 2 * PAGE_SIZE);
> > > +     ret |= test_copy_struct_from_user(test, kmem, usermem, 2 * PAGE_SIZE);
> > >
> > >       /*
> > >        * Invalid usage: none of these copies should succeed.
> > > @@ -309,23 +308,18 @@ static int __init test_user_copy_init(void)
> > >
> > >       vm_munmap(user_addr, PAGE_SIZE * 2);
> > >       kfree(kmem);
> > > -
> > > -     if (ret == 0) {
> > > -             pr_info("tests passed.\n");
> > > -             return 0;
> > > -     }
> > > -
> > > -     return -EINVAL;
> >
> > Does KUnit provide a end-of-test summary now?
>
> When you talk about end-of-test summary, is it what is written in
> dmesg and not the kunit-tool?
>
> >
> > >  }
> > >
> > > -module_init(test_user_copy_init);
> > > -
> > > -static void __exit test_user_copy_exit(void)
> > > -{
> > > -     pr_info("unloaded.\n");
> > > -}
> > > +static struct kunit_case user_copy_test_cases[] = {
> > > +     KUNIT_CASE(user_copy_test),
> > > +     {}
> > > +};
> > >
> > > -module_exit(test_user_copy_exit);
> > > +static struct kunit_suite user_copy_test_suite = {
> > > +     .name = "user_copy",
> > > +     .test_cases = user_copy_test_cases,
> > > +};
> > >
> > > +kunit_test_suites(&user_copy_test_suite);
> > >  MODULE_AUTHOR("Kees Cook <keescook@chromium.org>");
> > >  MODULE_LICENSE("GPL");
> > >
> > > base-commit: d43c7fb05765152d4d4a39a8ef957c4ea14d8847
> > > --
> > > 2.26.2
> > >
> >
> > Otherwise, yes, looking good.
> >
> > --
> > Kees Cook
>
> Thanks for the review!

Re: [PATCH v3] lib: Convert test_user_copy to KUnit test

[Kees Cook]

On Tue, Jul 21, 2020 at 07:19:12PM -0300, Vitor Massaru Iha wrote:
> When you talk about end-of-test summary, is it what is written in
> dmesg and not the kunit-tool?

Right, if I build this as a module and do "modprobe user_copy_kunit",
what will show up in dmesg?

-- 
Kees Cook

Re: [PATCH v3] lib: Convert test_user_copy to KUnit test

[Vitor Massaru Iha]

On Tue, Jul 21, 2020 at 11:12 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jul 21, 2020 at 07:19:12PM -0300, Vitor Massaru Iha wrote:
> > When you talk about end-of-test summary, is it what is written in
> > dmesg and not the kunit-tool?
>
> Right, if I build this as a module and do "modprobe user_copy_kunit",
> what will show up in dmesg?

No, It doesn't. I'll put the messages again.

Re: [PATCH v3] lib: Convert test_user_copy to KUnit test

[Kees Cook]

On Wed, Jul 22, 2020 at 03:29:27PM -0300, Vitor Massaru Iha wrote:
> On Tue, Jul 21, 2020 at 11:12 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Tue, Jul 21, 2020 at 07:19:12PM -0300, Vitor Massaru Iha wrote:
> > > When you talk about end-of-test summary, is it what is written in
> > > dmesg and not the kunit-tool?
> >
> > Right, if I build this as a module and do "modprobe user_copy_kunit",
> > what will show up in dmesg?
> 
> No, It doesn't. I'll put the messages again.

Would it be possible to add that behavior to the core KUnit output? Then
all module-based tests would include a summary line?

-- 
Kees Cook

Re: [PATCH v3] lib: Convert test_user_copy to KUnit test

[Vitor Massaru Iha]

On Wed, Jul 22, 2020 at 4:45 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Jul 22, 2020 at 03:29:27PM -0300, Vitor Massaru Iha wrote:
> > On Tue, Jul 21, 2020 at 11:12 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Tue, Jul 21, 2020 at 07:19:12PM -0300, Vitor Massaru Iha wrote:
> > > > When you talk about end-of-test summary, is it what is written in
> > > > dmesg and not the kunit-tool?
> > >
> > > Right, if I build this as a module and do "modprobe user_copy_kunit",
> > > what will show up in dmesg?
> >
> > No, It doesn't. I'll put the messages again.
>
> Would it be possible to add that behavior to the core KUnit output? Then
> all module-based tests would include a summary line?

I will check what can be done.

Re: [PATCH v3] lib: Convert test_user_copy to KUnit test

[Vitor Massaru Iha]

On Wed, Jul 22, 2020 at 4:45 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Jul 22, 2020 at 03:29:27PM -0300, Vitor Massaru Iha wrote:
> > On Tue, Jul 21, 2020 at 11:12 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Tue, Jul 21, 2020 at 07:19:12PM -0300, Vitor Massaru Iha wrote:
> > > > When you talk about end-of-test summary, is it what is written in
> > > > dmesg and not the kunit-tool?
> > >
> > > Right, if I build this as a module and do "modprobe user_copy_kunit",
> > > what will show up in dmesg?
> >
> > No, It doesn't. I'll put the messages again.
>
> Would it be possible to add that behavior to the core KUnit output? Then
> all module-based tests would include a summary line?

Nowadays with modprobe this is shown, is it necessary to add anything else?

root@(none):/# modprobe user_copy_kunit
TAP version 14
    # Subtest: user_copy
    1..1
random: fast init done
    ok 1 - user_copy_test
ok 1 - user_copy
root@(none):/#

>
> --
> Kees Cook