Message-ID: <CABVgOS=8zL+jj4w32Ckh3cKq1DvidMLPGs+vj2adhbmCEcQ_6Q@mail.gmail.com>
Date: Wed, 12 Jun 2024 17:13:35 +0800
From: David Gow <davidgow@...gle.com>
To: Kees Cook <kees@...nel.org>
Cc: Mark Rutland <mark.rutland@....com>, Vitor Massaru Iha <vitor@...saru.org>, 
	Ivan Orlov <ivan.orlov0322@...il.com>, Brendan Higgins <brendan.higgins@...ux.dev>, 
	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 v2 1/2] kunit: test: Add vm_mmap() allocation resource manager

On Tue, 11 Jun 2024 at 05:33, Kees Cook <kees@...nel.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@....com>
> Signed-off-by: Mark Rutland <mark.rutland@....com>
> Signed-off-by: Kees Cook <kees@...nel.org>
> ---

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

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

Cheers,
-- David


>  include/kunit/test.h   |  17 +++++++
>  lib/kunit/Makefile     |   1 +
>  lib/kunit/user_alloc.c | 111 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+)
>  create mode 100644 lib/kunit/user_alloc.c
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index e32b4cb7afa2..ec61cad6b71d 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -480,6 +480,23 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp
>         return kunit_kmalloc_array(test, n, size, gfp | __GFP_ZERO);
>  }
>
> +/**
> + * kunit_vm_mmap() - Allocate KUnit-tracked vm_mmap() area
> + * @test: The test context object.
> + * @file: struct file pointer to map from, if any
> + * @addr: desired address, if any
> + * @len: how many bytes to allocate
> + * @prot: mmap PROT_* bits
> + * @flag: mmap flags
> + * @offset: offset into @file to start mapping from.
> + *
> + * See vm_mmap() for more information.
> + */
> +unsigned long kunit_vm_mmap(struct kunit *test, struct file *file,
> +                           unsigned long addr, unsigned long len,
> +                           unsigned long prot, unsigned long flag,
> +                           unsigned long offset);
> +
>  void kunit_cleanup(struct kunit *test);
>
>  void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ...);
> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> index 309659a32a78..56dd67dc6e57 100644
> --- a/lib/kunit/Makefile
> +++ b/lib/kunit/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_KUNIT) +=                  kunit.o
>
>  kunit-objs +=                          test.o \
>                                         resource.o \
> +                                       user_alloc.o \
>                                         static_stub.o \
>                                         string-stream.o \
>                                         assert.o \
> diff --git a/lib/kunit/user_alloc.c b/lib/kunit/user_alloc.c
> new file mode 100644
> index 000000000000..d66f42282f43
> --- /dev/null
> +++ b/lib/kunit/user_alloc.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit userspace memory allocation resource management.
> + */
> +#include <kunit/resource.h>
> +#include <kunit/test.h>
> +#include <linux/kthread.h>
> +#include <linux/mm.h>
> +
> +struct kunit_vm_mmap_resource {
> +       unsigned long addr;
> +       size_t size;
> +};
> +
> +/* vm_mmap() arguments */
> +struct kunit_vm_mmap_params {
> +       struct file *file;
> +       unsigned long addr;
> +       unsigned long len;
> +       unsigned long prot;
> +       unsigned long flag;
> +       unsigned long offset;
> +};
> +
> +/* Create and attach a new mm if it doesn't already exist. */
> +static int kunit_attach_mm(void)
> +{
> +       struct mm_struct *mm;
> +
> +       if (current->mm)
> +               return 0;
> +
> +       mm = mm_alloc();
> +       if (!mm)
> +               return -ENOMEM;
> +
> +       /* Define the task size. */
> +       mm->task_size = TASK_SIZE;
> +
> +       /* Make sure we can allocate new VMAs. */
> +       arch_pick_mmap_layout(mm, &current->signal->rlim[RLIMIT_STACK]);
> +
> +       /* Attach the mm. It will be cleaned up when the process dies. */
> +       kthread_use_mm(mm);
> +
> +       return 0;
> +}
> +
> +static int kunit_vm_mmap_init(struct kunit_resource *res, void *context)
> +{
> +       struct kunit_vm_mmap_params *p = context;
> +       struct kunit_vm_mmap_resource vres;
> +       int ret;
> +
> +       ret = kunit_attach_mm();
> +       if (ret)
> +               return ret;
> +
> +       vres.size = p->len;
> +       vres.addr = vm_mmap(p->file, p->addr, p->len, p->prot, p->flag, p->offset);
> +       if (!vres.addr)
> +               return -ENOMEM;
> +       res->data = kmemdup(&vres, sizeof(vres), GFP_KERNEL);
> +       if (!res->data) {
> +               vm_munmap(vres.addr, vres.size);
> +               return -ENOMEM;
> +       }
> +
> +       return 0;
> +}
> +
> +static void kunit_vm_mmap_free(struct kunit_resource *res)
> +{
> +       struct kunit_vm_mmap_resource *vres = res->data;
> +
> +       /*
> +        * Since this is executed from the test monitoring process,
> +        * the test's mm has already been torn down. We don't need
> +        * to run vm_munmap(vres->addr, vres->size), only clean up
> +        * the vres.
> +        */
> +
> +       kfree(vres);
> +       res->data = NULL;
> +}
> +
> +unsigned long kunit_vm_mmap(struct kunit *test, struct file *file,
> +                           unsigned long addr, unsigned long len,
> +                           unsigned long prot, unsigned long flag,
> +                           unsigned long offset)
> +{
> +       struct kunit_vm_mmap_params params = {
> +               .file = file,
> +               .addr = addr,
> +               .len = len,
> +               .prot = prot,
> +               .flag = flag,
> +               .offset = offset,
> +       };
> +       struct kunit_vm_mmap_resource *vres;
> +
> +       vres = kunit_alloc_resource(test,
> +                                   kunit_vm_mmap_init,
> +                                   kunit_vm_mmap_free,
> +                                   GFP_KERNEL,
> +                                   &params);
> +       if (vres)
> +               return vres->addr;
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(kunit_vm_mmap);
> --
> 2.34.1
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4014 bytes)

Powered by blists - more mailing lists