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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202406101217.D14DF2F00@keescook>
Date: Mon, 10 Jun 2024 12:27:54 -0700
From: Kees Cook <kees@...nel.org>
To: David Gow <davidgow@...gle.com>
Cc: Mark Rutland <mark.rutland@....com>,
	Vitor Massaru Iha <vitor@...saru.org>,
	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 1/2] kunit: test: Add vm_mmap() allocation resource
 manager

On Sat, Jun 08, 2024 at 04:44:16PM +0800, David Gow wrote:
> On Mon, 20 May 2024 at 03:12, Kees Cook <keescook@...omium.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 <keescook@...omium.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@...gle.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