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

[Kees Cook]

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