Message-ID: <ZksX4r0a1EGE_VPl@J2N7QTR9R3>
Date: Mon, 20 May 2024 10:29:06 +0100
From: Mark Rutland <mark.rutland@....com>
To: Kees Cook <keescook@...omium.org>
Cc: Vitor Massaru Iha <vitor@...saru.org>,
	Brendan Higgins <brendan.higgins@...ux.dev>,
	David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
	kunit-dev@...glegroups.com, linux-hardening@...r.kernel.org
Subject: Re: [PATCH 1/2] kunit: test: Add vm_mmap() allocation resource
 manager

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@....com>
> Signed-off-by: Mark Rutland <mark.rutland@....com>
> Signed-off-by: Kees Cook <keescook@...omium.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
> 

Powered by blists - more mailing lists