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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Mon, 10 Jun 2024 12:05:54 -0700
From: Kees Cook <kees@...nel.org>
To: Mark Rutland <mark.rutland@....com>
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 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