Re: [PATCH 1/3] ima: Remove inode lock

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Fri, Oct 18, 2024 at 04:36:16PM +0200, Jann Horn wrote:
> On Fri, Oct 18, 2024 at 1:00 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> > On Fri, Oct 18, 2024 at 01:49:06PM +0300, Kirill A. Shutemov wrote:
> > > On Fri, Oct 18, 2024 at 11:24:06AM +0200, Roberto Sassu wrote:
> > > > Probably it is hard, @Kirill would there be any way to safely move
> > > > security_mmap_file() out of the mmap_lock lock?
> > >
> > > What about something like this (untested):
> > >
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index dd4b35a25aeb..03473e77d356 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -1646,6 +1646,26 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
> > >       if (pgoff + (size >> PAGE_SHIFT) < pgoff)
> > >               return ret;
> > >
> > > +     if (mmap_read_lock_killable(mm))
> > > +             return -EINTR;
> > > +
> > > +     vma = vma_lookup(mm, start);
> > > +
> > > +     if (!vma || !(vma->vm_flags & VM_SHARED)) {
> > > +             mmap_read_unlock(mm);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     file = get_file(vma->vm_file);
> > > +
> > > +     mmap_read_unlock(mm);
> > > +
> > > +     ret = security_mmap_file(vma->vm_file, prot, flags);
> >
> > Accessing VMA fields without any kind of lock is... very much not advised.
> >
> > I'm guessing you meant to say:
> >
> >         ret = security_mmap_file(file, prot, flags);
> >
> > Here? :)
> >
> > I see the original code did this, but obviously was under an mmap lock.
> >
> > I guess given you check that the file is the same below this.... should be
> > fine? Assuming nothing can come in and invalidate the security_mmap_file()
> > check in the mean time somehow?
> >
> > Jann any thoughts?
>
> The overall approach seems reasonable to me - it aligns this path with
> the other security_mmap_file() checks, which also don't happen under
> the lock.

Yeah equally I find this pattern fine as we check that the file is the same
after we reacquire the lock (a common pattern in mm), so if there's no
objections on security side I don't really see any issue myself - Roberto
if you want to go ahead and post the patch (separately perhaps?) we can
toss some tags your way :)