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
