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
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 checkson the copy_to/from_user infrastructure, making sure basicuser/kernel boundary testing is working. If it fails to load,a regression has been detected in the user/kernel memory boundaryprotections.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 checkson the copy_to/from_user infrastructure, making sure basicuser/kernel boundary testing is working. If it fails to load,a regression has been detected in the user/kernel memory boundaryprotections.For more information on KUnit and unit tests in general please referto 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");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.
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 mhelpThis builds the "test_user_copy" module that runs sanity checkson the copy_to/from_user infrastructure, making sure basicuser/kernel boundary testing is working. If it fails to load,a regression has been detected in the user/kernel memory boundaryprotections.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 KUNITdepends on mhelpThis builds the "user_copy_kunit" module that runs sanity checkson the copy_to/from_user infrastructure, making sure basicuser/kernel boundary testing is working. If it fails to load,a regression has been detected in the user/kernel memory boundaryprotections.For more information on KUnit and unit tests in general please referto 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!
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 mhelpThis builds the "test_user_copy" module that runs sanity checkson the copy_to/from_user infrastructure, making sure basicuser/kernel boundary testing is working. If it fails to load,a regression has been detected in the user/kernel memory boundaryprotections.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 KUNITdepends on mhelpThis builds the "user_copy_kunit" module that runs sanity checkson the copy_to/from_user infrastructure, making sure basicuser/kernel boundary testing is working. If it fails to load,a regression has been detected in the user/kernel memory boundaryprotections.For more information on KUnit and unit tests in general please referto 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!
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?
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.
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?
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.
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
linux-kselftest-mirror@lists.linaro.org
-
Kees Cook -
Vitor Massaru Iha