From: Mark Rutland <mark.rutland@arm.com>
To: Kees Cook <keescook@chromium.org>
Cc: Vitor Massaru Iha <vitor@massaru.org>,
Brendan Higgins <brendan.higgins@linux.dev>,
David Gow <davidgow@google.com>, Rae Moar <rmoar@google.com>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
kunit-dev@googlegroups.com, linux-hardening@vger.kernel.org
Subject: Re: [PATCH 1/2] kunit: test: Add vm_mmap() allocation resource manager
Date: Mon, 20 May 2024 10:29:06 +0100 [thread overview]
Message-ID: <ZksX4r0a1EGE_VPl@J2N7QTR9R3> (raw)
In-Reply-To: <20240519191254.651865-1-keescook@chromium.org>
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, ¤t->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,
> + ¶ms);
> + 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
>
next prev parent reply other threads:[~2024-05-20 9:29 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-19 19:12 [PATCH 0/2] usercopy: Convert test_user_copy to KUnit test Kees Cook
2024-05-19 19:12 ` [PATCH 1/2] kunit: test: Add vm_mmap() allocation resource manager Kees Cook
2024-05-20 9:29 ` Mark Rutland [this message]
2024-05-19 19:12 ` [PATCH 2/2] usercopy: Convert test_user_copy to KUnit test Kees Cook
2024-05-29 12:17 ` Ivan Orlov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZksX4r0a1EGE_VPl@J2N7QTR9R3 \
--to=mark.rutland@arm.com \
--cc=brendan.higgins@linux.dev \
--cc=davidgow@google.com \
--cc=gustavoars@kernel.org \
--cc=keescook@chromium.org \
--cc=kunit-dev@googlegroups.com \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=rmoar@google.com \
--cc=vitor@massaru.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).