[PATCH 0/2] usercopy: Convert test_user_copy to KUnit test

[Kees Cook]

Hi,

This builds on the proposal[1] from Mark and lets me convert the
existing usercopy selftest to KUnit. Besides adding this basic test to
the KUnit collection, it also opens the door for execve testing (which
depends on having a functional current->mm), and should provide the
basic infrastructure for adding Mark's much more complete usercopy tests.

-Kees

[1] https://lore.kernel.org/lkml/20230321122514.174...@arm.com/

Kees Cook (2):
kunit: test: Add vm_mmap() allocation resource manager
usercopy: Convert test_user_copy to KUnit test

MAINTAINERS | 1 +
include/kunit/test.h | 17 ++
lib/Kconfig.debug | 21 +-
lib/Makefile | 2 +-
lib/kunit/test.c | 139 +++++++++++-
lib/{test_user_copy.c => usercopy_kunit.c} | 252 ++++++++++-----------
6 files changed, 288 insertions(+), 144 deletions(-)
rename lib/{test_user_copy.c => usercopy_kunit.c} (52%)

--
2.34.1

[Kees Cook]

Convert the runtime tests of hardened usercopy to standard KUnit tests.

Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
Link: https://lore.kernel.org/r/20200721174654...@massaru.org
Signed-off-by: Kees Cook <kees...@chromium.org>
---
MAINTAINERS | 1 +

lib/Kconfig.debug | 21 +-
lib/Makefile | 2 +-

lib/{test_user_copy.c => usercopy_kunit.c} | 252 ++++++++++-----------

4 files changed, 133 insertions(+), 143 deletions(-)

rename lib/{test_user_copy.c => usercopy_kunit.c} (52%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7c121493f43d..73995b807e5a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11761,6 +11761,7 @@ F: arch/*/configs/hardening.config
F: include/linux/overflow.h
F: include/linux/randomize_kstack.h
F: kernel/configs/hardening.config
+F: lib/usercopy_kunit.c
F: mm/usercopy.c
K: \b(add|choose)_random_kstack_offset\b
K: \b__check_(object_size|heap_object)\b
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c63a5fbf1f1c..fd974480aa45 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2460,18 +2460,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
@@ -2779,6 +2767,15 @@ config SIPHASH_KUNIT_TEST
This is intended to help people writing architecture-specific
optimized versions. If unsure, say N.

+config USERCOPY_KUNIT_TEST
+ tristate "KUnit Test for user/kernel boundary protections"
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ This builds the "usercopy_kunit" module that runs sanity checks
+ on the copy_to/from_user infrastructure, making sure basic
+ user/kernel boundary testing is working.
+
config TEST_UDELAY
tristate "udelay test driver"
help
diff --git a/lib/Makefile b/lib/Makefile
index ffc6b2341b45..6287bd6be5d7 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_LKM) += test_module.o
obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.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_DYNAMIC_DEBUG) += test_dynamic_debug.o
@@ -406,6 +405,7 @@ obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o
obj-$(CONFIG_STRCAT_KUNIT_TEST) += strcat_kunit.o
obj-$(CONFIG_STRSCPY_KUNIT_TEST) += strscpy_kunit.o
obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o
+obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o

obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o

diff --git a/lib/test_user_copy.c b/lib/usercopy_kunit.c
similarity index 52%
rename from lib/test_user_copy.c
rename to lib/usercopy_kunit.c
index 5ff04d8fe971..515df08b3190 100644
--- a/lib/test_user_copy.c
+++ b/lib/usercopy_kunit.c
@@ -15,7 +15,7 @@
#include <linux/sched.h>
#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.
@@ -31,11 +31,17 @@
# define TEST_U64
#endif

+struct usercopy_test_priv {
+ char *kmem;
+ char __user *umem;
+ size_t size;
+};
+
#define test(condition, msg, ...) \
({ \
int cond = (condition); \
if (cond) \
- pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
+ KUNIT_EXPECT_FALSE_MSG(test, cond, msg, ##__VA_ARGS__); \
cond; \
})

@@ -44,13 +50,16 @@ 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)
+/* Test usage of check_nonzero_user(). */
+static void usercopy_test_check_nonzero_user(struct kunit *test)
{
- int ret = 0;
size_t start, end, i, zero_start, zero_end;
+ struct usercopy_test_priv *priv = test->priv;
+ char __user *umem = priv->umem;
+ char *kmem = priv->kmem;
+ size_t size = priv->size;

- if (test(size < 2 * PAGE_SIZE, "buffer too small"))
- return -EINVAL;
+ KUNIT_ASSERT_GE_MSG(test, size, 2 * PAGE_SIZE, "buffer too small");

/*
* We want to cross a page boundary to exercise the code more
@@ -84,8 +93,8 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
for (i = zero_end; i < size; i += 2)
kmem[i] = 0xff;

- ret |= test(copy_to_user(umem, kmem, size),
- "legitimate copy_to_user failed");
+ KUNIT_EXPECT_EQ_MSG(test, copy_to_user(umem, kmem, size), 0,
+ "legitimate copy_to_user failed");

for (start = 0; start <= size; start++) {
for (end = start; end <= size; end++) {
@@ -93,35 +102,32 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
int retval = check_zeroed_user(umem + start, len);
int expected = is_zeroed(kmem + start, len);

- ret |= test(retval != expected,
- "check_nonzero_user(=%d) != memchr_inv(=%d) mismatch (start=%zu, end=%zu)",
- retval, expected, start, end);
+ KUNIT_EXPECT_EQ_MSG(test, retval, expected,
+ "check_nonzero_user(=%d) != memchr_inv(=%d) mismatch (start=%zu, end=%zu)",
+ retval, expected, start, end);
}
}
-
- return ret;
}

-static int test_copy_struct_from_user(char *kmem, char __user *umem,
- size_t size)
+/* Test usage of copy_struct_from_user(). */
+static void usercopy_test_copy_struct_from_user(struct kunit *test)
{
- int ret = 0;
char *umem_src = NULL, *expected = NULL;
+ struct usercopy_test_priv *priv = test->priv;
+ char __user *umem = priv->umem;
+ char *kmem = priv->kmem;
+ size_t size = priv->size;
size_t ksize, usize;

- umem_src = kmalloc(size, GFP_KERNEL);
- ret = test(umem_src == NULL, "kmalloc failed");
- if (ret)
- goto out_free;
+ umem_src = kunit_kmalloc(test, size, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, umem_src);

- expected = kmalloc(size, GFP_KERNEL);
- ret = test(expected == NULL, "kmalloc failed");
- if (ret)
- goto out_free;
+ expected = kunit_kmalloc(test, size, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, expected);

/* Fill umem with a fixed byte pattern. */
memset(umem_src, 0x3e, size);
- ret |= test(copy_to_user(umem, umem_src, size),
+ KUNIT_ASSERT_EQ_MSG(test, copy_to_user(umem, umem_src, size), 0,
"legitimate copy_to_user failed");

/* Check basic case -- (usize == ksize). */
@@ -131,9 +137,9 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
memcpy(expected, umem_src, ksize);

memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+ KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), 0,
"copy_struct_from_user(usize == ksize) failed");
- ret |= test(memcmp(kmem, expected, ksize),
+ KUNIT_EXPECT_EQ_MSG(test, memcmp(kmem, expected, ksize), 0,
"copy_struct_from_user(usize == ksize) gives unexpected copy");

/* Old userspace case -- (usize < ksize). */
@@ -144,9 +150,9 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
memset(expected + usize, 0x0, ksize - usize);

memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+ KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), 0,
"copy_struct_from_user(usize < ksize) failed");
- ret |= test(memcmp(kmem, expected, ksize),
+ KUNIT_EXPECT_EQ_MSG(test, memcmp(kmem, expected, ksize), 0,
"copy_struct_from_user(usize < ksize) gives unexpected copy");

/* New userspace (-E2BIG) case -- (usize > ksize). */
@@ -154,7 +160,7 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
usize = size;

memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize) != -E2BIG,
+ KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), -E2BIG,
"copy_struct_from_user(usize > ksize) didn't give E2BIG");

/* New userspace (success) case -- (usize > ksize). */
@@ -162,78 +168,46 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
usize = size;

memcpy(expected, umem_src, ksize);
- ret |= test(clear_user(umem + ksize, usize - ksize),
+ KUNIT_EXPECT_EQ_MSG(test, clear_user(umem + ksize, usize - ksize), 0,
"legitimate clear_user failed");

memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+ KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), 0,
"copy_struct_from_user(usize > ksize) failed");
- ret |= test(memcmp(kmem, expected, ksize),
+ KUNIT_EXPECT_EQ_MSG(test, memcmp(kmem, expected, ksize), 0,
"copy_struct_from_user(usize > ksize) gives unexpected copy");
-
-out_free:
- kfree(expected);
- kfree(umem_src);
- return ret;
}

-static int __init test_user_copy_init(void)
+/*
+ * Legitimate usage: none of these copies should fail.
+ */
+static void usercopy_test_valid(struct kunit *test)
{
- int ret = 0;
- char *kmem;
- char __user *usermem;
- char *bad_usermem;
- unsigned long user_addr;
- u8 val_u8;
- u16 val_u16;
- u32 val_u32;
-#ifdef TEST_U64
- u64 val_u64;
-#endif
-
- kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
- if (!kmem)
- return -ENOMEM;
-
- 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;
- }
+ struct usercopy_test_priv *priv = test->priv;
+ char __user *usermem = priv->umem;
+ char *kmem = priv->kmem;

- usermem = (char __user *)user_addr;
- bad_usermem = (char *)user_addr;
-
- /*
- * Legitimate usage: none of these copies should fail.
- */
memset(kmem, 0x3a, PAGE_SIZE * 2);
- ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE),
- "legitimate copy_to_user failed");
+ KUNIT_EXPECT_EQ_MSG(test, 0, copy_to_user(usermem, kmem, PAGE_SIZE),
+ "legitimate copy_to_user failed");
memset(kmem, 0x0, PAGE_SIZE);
- ret |= test(copy_from_user(kmem, usermem, PAGE_SIZE),
- "legitimate copy_from_user failed");
- ret |= test(memcmp(kmem, kmem + PAGE_SIZE, PAGE_SIZE),
- "legitimate usercopy failed to copy data");
-
-#define test_legit(size, check) \
- do { \
- val_##size = check; \
- ret |= test(put_user(val_##size, (size __user *)usermem), \
- "legitimate put_user (" #size ") failed"); \
- val_##size = 0; \
- ret |= test(get_user(val_##size, (size __user *)usermem), \
- "legitimate get_user (" #size ") failed"); \
- ret |= test(val_##size != check, \
- "legitimate get_user (" #size ") failed to do copy"); \
- if (val_##size != check) { \
- pr_info("0x%llx != 0x%llx\n", \
- (unsigned long long)val_##size, \
- (unsigned long long)check); \
- } \
+ KUNIT_EXPECT_EQ_MSG(test, 0, copy_from_user(kmem, usermem, PAGE_SIZE),
+ "legitimate copy_from_user failed");
+ KUNIT_EXPECT_EQ_MSG(test, 0, memcmp(kmem, kmem + PAGE_SIZE, PAGE_SIZE),
+ "legitimate usercopy failed to copy data");
+
+#define test_legit(size, check) \
+ do { \
+ size val_##size = (check); \
+ KUNIT_EXPECT_EQ_MSG(test, 0, \
+ put_user(val_##size, (size __user *)usermem), \
+ "legitimate put_user (" #size ") failed"); \
+ val_##size = 0; \
+ KUNIT_EXPECT_EQ_MSG(test, 0, \
+ get_user(val_##size, (size __user *)usermem), \
+ "legitimate get_user (" #size ") failed"); \
+ KUNIT_EXPECT_EQ_MSG(test, val_##size, check, \
+ "legitimate get_user (" #size ") failed to do copy"); \
} while (0)

test_legit(u8, 0x5a);
@@ -243,27 +217,29 @@ static int __init test_user_copy_init(void)
test_legit(u64, 0x5a5b5c5d6a6b6c6d);
#endif
#undef test_legit
+}

- /* Test usage of check_nonzero_user(). */
- ret |= test_check_nonzero_user(kmem, usermem, 2 * PAGE_SIZE);
- /* Test usage of copy_struct_from_user(). */
- ret |= test_copy_struct_from_user(kmem, usermem, 2 * PAGE_SIZE);
-
- /*
- * Invalid usage: none of these copies should succeed.
- */
+/*
+ * Invalid usage: none of these copies should succeed.
+ */
+static void usercopy_test_invalid(struct kunit *test)
+{
+ struct usercopy_test_priv *priv = test->priv;
+ char __user *usermem = priv->umem;
+ char *bad_usermem = (char *)usermem;
+ char *kmem = priv->kmem;

/* Prepare kernel memory with check values. */
memset(kmem, 0x5a, PAGE_SIZE);
memset(kmem + PAGE_SIZE, 0, PAGE_SIZE);

/* Reject kernel-to-kernel copies through copy_from_user(). */
- ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+ PAGE_SIZE), 0,
"illegal all-kernel copy_from_user passed");

/* Destination half of buffer should have been zeroed. */
- ret |= test(memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE),
+ KUNIT_EXPECT_EQ_MSG(test, memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE), 0,
"zeroing failure for illegal all-kernel copy_from_user");

#if 0
@@ -273,29 +249,25 @@ static int __init test_user_copy_init(void)
* to be tested in LKDTM instead, since this test module does not
* expect to explode.
*/
- ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_from_user(bad_usermem, (char __user *)kmem,
+ PAGE_SIZE), 0,
"illegal reversed copy_from_user passed");
#endif
- ret |= test(!copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
+ PAGE_SIZE), 0,
"illegal all-kernel copy_to_user passed");
- ret |= test(!copy_to_user((char __user *)kmem, bad_usermem,
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, bad_usermem,
+ PAGE_SIZE), 0,
"illegal reversed copy_to_user passed");

#define test_illegal(size, check) \
do { \
- val_##size = (check); \
- ret |= test(!get_user(val_##size, (size __user *)kmem), \
+ size val_##size = (check); \
+ KUNIT_EXPECT_NE_MSG(test, get_user(val_##size, (size __user *)kmem), 0, \
"illegal get_user (" #size ") passed"); \
- ret |= test(val_##size != (size)0, \
+ KUNIT_EXPECT_EQ_MSG(test, val_##size, 0, \
"zeroing failure for illegal get_user (" #size ")"); \
- if (val_##size != (size)0) { \
- pr_info("0x%llx != 0\n", \
- (unsigned long long)val_##size); \
- } \
- ret |= test(!put_user(val_##size, (size __user *)kmem), \
+ KUNIT_EXPECT_NE_MSG(test, put_user(val_##size, (size __user *)kmem), 0, \
"illegal put_user (" #size ") passed"); \
} while (0)

@@ -306,26 +278,46 @@ static int __init test_user_copy_init(void)
test_illegal(u64, 0x5a5b5c5d6a6b6c6d);
#endif
#undef test_illegal
+}

- vm_munmap(user_addr, PAGE_SIZE * 2);
- kfree(kmem);
+static int usercopy_test_init(struct kunit *test)
+{
+ struct usercopy_test_priv *priv;
+ unsigned long user_addr;

- if (ret == 0) {
- pr_info("tests passed.\n");
- return 0;
- }
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+ test->priv = priv;
+ priv->size = PAGE_SIZE * 2;

- return -EINVAL;
-}
+ priv->kmem = kunit_kmalloc(test, priv->size, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->kmem);

-module_init(test_user_copy_init);
+ user_addr = kunit_vm_mmap(test, NULL, 0, priv->size,
+ PROT_READ | PROT_WRITE | PROT_EXEC,
+ MAP_ANONYMOUS | MAP_PRIVATE, 0);
+ KUNIT_ASSERT_LT_MSG(test, user_addr, (unsigned long)TASK_SIZE,
+ "Failed to allocate user memory");
+ priv->umem = (char __user *)user_addr;

-static void __exit test_user_copy_exit(void)
-{
- pr_info("unloaded.\n");
+ return 0;
}

-module_exit(test_user_copy_exit);
-
+static struct kunit_case usercopy_test_cases[] = {
+ KUNIT_CASE(usercopy_test_valid),
+ KUNIT_CASE(usercopy_test_invalid),
+ KUNIT_CASE(usercopy_test_check_nonzero_user),
+ KUNIT_CASE(usercopy_test_copy_struct_from_user),
+ {}
+};
+
+static struct kunit_suite usercopy_test_suite = {
+ .name = "usercopy",
+ .init = usercopy_test_init,
+ .test_cases = usercopy_test_cases,
+};
+
+kunit_test_suites(&usercopy_test_suite);
MODULE_AUTHOR("Kees Cook <kees...@chromium.org>");
MODULE_LICENSE("GPL");
--
2.34.1

[Ivan Orlov]

On 5/19/24 20:12, Kees Cook wrote:
> Convert the runtime tests of hardened usercopy to standard KUnit tests.
>
> Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
> Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
> Link: https://lore.kernel.org/r/20200721174654...@massaru.org
> Signed-off-by: Kees Cook <kees...@chromium.org>
> ---

Hi Kees,

It looks great, just a couple of minor comments below.

It looks like the 'test' macro is not used anymore, so probably it
should be removed.

Should the check be done with KUNIT_ASSERT_NOT_ERR_OR_NULL here as well,
as it is done with priv->kmem?

Other than that,

Tested-by: Ivan Orlov <ivan.or...@gmail.com>
--
Kind regards,
Ivan Orlov

[David Gow]

On Mon, 20 May 2024 at 03:12, Kees Cook <kees...@chromium.org> wrote:
>
> Convert the runtime tests of hardened usercopy to standard KUnit tests.
>
> Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
> Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
> Link: https://lore.kernel.org/r/20200721174654...@massaru.org
> Signed-off-by: Kees Cook <kees...@chromium.org>
> ---

This fails here on i386:
> # usercopy_test_invalid: EXPECTATION FAILED at lib/usercopy_kunit.c:278
> Expected val_u64 == 0, but
> val_u64 == -60129542144 (0xfffffff200000000)

It also seems to be hanging somewhere in usercopy_test_invalid on my
m68k/qemu setup:
./tools/testing/kunit/kunit.py run --build_dir=.kunit-m68k --arch m68k usercopy

Otherwise, it looks fine. Maybe it'd make sense to split some of the
tests up a bit more, but it's a matter of taste (and only really an
advantage for debugging hangs where more detailed progress is nice).

With those architecture-specific hangs either fixed, or documented (if
they're actual problems, not issues with the test), this is:

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

FYI: Checkpatch is whinging that this help text isn't detailed enough.
(I'm not sure what else you could add, though...)

[Kees Cook]

On Wed, May 29, 2024 at 01:17:35PM +0100, Ivan Orlov wrote:
> On 5/19/24 20:12, Kees Cook wrote:
> > #define test(condition, msg, ...) \
> > ({ \
> > int cond = (condition); \
> > if (cond) \
> > - pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
> > + KUNIT_EXPECT_FALSE_MSG(test, cond, msg, ##__VA_ARGS__); \
> > cond; \
> > })
> It looks like the 'test' macro is not used anymore, so probably it should be
> removed.

Oops, yes. Thanks!

> > +static int usercopy_test_init(struct kunit *test)
> > +{
> > + struct usercopy_test_priv *priv;
> > + unsigned long user_addr;
> > - if (ret == 0) {
> > - pr_info("tests passed.\n");
> > - return 0;
> > - }
> > + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
>
> Should the check be done with KUNIT_ASSERT_NOT_ERR_OR_NULL here as well, as
> it is done with priv->kmem?

Yes, that's much more idiomatic. I'll adjust this too.

> Other than that,
>
> Tested-by: Ivan Orlov <ivan.or...@gmail.com>

Thanks!

--
Kees Cook

[Kees Cook]

On Sat, Jun 08, 2024 at 04:44:10PM +0800, David Gow wrote:
> On Mon, 20 May 2024 at 03:12, Kees Cook <kees...@chromium.org> wrote:
> >
> > Convert the runtime tests of hardened usercopy to standard KUnit tests.
> >
> > Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
> > Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
> > Link: https://lore.kernel.org/r/20200721174654...@massaru.org
> > Signed-off-by: Kees Cook <kees...@chromium.org>
> > ---
>
> This fails here on i386:
> > # usercopy_test_invalid: EXPECTATION FAILED at lib/usercopy_kunit.c:278
> > Expected val_u64 == 0, but
> > val_u64 == -60129542144 (0xfffffff200000000)

Hunh. I can reproduce this with "--arch=i386" but not under UML with
SUBARCH=i386. But perhaps it's a difference in the get_user()
implementations between the two.

And this looks like a bug in the get_user() failure path on i386. I will
investigate...

> It also seems to be hanging somewhere in usercopy_test_invalid on my
> m68k/qemu setup:
> ./tools/testing/kunit/kunit.py run --build_dir=.kunit-m68k --arch m68k usercopy

Oh, that's weird. I'll need to get an m68k environment set up...

> Otherwise, it looks fine. Maybe it'd make sense to split some of the
> tests up a bit more, but it's a matter of taste (and only really an
> advantage for debugging hangs where more detailed progress is nice).

Yeah. I can do this in follow-up patches, perhaps.

> With those architecture-specific hangs either fixed, or documented (if
> they're actual problems, not issues with the test), this is:
>
> Reviewed-by: David Gow <davi...@google.com>

Thanks!

--
Kees Cook

[Kees Cook]

For tests that need to allocate using vm_mmap() (e.g. usercopy and
execve), provide the interface to have the allocation tracked by KUnit
itself. This requires bringing up a placeholder userspace mm.

This combines my earlier attempt at this with Mark Rutland's version[1].

Link: https://lore.kernel.org/lkml/20230321122514.174...@arm.com/ [1]
Co-developed-by: Mark Rutland <mark.r...@arm.com>
Signed-off-by: Mark Rutland <mark.r...@arm.com>
Signed-off-by: Kees Cook <ke...@kernel.org>
---
include/kunit/test.h | 17 +++++++
lib/kunit/Makefile | 1 +
lib/kunit/user_alloc.c | 111 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 129 insertions(+)
create mode 100644 lib/kunit/user_alloc.c

diff --git a/include/kunit/test.h b/include/kunit/test.h
index e32b4cb7afa2..ec61cad6b71d 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -480,6 +480,23 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp
return kunit_kmalloc_array(test, n, size, gfp | __GFP_ZERO);
}

+/**
+ * kunit_vm_mmap() - Allocate KUnit-tracked vm_mmap() area
+ * @test: The test context object.
+ * @file: struct file pointer to map from, if any
+ * @addr: desired address, if any
+ * @len: how many bytes to allocate
+ * @prot: mmap PROT_* bits
+ * @flag: mmap flags
+ * @offset: offset into @file to start mapping from.
+ *
+ * See vm_mmap() for more information.
+ */
+unsigned long kunit_vm_mmap(struct kunit *test, struct file *file,
+ unsigned long addr, unsigned long len,
+ unsigned long prot, unsigned long flag,
+ unsigned long offset);
+
void kunit_cleanup(struct kunit *test);

void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ...);
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index 309659a32a78..56dd67dc6e57 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_KUNIT) += kunit.o

kunit-objs += test.o \
resource.o \
+ user_alloc.o \
static_stub.o \
string-stream.o \
assert.o \
diff --git a/lib/kunit/user_alloc.c b/lib/kunit/user_alloc.c
new file mode 100644
index 000000000000..d66f42282f43
--- /dev/null
+++ b/lib/kunit/user_alloc.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit userspace memory allocation resource management.
+ */
+#include <kunit/resource.h>
+#include <kunit/test.h>
+#include <linux/kthread.h>
+#include <linux/mm.h>
+
+struct kunit_vm_mmap_resource {
+ unsigned long addr;

+ size_t size;
+};
+

+/* vm_mmap() arguments */
+struct kunit_vm_mmap_params {
+ struct file *file;
+ unsigned long addr;
+ unsigned long len;
+ unsigned long prot;
+ unsigned long flag;
+ unsigned long offset;
+};
+
+/* Create and attach a new mm if it doesn't already exist. */
+static int kunit_attach_mm(void)
+{
+ struct mm_struct *mm;
+
+ if (current->mm)
+ return 0;
+
+ mm = mm_alloc();
+ if (!mm)
+ return -ENOMEM;
+
+ /* Define the task size. */
+ mm->task_size = TASK_SIZE;
+
+ /* Make sure we can allocate new VMAs. */
+ arch_pick_mmap_layout(mm, &current->signal->rlim[RLIMIT_STACK]);
+
+ /* Attach the mm. It will be cleaned up when the process dies. */
+ kthread_use_mm(mm);
+
+ return 0;
+}
+
+static int kunit_vm_mmap_init(struct kunit_resource *res, void *context)
+{
+ struct kunit_vm_mmap_params *p = context;
+ struct kunit_vm_mmap_resource vres;
+ int ret;
+
+ ret = kunit_attach_mm();
+ if (ret)
+ return ret;
+
+ vres.size = p->len;
+ vres.addr = vm_mmap(p->file, p->addr, p->len, p->prot, p->flag, p->offset);
+ if (!vres.addr)
+ return -ENOMEM;
+ res->data = kmemdup(&vres, sizeof(vres), GFP_KERNEL);
+ if (!res->data) {
+ vm_munmap(vres.addr, vres.size);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void kunit_vm_mmap_free(struct kunit_resource *res)
+{
+ struct kunit_vm_mmap_resource *vres = res->data;
+
+ /*
+ * Since this is executed from the test monitoring process,
+ * the test's mm has already been torn down. We don't need
+ * to run vm_munmap(vres->addr, vres->size), only clean up
+ * the vres.
+ */
+
+ kfree(vres);
+ res->data = NULL;
+}
+
+unsigned long kunit_vm_mmap(struct kunit *test, struct file *file,
+ unsigned long addr, unsigned long len,
+ unsigned long prot, unsigned long flag,
+ unsigned long offset)
+{
+ struct kunit_vm_mmap_params params = {
+ .file = file,
+ .addr = addr,
+ .len = len,
+ .prot = prot,
+ .flag = flag,
+ .offset = offset,
+ };
+ struct kunit_vm_mmap_resource *vres;
+
+ vres = kunit_alloc_resource(test,
+ kunit_vm_mmap_init,
+ kunit_vm_mmap_free,
+ GFP_KERNEL,
+ &params);
+ if (vres)
+ return vres->addr;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(kunit_vm_mmap);
--
2.34.1

[Kees Cook]

Convert the runtime tests of hardened usercopy to standard KUnit tests.

Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
Link: https://lore.kernel.org/r/20200721174654...@massaru.org

Tested-by: Ivan Orlov <ivan.or...@gmail.com>

Signed-off-by: Kees Cook <ke...@kernel.org>
---

MAINTAINERS | 1 +
lib/Kconfig.debug | 21 +-
lib/Makefile | 2 +-

lib/{test_user_copy.c => usercopy_kunit.c} | 273 ++++++++++-----------
4 files changed, 142 insertions(+), 155 deletions(-)
rename lib/{test_user_copy.c => usercopy_kunit.c} (47%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8754ac2c259d..0cd171ec6010 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11962,6 +11962,7 @@ F: arch/*/configs/hardening.config

F: include/linux/overflow.h
F: include/linux/randomize_kstack.h
F: kernel/configs/hardening.config
+F: lib/usercopy_kunit.c
F: mm/usercopy.c
K: \b(add|choose)_random_kstack_offset\b
K: \b__check_(object_size|heap_object)\b
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug

index 59b6765d86b8..561e346f5cb0 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2505,18 +2505,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

@@ -2814,6 +2802,15 @@ config SIPHASH_KUNIT_TEST

This is intended to help people writing architecture-specific
optimized versions. If unsure, say N.

+config USERCOPY_KUNIT_TEST
+ tristate "KUnit Test for user/kernel boundary protections"
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ This builds the "usercopy_kunit" module that runs sanity checks
+ on the copy_to/from_user infrastructure, making sure basic
+ user/kernel boundary testing is working.
+

config TEST_UDELAY
tristate "udelay test driver"
help
diff --git a/lib/Makefile b/lib/Makefile

index 3b1769045651..fae5cc67b95a 100644

--- a/lib/Makefile
+++ b/lib/Makefile
@@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_LKM) += test_module.o
obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.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_DYNAMIC_DEBUG) += test_dynamic_debug.o

@@ -388,6 +387,7 @@ CFLAGS_fortify_kunit.o += $(call cc-disable-warning, stringop-truncation)
CFLAGS_fortify_kunit.o += $(DISABLE_STRUCTLEAK_PLUGIN)
obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o

obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o
+obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o

obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o

diff --git a/lib/test_user_copy.c b/lib/usercopy_kunit.c

similarity index 47%

rename from lib/test_user_copy.c
rename to lib/usercopy_kunit.c

index 5ff04d8fe971..f1689f2c5c7b 100644

--- a/lib/test_user_copy.c
+++ b/lib/usercopy_kunit.c
@@ -15,7 +15,7 @@
#include <linux/sched.h>
#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.

@@ -31,26 +31,27 @@
# define TEST_U64
#endif

-#define test(condition, msg, ...) \
-({ \
- int cond = (condition); \
- if (cond) \

- pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \

- cond; \
-})

+struct usercopy_test_priv {
+ char *kmem;
+ char __user *umem;
+ size_t size;
+};

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)
+/* Test usage of check_nonzero_user(). */
+static void usercopy_test_check_nonzero_user(struct kunit *test)
{
- int ret = 0;
size_t start, end, i, zero_start, zero_end;
+ struct usercopy_test_priv *priv = test->priv;
+ char __user *umem = priv->umem;
+ char *kmem = priv->kmem;
+ size_t size = priv->size;

- if (test(size < 2 * PAGE_SIZE, "buffer too small"))
- return -EINVAL;
+ KUNIT_ASSERT_GE_MSG(test, size, 2 * PAGE_SIZE, "buffer too small");

/*
* We want to cross a page boundary to exercise the code more

@@ -84,8 +85,8 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)

for (i = zero_end; i < size; i += 2)
kmem[i] = 0xff;

- ret |= test(copy_to_user(umem, kmem, size),
- "legitimate copy_to_user failed");
+ KUNIT_EXPECT_EQ_MSG(test, copy_to_user(umem, kmem, size), 0,
+ "legitimate copy_to_user failed");

for (start = 0; start <= size; start++) {
for (end = start; end <= size; end++) {

@@ -93,35 +94,32 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)

@@ -131,9 +129,9 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,

memcpy(expected, umem_src, ksize);

memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+ KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), 0,
"copy_struct_from_user(usize == ksize) failed");
- ret |= test(memcmp(kmem, expected, ksize),
+ KUNIT_EXPECT_EQ_MSG(test, memcmp(kmem, expected, ksize), 0,
"copy_struct_from_user(usize == ksize) gives unexpected copy");

/* Old userspace case -- (usize < ksize). */

@@ -144,9 +142,9 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,

memset(expected + usize, 0x0, ksize - usize);

memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+ KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), 0,
"copy_struct_from_user(usize < ksize) failed");
- ret |= test(memcmp(kmem, expected, ksize),
+ KUNIT_EXPECT_EQ_MSG(test, memcmp(kmem, expected, ksize), 0,
"copy_struct_from_user(usize < ksize) gives unexpected copy");

/* New userspace (-E2BIG) case -- (usize > ksize). */

@@ -154,7 +152,7 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,

usize = size;

memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize) != -E2BIG,
+ KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), -E2BIG,
"copy_struct_from_user(usize > ksize) didn't give E2BIG");

/* New userspace (success) case -- (usize > ksize). */

@@ -162,78 +160,46 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,

+ struct usercopy_test_priv *priv = test->priv;
+ char __user *usermem = priv->umem;
+ char *kmem = priv->kmem;

- 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;
- }

-

@@ -243,27 +209,30 @@ static int __init test_user_copy_init(void)

test_legit(u64, 0x5a5b5c5d6a6b6c6d);
#endif
#undef test_legit
+}

- /* Test usage of check_nonzero_user(). */
- ret |= test_check_nonzero_user(kmem, usermem, 2 * PAGE_SIZE);
- /* Test usage of copy_struct_from_user(). */
- ret |= test_copy_struct_from_user(kmem, usermem, 2 * PAGE_SIZE);
-
- /*
- * Invalid usage: none of these copies should succeed.
- */
+/*
+ * Invalid usage: none of these copies should succeed.
+ */

+static void usercopy_test_invalid(struct kunit *test)
+{

+ struct usercopy_test_priv *priv = test->priv;
+ char __user *usermem = priv->umem;
+ char *bad_usermem = (char *)usermem;
+ char *kmem = priv->kmem;

+ u64 *kmem_u64 = (u64 *)kmem;

/* Prepare kernel memory with check values. */
memset(kmem, 0x5a, PAGE_SIZE);
memset(kmem + PAGE_SIZE, 0, PAGE_SIZE);

/* Reject kernel-to-kernel copies through copy_from_user(). */
- ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+ PAGE_SIZE), 0,
"illegal all-kernel copy_from_user passed");

/* Destination half of buffer should have been zeroed. */
- ret |= test(memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE),
+ KUNIT_EXPECT_EQ_MSG(test, memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE), 0,
"zeroing failure for illegal all-kernel copy_from_user");

#if 0

@@ -273,30 +242,31 @@ static int __init test_user_copy_init(void)

* to be tested in LKDTM instead, since this test module does not
* expect to explode.
*/
- ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_from_user(bad_usermem, (char __user *)kmem,
+ PAGE_SIZE), 0,
"illegal reversed copy_from_user passed");
#endif
- ret |= test(!copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
+ PAGE_SIZE), 0,
"illegal all-kernel copy_to_user passed");
- ret |= test(!copy_to_user((char __user *)kmem, bad_usermem,
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, bad_usermem,
+ PAGE_SIZE), 0,
"illegal reversed copy_to_user passed");

-#define test_illegal(size, check) \
- do { \

- val_##size = (check); \
- ret |= test(!get_user(val_##size, (size __user *)kmem), \

- "illegal get_user (" #size ") passed"); \

- ret |= test(val_##size != (size)0, \

- "zeroing failure for illegal get_user (" #size ")"); \

- if (val_##size != (size)0) { \
- pr_info("0x%llx != 0\n", \
- (unsigned long long)val_##size); \
- } \
- ret |= test(!put_user(val_##size, (size __user *)kmem), \

- "illegal put_user (" #size ") passed"); \
+#define test_illegal(size, check) \

+ do { \
+ size val_##size = (check); \

+ /* get_user() */ \

+ KUNIT_EXPECT_NE_MSG(test, get_user(val_##size, (size __user *)kmem), 0, \

+ "illegal get_user (" #size ") passed"); \
+ KUNIT_EXPECT_EQ_MSG(test, val_##size, 0, \
+ "zeroing failure for illegal get_user (" #size ")"); \
+ /* put_user() */ \
+ *kmem_u64 = 0xF09FA4AFF09FA4AF; \

+ KUNIT_EXPECT_NE_MSG(test, put_user(val_##size, (size __user *)kmem), 0, \

+ "illegal put_user (" #size ") passed"); \
+ KUNIT_EXPECT_EQ_MSG(test, *kmem_u64, 0xF09FA4AFF09FA4AF, \
+ "illegal put_user (" #size ") wrote to kernel memory!"); \
} while (0)

test_illegal(u8, 0x5a);
@@ -306,26 +276,45 @@ static int __init test_user_copy_init(void)

test_illegal(u64, 0x5a5b5c5d6a6b6c6d);
#endif
#undef test_illegal
+}

- vm_munmap(user_addr, PAGE_SIZE * 2);
- kfree(kmem);

+static int usercopy_test_init(struct kunit *test)
+{
+ struct usercopy_test_priv *priv;
+ unsigned long user_addr;

- if (ret == 0) {
- pr_info("tests passed.\n");
- return 0;
- }
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);

+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);

+ test->priv = priv;
+ priv->size = PAGE_SIZE * 2;

- return -EINVAL;
-}
+ priv->kmem = kunit_kmalloc(test, priv->size, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->kmem);

-module_init(test_user_copy_init);
+ user_addr = kunit_vm_mmap(test, NULL, 0, priv->size,
+ PROT_READ | PROT_WRITE | PROT_EXEC,
+ MAP_ANONYMOUS | MAP_PRIVATE, 0);
+ KUNIT_ASSERT_LT_MSG(test, user_addr, (unsigned long)TASK_SIZE,
+ "Failed to allocate user memory");
+ priv->umem = (char __user *)user_addr;

-static void __exit test_user_copy_exit(void)
-{
- pr_info("unloaded.\n");
+ return 0;
}

-module_exit(test_user_copy_exit);
-

-MODULE_AUTHOR("Kees Cook <kees...@chromium.org>");

+static struct kunit_case usercopy_test_cases[] = {
+ KUNIT_CASE(usercopy_test_valid),
+ KUNIT_CASE(usercopy_test_invalid),
+ KUNIT_CASE(usercopy_test_check_nonzero_user),
+ KUNIT_CASE(usercopy_test_copy_struct_from_user),
+ {}
+};
+
+static struct kunit_suite usercopy_test_suite = {
+ .name = "usercopy",
+ .init = usercopy_test_init,
+ .test_cases = usercopy_test_cases,
+};
+
+kunit_test_suites(&usercopy_test_suite);

+MODULE_AUTHOR("Kees Cook <ke...@kernel.org>");

[Kees Cook]

Hi,

This builds on the proposal[1] from Mark and lets me convert the
existing usercopy selftest to KUnit. Besides adding this basic test to
the KUnit collection, it also opens the door for execve testing (which
depends on having a functional current->mm), and should provide the
basic infrastructure for adding Mark's much more complete usercopy tests.

v2:
- dropped "initial VMA", turns out it wasn't needed (Mark)
- various cleanups in the test itself (Vitor)
- moved new kunit resource to a separate file (David)
v1: https://lore.kernel.org/all/20240519190422...@kernel.org/

-Kees

[1] https://lore.kernel.org/lkml/20230321122514.174...@arm.com/

Kees Cook (2):
kunit: test: Add vm_mmap() allocation resource manager
usercopy: Convert test_user_copy to KUnit test

MAINTAINERS | 1 +
include/kunit/test.h | 17 ++

lib/Kconfig.debug | 21 +-
lib/Makefile | 2 +-

lib/kunit/Makefile | 1 +
lib/kunit/user_alloc.c | 111 +++++++++

lib/{test_user_copy.c => usercopy_kunit.c} | 273 ++++++++++-----------

7 files changed, 271 insertions(+), 155 deletions(-)
create mode 100644 lib/kunit/user_alloc.c

rename lib/{test_user_copy.c => usercopy_kunit.c} (47%)

--
2.34.1

[David Gow]

On Tue, 11 Jun 2024 at 05:33, Kees Cook <ke...@kernel.org> wrote:
>
> For tests that need to allocate using vm_mmap() (e.g. usercopy and
> execve), provide the interface to have the allocation tracked by KUnit
> itself. This requires bringing up a placeholder userspace mm.
>
> This combines my earlier attempt at this with Mark Rutland's version[1].
>
> Link: https://lore.kernel.org/lkml/20230321122514.174...@arm.com/ [1]
> Co-developed-by: Mark Rutland <mark.r...@arm.com>
> Signed-off-by: Mark Rutland <mark.r...@arm.com>
> Signed-off-by: Kees Cook <ke...@kernel.org>
> ---

Thanks: this looks good to me, at least on the KUnit side of things.

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

[David Gow]

On Tue, 11 Jun 2024 at 05:33, Kees Cook <ke...@kernel.org> wrote:
>

> Convert the runtime tests of hardened usercopy to standard KUnit tests.
>
> Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
> Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
> Link: https://lore.kernel.org/r/20200721174654...@massaru.org
> Tested-by: Ivan Orlov <ivan.or...@gmail.com>
> Signed-off-by: Kees Cook <ke...@kernel.org>
> ---

This looks good, particularly with the x86 fix applied.

It's still hanging on m68k -- I think at the 'illegal reversed
copy_to_user passed' test -- but I'll admit to not having tried to
debug it further.

One other (set of) notes below about using KUNIT_EXPECT_MEMEQ_MSG(),
otherwise (assuming the m68k stuff isn't actually a regression, which
I haven't tested but I imagine is unlikely),

Reviewed-by: David Gow <davi...@google.com>

Thanks,
-- David

Should we use KUNIT_EXPECT_MEMEQ_MSG() here, rather than memcmp()?
That'll print a nice hexdump of the difference.

>
> /* Old userspace case -- (usize < ksize). */
> @@ -144,9 +142,9 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
> memset(expected + usize, 0x0, ksize - usize);
>
> memset(kmem, 0x0, size);
> - ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
> + KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), 0,
> "copy_struct_from_user(usize < ksize) failed");
> - ret |= test(memcmp(kmem, expected, ksize),
> + KUNIT_EXPECT_EQ_MSG(test, memcmp(kmem, expected, ksize), 0,
> "copy_struct_from_user(usize < ksize) gives unexpected copy");

Again, can we use KUNIT_EXPECT_MEMEQ_MSG() here, rather than memcmp()?

> /* New userspace (-E2BIG) case -- (usize > ksize). */
> @@ -154,7 +152,7 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
> usize = size;
>
> memset(kmem, 0x0, size);
> - ret |= test(copy_struct_from_user(kmem, ksize, umem, usize) != -E2BIG,
> + KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), -E2BIG,
> "copy_struct_from_user(usize > ksize) didn't give E2BIG");
>
> /* New userspace (success) case -- (usize > ksize). */
> @@ -162,78 +160,46 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
> usize = size;
>
> memcpy(expected, umem_src, ksize);
> - ret |= test(clear_user(umem + ksize, usize - ksize),
> + KUNIT_EXPECT_EQ_MSG(test, clear_user(umem + ksize, usize - ksize), 0,
> "legitimate clear_user failed");
>
> memset(kmem, 0x0, size);
> - ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
> + KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), 0,
> "copy_struct_from_user(usize > ksize) failed");
> - ret |= test(memcmp(kmem, expected, ksize),
> + KUNIT_EXPECT_EQ_MSG(test, memcmp(kmem, expected, ksize), 0,
> "copy_struct_from_user(usize > ksize) gives unexpected copy");

Another place to use KUNIT_EXPECT_MEMEQ_MSG()?

KUNIT_EXPECT_MEMEQ_MSG()?

KUNIT_EXPECT_MEMEQ_MSG()?

[Kees Cook]

On Wed, Jun 12, 2024 at 05:13:39PM +0800, David Gow wrote:
> On Tue, 11 Jun 2024 at 05:33, Kees Cook <ke...@kernel.org> wrote:
> >
> > Convert the runtime tests of hardened usercopy to standard KUnit tests.
> >
> > Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
> > Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
> > Link: https://lore.kernel.org/r/20200721174654...@massaru.org
> > Tested-by: Ivan Orlov <ivan.or...@gmail.com>
> > Signed-off-by: Kees Cook <ke...@kernel.org>
> > ---
>
> This looks good, particularly with the x86 fix applied.

Thanks!

> It's still hanging on m68k -- I think at the 'illegal reversed
> copy_to_user passed' test -- but I'll admit to not having tried to
> debug it further.

For my own future reference, I have reproduced this with:

$ sudo apt install gcc-m68k-linux-gnu
$ tools/testing/kunit/kunit.py run --arch m68k --make_option CROSS_COMPILE=m68k-linux-gnu- usercopy

I'll figure it out...

> One other (set of) notes below about using KUNIT_EXPECT_MEMEQ_MSG(),
> otherwise (assuming the m68k stuff isn't actually a regression, which
> I haven't tested but I imagine is unlikely),

I should really read all the API docs every few releases. :) I will
switch to KUNIT_EXPECT_MEMEQ_MSG!

--
Kees Cook

[Kees Cook]

On Wed, Jun 12, 2024 at 05:13:39PM +0800, David Gow wrote:

> On Tue, 11 Jun 2024 at 05:33, Kees Cook <ke...@kernel.org> wrote:
> >
> > Convert the runtime tests of hardened usercopy to standard KUnit tests.
> >
> > Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
> > Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
> > Link: https://lore.kernel.org/r/20200721174654...@massaru.org
> > Tested-by: Ivan Orlov <ivan.or...@gmail.com>
> > Signed-off-by: Kees Cook <ke...@kernel.org>
> > ---
>
> This looks good, particularly with the x86 fix applied.
>
> It's still hanging on m68k -- I think at the 'illegal reversed
> copy_to_user passed' test -- but I'll admit to not having tried to
> debug it further.
>
> One other (set of) notes below about using KUNIT_EXPECT_MEMEQ_MSG(),
> otherwise (assuming the m68k stuff isn't actually a regression, which
> I haven't tested but I imagine is unlikely),

Hi Geert,

I'm trying to debug a hang on m68k in the usercopy behavioral testing
routines. It's testing for the pathological case of having inverted
arguments to copy_to_user():

user_addr = kunit_vm_mmap(test, NULL, 0, priv->size,

PROT_READ | PROT_WRITE | PROT_EXEC,
MAP_ANONYMOUS | MAP_PRIVATE, 0);
...
bad_usermem = (char *)user_addr;
...

KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, bad_usermem,

PAGE_SIZE), 0,
"illegal reversed copy_to_user passed");

On other architectures, this immediate fails because the access_ok()
check rejects it. On m68k with CONFIG_ALTERNATE_USER_ADDRESS_SPACE,
access_ok() short-circuits to "true". I've tried reading
arch/m68k/include/asm/uaccess.h but I'm not sure what's happening under
CONFIG_CPU_HAS_ADDRESS_SPACES.

For now I've excluded that test for m68k, but I'm not sure what's
expected to happen here on m68k for this set of bad arguments. Can you
advise?

Thanks!

-Kees

--
Kees Cook

[Geert Uytterhoeven]

Hi Kees,

On Wed, Jun 12, 2024 at 6:51 PM Kees Cook <ke...@kernel.org> wrote:
> On Wed, Jun 12, 2024 at 05:13:39PM +0800, David Gow wrote:
> > On Tue, 11 Jun 2024 at 05:33, Kees Cook <ke...@kernel.org> wrote:
> > > Convert the runtime tests of hardened usercopy to standard KUnit tests.
> > >
> > > Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
> > > Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
> > > Link: https://lore.kernel.org/r/20200721174654...@massaru.org
> > > Tested-by: Ivan Orlov <ivan.or...@gmail.com>
> > > Signed-off-by: Kees Cook <ke...@kernel.org>
> > > ---
> >
> > This looks good, particularly with the x86 fix applied.
> >
> > It's still hanging on m68k -- I think at the 'illegal reversed
> > copy_to_user passed' test -- but I'll admit to not having tried to
> > debug it further.
> >
> > One other (set of) notes below about using KUNIT_EXPECT_MEMEQ_MSG(),
> > otherwise (assuming the m68k stuff isn't actually a regression, which
> > I haven't tested but I imagine is unlikely),
>

> I'm trying to debug a hang on m68k in the usercopy behavioral testing
> routines. It's testing for the pathological case of having inverted
> arguments to copy_to_user():
>
> user_addr = kunit_vm_mmap(test, NULL, 0, priv->size,
> PROT_READ | PROT_WRITE | PROT_EXEC,
> MAP_ANONYMOUS | MAP_PRIVATE, 0);
> ...
> bad_usermem = (char *)user_addr;
> ...
> KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, bad_usermem,
> PAGE_SIZE), 0,
> "illegal reversed copy_to_user passed");
>
> On other architectures, this immediate fails because the access_ok()
> check rejects it. On m68k with CONFIG_ALTERNATE_USER_ADDRESS_SPACE,
> access_ok() short-circuits to "true". I've tried reading
> arch/m68k/include/asm/uaccess.h but I'm not sure what's happening under
> CONFIG_CPU_HAS_ADDRESS_SPACES.

On m68k CPUs that support CPU_HAS_ADDRESS_SPACES (i.e. all traditional
680x0 that can run real Linux), the CPU has separate address spaces
for kernel and user addresses. Accessing userspace addresses is done
using the special "moves" instruction, so we can just use the MMU to
catch invalid accesses.

> For now I've excluded that test for m68k, but I'm not sure what's
> expected to happen here on m68k for this set of bad arguments. Can you
> advise?

Perhaps the kernel address is actually a valid user address, or
vice versa?

Does the test work on systems that use 4G/4G for kernel/userspace
instead of the usual 1G/3G split?

/me runs the old test_user_copy.ko on ARAnyM
Seems to take a while? Or it hangs, too?

Related reading material
https://lore.kernel.org/all/CAMuHMdUzHwm5_TL7TNAOF+uq...@mail.gmail.com/
https://lore.kernel.org/all/CAMuHMdVQ93ihgcxUbjptTaHd...@mail.gmail.com/

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[Kees Cook]

Okay, that's what I suspected. I think I'll need to just not test this
particular case for archs with separate address spaces, since it would
be meaningless.

>
> > For now I've excluded that test for m68k, but I'm not sure what's
> > expected to happen here on m68k for this set of bad arguments. Can you
> > advise?
>
> Perhaps the kernel address is actually a valid user address, or
> vice versa?

Right -- I think that's what's happened.

>
> Does the test work on systems that use 4G/4G for kernel/userspace
> instead of the usual 1G/3G split?
>
> /me runs the old test_user_copy.ko on ARAnyM
> Seems to take a while? Or it hangs, too?

Sounds like the same behavior.

> Related reading material
> https://lore.kernel.org/all/CAMuHMdUzHwm5_TL7TNAOF+uq...@mail.gmail.com/
> https://lore.kernel.org/all/CAMuHMdVQ93ihgcxUbjptTaHd...@mail.gmail.com/

[Kees Cook]

Convert the runtime tests of hardened usercopy to standard KUnit tests.

Additionally disable usercopy_test_invalid() for systems with separate
address spaces (or no MMU) since it's not sensible to test for address
confusion there (e.g. m68k).

Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
Link: https://lore.kernel.org/r/20200721174654...@massaru.org
Tested-by: Ivan Orlov <ivan.or...@gmail.com>

Reviewed-by: David Gow <davi...@google.com>

Signed-off-by: Kees Cook <ke...@kernel.org>
---

MAINTAINERS | 1 +
lib/Kconfig.debug | 21 +-
lib/Makefile | 2 +-

lib/{test_user_copy.c => usercopy_kunit.c} | 282 ++++++++++-----------
4 files changed, 151 insertions(+), 155 deletions(-)
rename lib/{test_user_copy.c => usercopy_kunit.c} (46%)

similarity index 46%

rename from lib/test_user_copy.c
rename to lib/usercopy_kunit.c

index 5ff04d8fe971..45f1e558c464 100644

+ KUNIT_ASSERT_EQ_MSG(test, retval, expected,

+ KUNIT_EXPECT_MEMEQ_MSG(test, kmem, expected, ksize,

"copy_struct_from_user(usize == ksize) gives unexpected copy");

/* Old userspace case -- (usize < ksize). */
@@ -144,9 +142,9 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
memset(expected + usize, 0x0, ksize - usize);

memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+ KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), 0,
"copy_struct_from_user(usize < ksize) failed");
- ret |= test(memcmp(kmem, expected, ksize),

+ KUNIT_EXPECT_MEMEQ_MSG(test, kmem, expected, ksize,

"copy_struct_from_user(usize < ksize) gives unexpected copy");

/* New userspace (-E2BIG) case -- (usize > ksize). */
@@ -154,7 +152,7 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
usize = size;

memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize) != -E2BIG,
+ KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), -E2BIG,
"copy_struct_from_user(usize > ksize) didn't give E2BIG");

/* New userspace (success) case -- (usize > ksize). */
@@ -162,78 +160,46 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem,
usize = size;

memcpy(expected, umem_src, ksize);
- ret |= test(clear_user(umem + ksize, usize - ksize),
+ KUNIT_EXPECT_EQ_MSG(test, clear_user(umem + ksize, usize - ksize), 0,
"legitimate clear_user failed");

memset(kmem, 0x0, size);
- ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+ KUNIT_EXPECT_EQ_MSG(test, copy_struct_from_user(kmem, ksize, umem, usize), 0,
"copy_struct_from_user(usize > ksize) failed");
- ret |= test(memcmp(kmem, expected, ksize),

+ KUNIT_EXPECT_MEMEQ_MSG(test, kmem, expected, ksize,

"copy_struct_from_user(usize > ksize) gives unexpected copy");

-
-out_free:
- kfree(expected);
- kfree(umem_src);
- return ret;
}

-static int __init test_user_copy_init(void)
+/*
+ * Legitimate usage: none of these copies should fail.
+ */
+static void usercopy_test_valid(struct kunit *test)
{
- int ret = 0;
- char *kmem;
- char __user *usermem;
- char *bad_usermem;
- unsigned long user_addr;
- u8 val_u8;
- u16 val_u16;
- u32 val_u32;
-#ifdef TEST_U64
- u64 val_u64;
-#endif

+ struct usercopy_test_priv *priv = test->priv;
+ char __user *usermem = priv->umem;
+ char *kmem = priv->kmem;

- kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
- if (!kmem)
- return -ENOMEM;

-

+ KUNIT_EXPECT_MEMEQ_MSG(test, kmem, kmem + PAGE_SIZE, PAGE_SIZE,

+ "legitimate usercopy failed to copy data");

+
+#define test_legit(size, check) \
+ do { \
+ size val_##size = (check); \
+ KUNIT_EXPECT_EQ_MSG(test, 0, \
+ put_user(val_##size, (size __user *)usermem), \
+ "legitimate put_user (" #size ") failed"); \
+ val_##size = 0; \
+ KUNIT_EXPECT_EQ_MSG(test, 0, \
+ get_user(val_##size, (size __user *)usermem), \
+ "legitimate get_user (" #size ") failed"); \
+ KUNIT_EXPECT_EQ_MSG(test, val_##size, check, \
+ "legitimate get_user (" #size ") failed to do copy"); \
} while (0)

test_legit(u8, 0x5a);

@@ -243,27 +209,36 @@ static int __init test_user_copy_init(void)

test_legit(u64, 0x5a5b5c5d6a6b6c6d);
#endif
#undef test_legit
+}

- /* Test usage of check_nonzero_user(). */
- ret |= test_check_nonzero_user(kmem, usermem, 2 * PAGE_SIZE);
- /* Test usage of copy_struct_from_user(). */
- ret |= test_copy_struct_from_user(kmem, usermem, 2 * PAGE_SIZE);
-
- /*
- * Invalid usage: none of these copies should succeed.
- */
+/*
+ * Invalid usage: none of these copies should succeed.
+ */
+static void usercopy_test_invalid(struct kunit *test)
+{
+ struct usercopy_test_priv *priv = test->priv;
+ char __user *usermem = priv->umem;
+ char *bad_usermem = (char *)usermem;
+ char *kmem = priv->kmem;
+ u64 *kmem_u64 = (u64 *)kmem;

+
+ if (IS_ENABLED(CONFIG_ALTERNATE_USER_ADDRESS_SPACE) ||
+ !IS_ENABLED(CONFIG_MMU)) {
+ kunit_skip(test, "Testing for kernel/userspace address confusion is only sensible on architectures with a shared address space");
+ return;
+ }

/* Prepare kernel memory with check values. */
memset(kmem, 0x5a, PAGE_SIZE);
memset(kmem + PAGE_SIZE, 0, PAGE_SIZE);

/* Reject kernel-to-kernel copies through copy_from_user(). */
- ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+ PAGE_SIZE), 0,
"illegal all-kernel copy_from_user passed");

/* Destination half of buffer should have been zeroed. */
- ret |= test(memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE),

+ KUNIT_EXPECT_MEMEQ_MSG(test, kmem + PAGE_SIZE, kmem, PAGE_SIZE,

"zeroing failure for illegal all-kernel copy_from_user");

#if 0
@@ -273,30 +248,32 @@ static int __init test_user_copy_init(void)

* to be tested in LKDTM instead, since this test module does not
* expect to explode.
*/
- ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_from_user(bad_usermem, (char __user *)kmem,
+ PAGE_SIZE), 0,
"illegal reversed copy_from_user passed");
#endif
- ret |= test(!copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
- PAGE_SIZE),
+ KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
+ PAGE_SIZE), 0,
"illegal all-kernel copy_to_user passed");
- ret |= test(!copy_to_user((char __user *)kmem, bad_usermem,
- PAGE_SIZE),
+

+ KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, bad_usermem,
+ PAGE_SIZE), 0,

"illegal reversed copy_to_user passed");

@@ -306,26 +283,47 @@ static int __init test_user_copy_init(void)

+ KUNIT_ASSERT_NE_MSG(test, user_addr, 0,
+ "Could not create userspace mm");

[Kees Cook]

Hi,

This builds on the proposal[1] from Mark and lets me convert the
existing usercopy selftest to KUnit. Besides adding this basic test to
the KUnit collection, it also opens the door for execve testing (which
depends on having a functional current->mm), and should provide the
basic infrastructure for adding Mark's much more complete usercopy tests.

v3:
- use MEMEQ KUnit helper (David)
- exclude pathological address confusion test for systems with separate
address spaces, noticed by David
- add KUnit-conditional exports for alloc_mm() and arch_pick_mmap_layout()
noticed by 0day
v2: https://lore.kernel.org/lkml/2024061021305...@kernel.org/
v1: https://lore.kernel.org/lkml/20240519190422...@kernel.org/

-Kees

[1] https://lore.kernel.org/lkml/20230321122514.174...@arm.com/

Kees Cook (2):
kunit: test: Add vm_mmap() allocation resource manager
usercopy: Convert test_user_copy to KUnit test

MAINTAINERS | 1 +
include/kunit/test.h | 17 ++

kernel/fork.c | 3 +

lib/Kconfig.debug | 21 +-
lib/Makefile | 2 +-

lib/kunit/Makefile | 1 +
lib/kunit/user_alloc.c | 113 +++++++++

lib/{test_user_copy.c => usercopy_kunit.c} | 282 ++++++++++-----------

mm/util.c | 3 +
9 files changed, 288 insertions(+), 155 deletions(-)
create mode 100644 lib/kunit/user_alloc.c

rename lib/{test_user_copy.c => usercopy_kunit.c} (46%)

--
2.34.1

[Kees Cook]

For tests that need to allocate using vm_mmap() (e.g. usercopy and
execve), provide the interface to have the allocation tracked by KUnit
itself. This requires bringing up a placeholder userspace mm.

This combines my earlier attempt at this with Mark Rutland's version[1].

Normally alloc_mm() and arch_pick_mmap_layout() aren't exported for
modules, so export these only for KUnit testing.

Link: https://lore.kernel.org/lkml/20230321122514.174...@arm.com/ [1]
Co-developed-by: Mark Rutland <mark.r...@arm.com>
Signed-off-by: Mark Rutland <mark.r...@arm.com>

Reviewed-by: David Gow <davi...@google.com>
Signed-off-by: Kees Cook <ke...@kernel.org>
---

include/kunit/test.h | 17 +++++++
kernel/fork.c | 3 ++
lib/kunit/Makefile | 1 +
lib/kunit/user_alloc.c | 113 +++++++++++++++++++++++++++++++++++++++++
mm/util.c | 3 ++
5 files changed, 137 insertions(+)

diff --git a/kernel/fork.c b/kernel/fork.c
index 99076dbe27d8..cea203197136 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -115,6 +115,8 @@
#define CREATE_TRACE_POINTS
#include <trace/events/task.h>

+#include <kunit/visibility.h>
+
/*
* Minimum number of threads to boot the kernel
*/
@@ -1334,6 +1336,7 @@ struct mm_struct *mm_alloc(void)
memset(mm, 0, sizeof(*mm));
return mm_init(mm, current, current_user_ns());
}
+EXPORT_SYMBOL_IF_KUNIT(mm_alloc);

static inline void __mmput(struct mm_struct *mm)
{

diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index 309659a32a78..56dd67dc6e57 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_KUNIT) += kunit.o

kunit-objs += test.o \
resource.o \
+ user_alloc.o \
static_stub.o \
string-stream.o \
assert.o \
diff --git a/lib/kunit/user_alloc.c b/lib/kunit/user_alloc.c
new file mode 100644

index 000000000000..76d3d1345ed7
--- /dev/null
+++ b/lib/kunit/user_alloc.c
@@ -0,0 +1,113 @@

+
+MODULE_IMPORT_NS(EXPORTED_FOR_KUNIT_TESTING);
diff --git a/mm/util.c b/mm/util.c
index c9e519e6811f..df37c47d9374 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -26,6 +26,8 @@

#include <linux/uaccess.h>

+#include <kunit/visibility.h>
+
#include "internal.h"
#include "swap.h"

@@ -482,6 +484,7 @@ void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack)
clear_bit(MMF_TOPDOWN, &mm->flags);
}
#endif
+EXPORT_SYMBOL_IF_KUNIT(arch_pick_mmap_layout);

/**
* __account_locked_vm - account locked pages to an mm's locked_vm
--
2.34.1

[David Gow]

On Thu, 13 Jun 2024 at 03:59, Kees Cook <ke...@kernel.org> wrote:
>
> Hi,
>
> This builds on the proposal[1] from Mark and lets me convert the
> existing usercopy selftest to KUnit. Besides adding this basic test to
> the KUnit collection, it also opens the door for execve testing (which
> depends on having a functional current->mm), and should provide the
> basic infrastructure for adding Mark's much more complete usercopy tests.
>
> v3:
> - use MEMEQ KUnit helper (David)
> - exclude pathological address confusion test for systems with separate
> address spaces, noticed by David
> - add KUnit-conditional exports for alloc_mm() and arch_pick_mmap_layout()
> noticed by 0day
> v2: https://lore.kernel.org/lkml/2024061021305...@kernel.org/
> v1: https://lore.kernel.org/lkml/20240519190422...@kernel.org/
>
> -Kees
>
> [1] https://lore.kernel.org/lkml/20230321122514.174...@arm.com/

Thanks! This looks good to me (and passes everything here). Unless
there's a compelling reason not to, I think we can take this via the
KUnit tree.

Cheers,
-- David

[Kees Cook]

That would be lovely, thank you! :)

--
Kees Cook

[Shuah Khan]

Please carry the following line forward as well to be complete assuming
it is relevant.

If unsure, say N.

thanks,
-- Shuah

[Kees Cook]

I've explicitly removed that because it would be repetitive if it were
included for all KUnit tests.

--
Kees Cook

[Shuah Khan]

Thanks for the explanation.

thanks,
-- Shuah

[Jeff Johnson]

On 6/12/24 12:59, Kees Cook wrote:
> Convert the runtime tests of hardened usercopy to standard KUnit tests.
>
> Additionally disable usercopy_test_invalid() for systems with separate
> address spaces (or no MMU) since it's not sensible to test for address
> confusion there (e.g. m68k).
>
> Co-developed-by: Vitor Massaru Iha <vi...@massaru.org>
> Signed-off-by: Vitor Massaru Iha <vi...@massaru.org>
> Link: https://lore.kernel.org/r/20200721174654...@massaru.org
> Tested-by: Ivan Orlov <ivan.or...@gmail.com>
> Reviewed-by: David Gow <davi...@google.com>
> Signed-off-by: Kees Cook <ke...@kernel.org>
> ---

...

> +kunit_test_suites(&usercopy_test_suite);
> +MODULE_AUTHOR("Kees Cook <ke...@kernel.org>");
> MODULE_LICENSE("GPL");

Can you add the missing MODULE_DESCRIPTION() to remove the W=1 warning?

The fix to the current file is part of:
https://lore.kernel.org/all/20240601-md-lib-tes...@quicinc.com/

[Kees Cook]

Thanks for the reminder! I've split it out and sent a separate patch to
Shuah.

--
Kees Cook

[Guenter Roeck]

Hi,

On Wed, Jun 12, 2024 at 12:59:18PM -0700, Kees Cook wrote:
> For tests that need to allocate using vm_mmap() (e.g. usercopy and
> execve), provide the interface to have the allocation tracked by KUnit
> itself. This requires bringing up a placeholder userspace mm.
>
> This combines my earlier attempt at this with Mark Rutland's version[1].
>
> Normally alloc_mm() and arch_pick_mmap_layout() aren't exported for
> modules, so export these only for KUnit testing.
>
> Link: https://lore.kernel.org/lkml/20230321122514.174...@arm.com/ [1]

FWIW, not sure I understand what the above link has to do with this patch.

> Co-developed-by: Mark Rutland <mark.r...@arm.com>
> Signed-off-by: Mark Rutland <mark.r...@arm.com>
> Reviewed-by: David Gow <davi...@google.com>
> Signed-off-by: Kees Cook <ke...@kernel.org>

This patch results in a build failure for nommu_kc705_defconfig if kunit tests
are also enabled.

ERROR: modpost: vmlinux: local symbol 'arch_pick_mmap_layout' was exported

If CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT=n, CONFIG_MMU=n, and
CONFIG_KUNIT=y, arch_pick_mmap_layout is exported. However, if
CONFIG_MMU=n, it is declared as static inline function.

Guenter

---
bisect log:

# bad: [b992b79ca8bc336fa8e2c80990b5af80ed8f36fd] Add linux-next specific files for 20240620
# good: [6ba59ff4227927d3a8530fc2973b80e94b54d58f] Linux 6.10-rc4
git bisect start 'HEAD' 'v6.10-rc4'
# good: [c02e717c5a89654b244fec58bb5cda32770966b5] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
git bisect good c02e717c5a89654b244fec58bb5cda32770966b5
# good: [29e7d78253b7ebf4b76fcf6d95e227d0b0c57dc0] Merge branch 'msm-next' of https://gitlab.freedesktop.org/drm/msm.git
git bisect good 29e7d78253b7ebf4b76fcf6d95e227d0b0c57dc0
# good: [bf8fd0d956bfcbf4fd6ff063366374c4bf87d806] Merge branch 'non-rcu/next' of git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
git bisect good bf8fd0d956bfcbf4fd6ff063366374c4bf87d806
# good: [1110f16317b1e0742521eaef5613eb1eb17f55ca] Merge branch 'icc-next' of git://git.kernel.org/pub/scm/linux/kernel/git/djakov/icc.git
git bisect good 1110f16317b1e0742521eaef5613eb1eb17f55ca
# good: [63f3716198e5644713748d83e6a6df3b4a6a3b10] Merge branch 'gpio/for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
git bisect good 63f3716198e5644713748d83e6a6df3b4a6a3b10
# bad: [91b48d9adafddb242264ba19c0bae6e23f71b18a] Merge branch 'kunit' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
git bisect bad 91b48d9adafddb242264ba19c0bae6e23f71b18a
# good: [9b401f4a7170125365160c9af267a41ff6b39001] pinctrl: ti: ti-iodelay: fix possible memory leak when pinctrl_enable() fails
git bisect good 9b401f4a7170125365160c9af267a41ff6b39001
# good: [1963f932d368f18185466979cc0bc89414d798e7] Merge branch 'pwm/for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git
git bisect good 1963f932d368f18185466979cc0bc89414d798e7
# good: [f91869accbe23a2bb08712b7262fa61eab716d42] selftests/resctrl: Simplify bandwidth report type handling
git bisect good f91869accbe23a2bb08712b7262fa61eab716d42
# good: [cbf284291604e818424f45dbdf6a8a705b5480ad] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
git bisect good cbf284291604e818424f45dbdf6a8a705b5480ad
# good: [a5217468214c228b89da37291de604cd756914ab] kunit: add missing MODULE_DESCRIPTION() macros to core modules
git bisect good a5217468214c228b89da37291de604cd756914ab
# bad: [51104c19d8570eb23208e08eac0e9ae09ced1c15] kunit: test: Add vm_mmap() allocation resource manager
git bisect bad 51104c19d8570eb23208e08eac0e9ae09ced1c15
# good: [425ae3ab5a1fa744a00680f059cf1accaaaecb28] list: test: add the missing MODULE_DESCRIPTION() macro
git bisect good 425ae3ab5a1fa744a00680f059cf1accaaaecb28
# first bad commit: [51104c19d8570eb23208e08eac0e9ae09ced1c15] kunit: test: Add vm_mmap() allocation resource manager

[Kees Cook]

On Sat, Jun 22, 2024 at 06:47:39AM -0700, Guenter Roeck wrote:
> Hi,
>
> On Wed, Jun 12, 2024 at 12:59:18PM -0700, Kees Cook wrote:
> > For tests that need to allocate using vm_mmap() (e.g. usercopy and
> > execve), provide the interface to have the allocation tracked by KUnit
> > itself. This requires bringing up a placeholder userspace mm.
> >
> > This combines my earlier attempt at this with Mark Rutland's version[1].
> >
> > Normally alloc_mm() and arch_pick_mmap_layout() aren't exported for
> > modules, so export these only for KUnit testing.
> >
> > Link: https://lore.kernel.org/lkml/20230321122514.174...@arm.com/ [1]
>
> FWIW, not sure I understand what the above link has to do with this patch.

Both the above Link and this patch were implementing KUnit usercopy
tests (and the required infrastructure).

>
> > Co-developed-by: Mark Rutland <mark.r...@arm.com>
> > Signed-off-by: Mark Rutland <mark.r...@arm.com>
> > Reviewed-by: David Gow <davi...@google.com>
> > Signed-off-by: Kees Cook <ke...@kernel.org>
>
> This patch results in a build failure for nommu_kc705_defconfig if kunit tests
> are also enabled.
>
> ERROR: modpost: vmlinux: local symbol 'arch_pick_mmap_layout' was exported
>
> If CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT=n, CONFIG_MMU=n, and
> CONFIG_KUNIT=y, arch_pick_mmap_layout is exported. However, if
> CONFIG_MMU=n, it is declared as static inline function.

I replied in the other thread too, but this has had a fix pending:
https://lore.kernel.org/lkml/202406271005.4E767DAE@keescook/

I pinged the patch again today.

-Kees

--
Kees Cook