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

[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.1743889-2-mark.rutland@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

[PATCH 1/2] kunit: test: Add vm_mmap() allocation resource manager

[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.1743889-2-mark.rutland@arm.com/ [1]
Co-developed-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/kunit/test.h |  17 ++++++
 lib/kunit/test.c     | 139 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 155 insertions(+), 1 deletion(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 61637ef32302..8c3835a6f282 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -478,6 +478,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/test.c b/lib/kunit/test.c
index 1d1475578515..09194dbffb63 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -11,13 +11,14 @@
 #include <kunit/test-bug.h>
 #include <kunit/attributes.h>
 #include <linux/kernel.h>
+#include <linux/kthread.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/mutex.h>
 #include <linux/panic.h>
 #include <linux/sched/debug.h>
 #include <linux/sched.h>
-#include <linux/mm.h>
 
 #include "debugfs.h"
 #include "device-impl.h"
@@ -871,6 +872,142 @@ void kunit_kfree(struct kunit *test, const void *ptr)
 }
 EXPORT_SYMBOL_GPL(kunit_kfree);
 
+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;
+};
+
+/*
+ * Arbitrarily chosen user address for the base allocation.
+ */
+#define UBUF_ADDR_BASE	SZ_2M
+
+/* Create and attach a new mm if it doesn't already exist. */
+static int kunit_attach_mm(void)
+{
+	struct vm_area_struct *vma;
+	struct mm_struct *mm;
+
+	if (current->mm)
+		return 0;
+
+	mm = mm_alloc();
+	if (!mm)
+		return -ENOMEM;
+
+	if (mmap_write_lock_killable(mm))
+		goto out_free;
+
+	/* Define the task size. */
+	mm->task_size = TASK_SIZE;
+
+	/* Prepare the base VMA. */
+	vma = vm_area_alloc(mm);
+	if (!vma)
+		goto out_unlock;
+
+	vma_set_anonymous(vma);
+	vma->vm_start = UBUF_ADDR_BASE;
+	vma->vm_end = UBUF_ADDR_BASE + PAGE_SIZE;
+	vm_flags_init(vma, VM_READ | VM_MAYREAD | VM_WRITE | VM_MAYWRITE);
+	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
+
+	if (insert_vm_struct(mm, vma))
+		goto out_free_vma;
+
+	mmap_write_unlock(mm);
+
+	/* 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;
+
+out_free_vma:
+	vm_area_free(vma);
+out_unlock:
+	mmap_write_unlock(mm);
+out_free:
+	mmput(mm);
+	return -ENOMEM;
+}
+
+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);
+
 void kunit_cleanup(struct kunit *test)
 {
 	struct kunit_resource *res;
-- 
2.34.1

Re: [PATCH 1/2] kunit: test: Add vm_mmap() allocation resource manager

[Mark Rutland]

On Sun, May 19, 2024 at 12:12:52PM -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].
> 
> Link: https://lore.kernel.org/lkml/20230321122514.1743889-2-mark.rutland@arm.com/ [1]
> Co-developed-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/kunit/test.h |  17 ++++++
>  lib/kunit/test.c     | 139 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 155 insertions(+), 1 deletion(-)
> 
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 61637ef32302..8c3835a6f282 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -478,6 +478,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/test.c b/lib/kunit/test.c
> index 1d1475578515..09194dbffb63 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -11,13 +11,14 @@
>  #include <kunit/test-bug.h>
>  #include <kunit/attributes.h>
>  #include <linux/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
>  #include <linux/mutex.h>
>  #include <linux/panic.h>
>  #include <linux/sched/debug.h>
>  #include <linux/sched.h>
> -#include <linux/mm.h>
>  
>  #include "debugfs.h"
>  #include "device-impl.h"
> @@ -871,6 +872,142 @@ void kunit_kfree(struct kunit *test, const void *ptr)
>  }
>  EXPORT_SYMBOL_GPL(kunit_kfree);
>  
> +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;
> +};
> +
> +/*
> + * Arbitrarily chosen user address for the base allocation.
> + */
> +#define UBUF_ADDR_BASE	SZ_2M
> +
> +/* Create and attach a new mm if it doesn't already exist. */
> +static int kunit_attach_mm(void)
> +{
> +	struct vm_area_struct *vma;
> +	struct mm_struct *mm;
> +
> +	if (current->mm)
> +		return 0;

My tests deliberately created/destroyed the mm for each test; surely we
don't want to inherit an MM in some arbitrary state? ... or is this just
so the mm can be allocated lazily upon the first mmap() within a test?

> +
> +	mm = mm_alloc();
> +	if (!mm)
> +		return -ENOMEM;
> +
> +	if (mmap_write_lock_killable(mm))
> +		goto out_free;
> +
> +	/* Define the task size. */
> +	mm->task_size = TASK_SIZE;
> +
> +	/* Prepare the base VMA. */
> +	vma = vm_area_alloc(mm);
> +	if (!vma)
> +		goto out_unlock;
> +
> +	vma_set_anonymous(vma);
> +	vma->vm_start = UBUF_ADDR_BASE;
> +	vma->vm_end = UBUF_ADDR_BASE + PAGE_SIZE;
> +	vm_flags_init(vma, VM_READ | VM_MAYREAD | VM_WRITE | VM_MAYWRITE);
> +	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +
> +	if (insert_vm_struct(mm, vma))
> +		goto out_free_vma;
> +
> +	mmap_write_unlock(mm);

Why do we need this VMA given you have kunit_vm_mmap()?

This existed in my uaccess tests because I didn't use vm_mmap(), and I
wanted complete control over the addresses used.

Given you add kunit_vm_mmap(), I don't think we want this VMA -- it
doesn't serve any real purpose to tests, and accesses can erroneously
hit it, which is problematic.

UBUF_ADDR_BASE shouldn't be necessary either with kunit_vm_mmap(),
unless you want to use fixed addresses. That was just arbitrarily chosen
to be above NULL and the usual minimum mmap limit.

Mark.

> +
> +	/* 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;
> +
> +out_free_vma:
> +	vm_area_free(vma);
> +out_unlock:
> +	mmap_write_unlock(mm);
> +out_free:
> +	mmput(mm);
> +	return -ENOMEM;
> +}
> +
> +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);
> +
>  void kunit_cleanup(struct kunit *test)
>  {
>  	struct kunit_resource *res;
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] kunit: test: Add vm_mmap() allocation resource manager

[Kees Cook]

On Mon, May 20, 2024 at 10:29:06AM +0100, Mark Rutland wrote:
> On Sun, May 19, 2024 at 12:12:52PM -0700, Kees Cook wrote:
> > +/* Create and attach a new mm if it doesn't already exist. */
> > +static int kunit_attach_mm(void)
> > +{
> > +	struct vm_area_struct *vma;
> > +	struct mm_struct *mm;
> > +
> > +	if (current->mm)
> > +		return 0;
> 
> My tests deliberately created/destroyed the mm for each test; surely we
> don't want to inherit an MM in some arbitrary state? ... or is this just
> so the mm can be allocated lazily upon the first mmap() within a test?

It's for lazily creation and for supporting running the KUnit test as a
module (where a userspace would exist). The old usercopy test worked
against the existing userspace, so I'd want to continue to support that.

> 
> > +
> > +	mm = mm_alloc();
> > +	if (!mm)
> > +		return -ENOMEM;
> > +
> > +	if (mmap_write_lock_killable(mm))
> > +		goto out_free;
> > +
> > +	/* Define the task size. */
> > +	mm->task_size = TASK_SIZE;
> > +
> > +	/* Prepare the base VMA. */
> > +	vma = vm_area_alloc(mm);
> > +	if (!vma)
> > +		goto out_unlock;
> > +
> > +	vma_set_anonymous(vma);
> > +	vma->vm_start = UBUF_ADDR_BASE;
> > +	vma->vm_end = UBUF_ADDR_BASE + PAGE_SIZE;
> > +	vm_flags_init(vma, VM_READ | VM_MAYREAD | VM_WRITE | VM_MAYWRITE);
> > +	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> > +
> > +	if (insert_vm_struct(mm, vma))
> > +		goto out_free_vma;
> > +
> > +	mmap_write_unlock(mm);
> 
> Why do we need this VMA given you have kunit_vm_mmap()?

When I was originally testing this, it seemed like I couldn't perform a
vm_mmap() without an existing VMA.

> This existed in my uaccess tests because I didn't use vm_mmap(), and I
> wanted complete control over the addresses used.
> 
> Given you add kunit_vm_mmap(), I don't think we want this VMA -- it
> doesn't serve any real purpose to tests, and accesses can erroneously
> hit it, which is problematic.
> 
> UBUF_ADDR_BASE shouldn't be necessary either with kunit_vm_mmap(),
> unless you want to use fixed addresses. That was just arbitrarily chosen
> to be above NULL and the usual minimum mmap limit.

I'll recheck whether this is needed. I think I had to make some other
changes as well, so maybe something here ended up being redundant
without my noticing it the first time.

Thanks for looking this over!

-- 
Kees Cook

Re: [PATCH 1/2] kunit: test: Add vm_mmap() allocation resource manager

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 8539 bytes --]

On Mon, 20 May 2024 at 03:12, Kees Cook <keescook@chromium.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.1743889-2-mark.rutland@arm.com/ [1]
> Co-developed-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Thanks very much for this!

A few high-level thoughts:
- Do we want to move this into a separate file? test.{c,h} is already
getting pretty big, and this would probably fit more comfortably with
some of the resource-management bits, which are in their own files.
Not every test will need mm support.
- It'd be nice for there to be a way to explicitly teardown/reset
this: I agree that this is made more awkward by KUnit cleanup normally
running on a different thread, but I could definitely see why a test
might want to unset/reset this, and it would be more consistent with
other resources.

Otherwise, I have a few small questions below, but nothing essential.
There are a couple of test failures/hangs for the usercopy test (on
i386 and m68k), which may have origins here: I've mentioned them
there.

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

Cheers,
-- David

>  include/kunit/test.h |  17 ++++++
>  lib/kunit/test.c     | 139 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 155 insertions(+), 1 deletion(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 61637ef32302..8c3835a6f282 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -478,6 +478,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/test.c b/lib/kunit/test.c
> index 1d1475578515..09194dbffb63 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -11,13 +11,14 @@
>  #include <kunit/test-bug.h>
>  #include <kunit/attributes.h>
>  #include <linux/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
>  #include <linux/mutex.h>
>  #include <linux/panic.h>
>  #include <linux/sched/debug.h>
>  #include <linux/sched.h>
> -#include <linux/mm.h>
>
>  #include "debugfs.h"
>  #include "device-impl.h"
> @@ -871,6 +872,142 @@ void kunit_kfree(struct kunit *test, const void *ptr)
>  }
>  EXPORT_SYMBOL_GPL(kunit_kfree);
>
> +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;
> +};
> +
> +/*
> + * Arbitrarily chosen user address for the base allocation.
> + */
> +#define UBUF_ADDR_BASE SZ_2M

Are there any circumstances where we'd want a _different_ base address
here? Could it conflict with something / could tests require something
different?

I suspect it's fine to leave it like this until such a case actually shows up.

> +
> +/* Create and attach a new mm if it doesn't already exist. */
> +static int kunit_attach_mm(void)
> +{
> +       struct vm_area_struct *vma;
> +       struct mm_struct *mm;
> +
> +       if (current->mm)
> +               return 0;
> +
> +       mm = mm_alloc();
> +       if (!mm)
> +               return -ENOMEM;
> +
> +       if (mmap_write_lock_killable(mm))
> +               goto out_free;
> +
> +       /* Define the task size. */
> +       mm->task_size = TASK_SIZE;
> +
> +       /* Prepare the base VMA. */
> +       vma = vm_area_alloc(mm);
> +       if (!vma)
> +               goto out_unlock;
> +
> +       vma_set_anonymous(vma);
> +       vma->vm_start = UBUF_ADDR_BASE;
> +       vma->vm_end = UBUF_ADDR_BASE + PAGE_SIZE;
> +       vm_flags_init(vma, VM_READ | VM_MAYREAD | VM_WRITE | VM_MAYWRITE);
> +       vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +
> +       if (insert_vm_struct(mm, vma))
> +               goto out_free_vma;
> +
> +       mmap_write_unlock(mm);
> +
> +       /* 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;
> +
> +out_free_vma:
> +       vm_area_free(vma);
> +out_unlock:
> +       mmap_write_unlock(mm);
> +out_free:
> +       mmput(mm);
> +       return -ENOMEM;
> +}
> +
> +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);

It could be easier to use kunit_add_action() here, rather than
kunit_alloc_resource(), as you wouldn't need the params struct to pass
things through.

The advantage to keeping the separate resource is that we can more
easily look it up later if we, for example, wanted to be able to make
it current on other threads (is that something we'd ever want to do?).


> +       if (vres)
> +               return vres->addr;
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(kunit_vm_mmap);
> +
>  void kunit_cleanup(struct kunit *test)
>  {
>         struct kunit_resource *res;
> --
> 2.34.1
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240519191254.651865-1-keescook%40chromium.org.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]

Re: [PATCH 1/2] kunit: test: Add vm_mmap() allocation resource manager

[Kees Cook]

On Sat, Jun 08, 2024 at 04:44:16PM +0800, David Gow wrote:
> On Mon, 20 May 2024 at 03:12, Kees Cook <keescook@chromium.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.1743889-2-mark.rutland@arm.com/ [1]
> > Co-developed-by: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> 
> Thanks very much for this!
> 
> A few high-level thoughts:
> - Do we want to move this into a separate file? test.{c,h} is already
> getting pretty big, and this would probably fit more comfortably with
> some of the resource-management bits, which are in their own files.
> Not every test will need mm support.

I'm happy to do that -- I was just following where kunit_kmalloc() was
defined. I'll create a new file for it.

> - It'd be nice for there to be a way to explicitly teardown/reset
> this: I agree that this is made more awkward by KUnit cleanup normally
> running on a different thread, but I could definitely see why a test
> might want to unset/reset this, and it would be more consistent with
> other resources.

Yeah, it's weird, but it's naturally managed?

> Otherwise, I have a few small questions below, but nothing essential.
> There are a couple of test failures/hangs for the usercopy test (on
> i386 and m68k), which may have origins here: I've mentioned them
> there.

I'll look into this. I must have some 64/32 oversight...

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

Thanks!

> > +/*
> > + * Arbitrarily chosen user address for the base allocation.
> > + */
> > +#define UBUF_ADDR_BASE SZ_2M
> 
> Are there any circumstances where we'd want a _different_ base address
> here? Could it conflict with something / could tests require something
> different?
> 
> I suspect it's fine to leave it like this until such a case actually shows up.

Yeah, it shouldn't be important, and as Mark has pointed out, it might
not be needed at all. I'll see what I can do.

> > +       vres = kunit_alloc_resource(test,
> > +                                   kunit_vm_mmap_init,
> > +                                   kunit_vm_mmap_free,
> > +                                   GFP_KERNEL,
> > +                                   &params);
> 
> It could be easier to use kunit_add_action() here, rather than
> kunit_alloc_resource(), as you wouldn't need the params struct to pass
> things through.
> 
> The advantage to keeping the separate resource is that we can more
> easily look it up later if we, for example, wanted to be able to make
> it current on other threads (is that something we'd ever want to do?).

I like having it follow the pattern of the other resource allocators,
but if there's not a strong reason to switch, I'll leave it as-is.

-- 
Kees Cook

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

[Kees Cook]

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

Co-developed-by: Vitor Massaru Iha <vitor@massaru.org>
Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
Link: https://lore.kernel.org/r/20200721174654.72132-1-vitor@massaru.org
Signed-off-by: Kees Cook <keescook@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 <keescook@chromium.org>");
 MODULE_LICENSE("GPL");
-- 
2.34.1

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

[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 <vitor@massaru.org>
> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
> Link: https://lore.kernel.org/r/20200721174654.72132-1-vitor@massaru.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Hi Kees,

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

>   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;								\
>   })
It looks like the 'test' macro is not used anymore, so probably it 
should be removed.

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

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

> +	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 <keescook@chromium.org>");
>   MODULE_LICENSE("GPL");

Other than that,

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

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

[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.orlov0322@gmail.com>

Thanks!

-- 
Kees Cook

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

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 22627 bytes --]

On Mon, 20 May 2024 at 03:12, Kees Cook <keescook@chromium.org> wrote:
>
> Convert the runtime tests of hardened usercopy to standard KUnit tests.
>
> Co-developed-by: Vitor Massaru Iha <vitor@massaru.org>
> Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
> Link: https://lore.kernel.org/r/20200721174654.72132-1-vitor@massaru.org
> Signed-off-by: Kees Cook <keescook@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 <davidgow@google.com>

Cheers,
-- David

>  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.
> +

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


>  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 <keescook@chromium.org>");
>  MODULE_LICENSE("GPL");
> --
> 2.34.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]

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

[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 <keescook@chromium.org> wrote:
> >
> > Convert the runtime tests of hardened usercopy to standard KUnit tests.
> >
> > Co-developed-by: Vitor Massaru Iha <vitor@massaru.org>
> > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
> > Link: https://lore.kernel.org/r/20200721174654.72132-1-vitor@massaru.org
> > Signed-off-by: Kees Cook <keescook@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 <davidgow@google.com>

Thanks!

-- 
Kees Cook