linux-kernel - Re: [PATCH 4/7] binder: remove binder_alloc_set

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

Message-ID: <Yw58Mm5QrPIFtYak@google.com>
Date:   Tue, 30 Aug 2022 21:08:02 +0000
From:   Carlos Llamas <cmllamas@...gle.com>
To:     Liam Howlett <liam.howlett@...cle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <brauner@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        "kernel-team@...roid.com" <kernel-team@...roid.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/7] binder: remove binder_alloc_set_vma()

On Tue, Aug 30, 2022 at 06:57:59PM +0000, Liam Howlett wrote:
> * Carlos Llamas <cmllamas@...gle.com> [220829 16:13]:
> > The mmap_locked asserts here are not needed since this is only called
> > back from the mmap stack in ->mmap() and ->close() which always acquire
> > the lock first. Remove these asserts along with binder_alloc_set_vma()
> > altogether since it's trivial enough to be consumed by callers.
> 
> I agree that it is not called anywhere else today.  I think it's still
> worth while since these asserts do nothing if you don't build with
> lockdep and/or debug_vm enabled.  I was hoping having these here would
> avoid future mistakes a lot like what we have in the mm code's
> find_vma() (mm/mmap.c ~L2297).
> 

Yes, the assert in find_vma() is perfectly fine, we need to check that
callers have taken the lock before looking up a vma. However, the
scenario here is different as these are callbacks for vm_ops->close()
and mmap() so we are guaranteed to have the lock at this point. We don't
really want to duplicate checks for each user of these callbacks such as
the binder driver here.