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

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] [day] [month] [year] [list]

Message-ID: <5e288d07-5992-4ca4-8ec1-214d2fa6a326@lucifer.local>
Date: Fri, 18 Oct 2024 15:48:19 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Jann Horn <jannh@...gle.com>
Cc: "Kirill A. Shutemov" <kirill@...temov.name>,
        Roberto Sassu <roberto.sassu@...weicloud.com>,
        Paul Moore <paul@...l-moore.com>, ebpqwerty472123@...il.com,
        kirill.shutemov@...ux.intel.com, zohar@...ux.ibm.com,
        dmitry.kasatkin@...il.com, eric.snowberg@...cle.com, jmorris@...ei.org,
        serge@...lyn.com, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
        bpf@...r.kernel.org, Roberto Sassu <roberto.sassu@...wei.com>,
        linux-mm@...ck.org, akpm@...ux-foundation.org, vbabka@...e.cz,
        linux-fsdevel@...r.kernel.org, Liam Howlett <liam.howlett@...cle.com>
Subject: Re: [PATCH 1/3] ima: Remove inode lock

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@...cle.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 :)