Received-SPF: pass (google.com: domain of linux-kernel+bounces-208772-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 10 Jun 2024 12:05:54 -0700
From: Kees Cook <kees@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
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
Message-ID: <202406101202.3D887825@keescook>
References: <20240519190422.work.715-kees@kernel.org>
 <20240519191254.651865-1-keescook@chromium.org>
 <ZksX4r0a1EGE_VPl@J2N7QTR9R3>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZksX4r0a1EGE_VPl@J2N7QTR9R3>

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