linux-kernel - Re: [PATCH v2] mm: Split critical region in remap_file

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: <c0e85aaa89283d5e4b742d23299f286a2e3eeaad.camel@huaweicloud.com>
Date: Mon, 21 Oct 2024 09:59:22 +0200
From: Roberto Sassu <roberto.sassu@...weicloud.com>
To: Paul Moore <paul@...l-moore.com>, "Kirill A. Shutemov"
	 <kirill.shutemov@...ux.intel.com>, akpm@...ux-foundation.org
Cc: Liam.Howlett@...cle.com, lorenzo.stoakes@...cle.com, vbabka@...e.cz, 
 jannh@...gle.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
 ebpqwerty472123@...il.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, 
 bpf@...r.kernel.org, linux-fsdevel@...r.kernel.org, stable@...r.kernel.org,
  syzbot+1cd571a672400ef3a930@...kaller.appspotmail.com, Roberto Sassu
 <roberto.sassu@...wei.com>
Subject: Re: [PATCH v2] mm: Split critical region in remap_file_pages() and
 invoke LSMs in between

On Sat, 2024-10-19 at 11:34 -0400, Paul Moore wrote:
> On Fri, Oct 18, 2024 at 12:15 PM Roberto Sassu
> <roberto.sassu@...weicloud.com> wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
> > 
> > Commit ea7e2d5e49c0 ("mm: call the security_mmap_file() LSM hook in
> > remap_file_pages()") fixed a security issue, it added an LSM check when
> > trying to remap file pages, so that LSMs have the opportunity to evaluate
> > such action like for other memory operations such as mmap() and mprotect().
> > 
> > However, that commit called security_mmap_file() inside the mmap_lock lock,
> > while the other calls do it before taking the lock, after commit
> > 8b3ec6814c83 ("take security_mmap_file() outside of ->mmap_sem").
> > 
> > This caused lock inversion issue with IMA which was taking the mmap_lock
> > and i_mutex lock in the opposite way when the remap_file_pages() system
> > call was called.
> > 
> > Solve the issue by splitting the critical region in remap_file_pages() in
> > two regions: the first takes a read lock of mmap_lock, retrieves the VMA
> > and the file descriptor associated, and calculates the 'prot' and 'flags'
> > variables; the second takes a write lock on mmap_lock, checks that the VMA
> > flags and the VMA file descriptor are the same as the ones obtained in the
> > first critical region (otherwise the system call fails), and calls
> > do_mmap().
> > 
> > In between, after releasing the read lock and before taking the write lock,
> > call security_mmap_file(), and solve the lock inversion issue.
> > 
> > Cc: stable@...r.kernel.org # v6.12-rcx
> > Fixes: ea7e2d5e49c0 ("mm: call the security_mmap_file() LSM hook in remap_file_pages()")
> > Reported-by: syzbot+1cd571a672400ef3a930@...kaller.appspotmail.com
> > Closes: https://lore.kernel.org/linux-security-module/66f7b10e.050a0220.46d20.0036.GAE@google.com/
> > Reviewed-by: Roberto Sassu <roberto.sassu@...wei.com>
> > Reviewed-by: Jann Horn <jannh@...gle.com>
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > Tested-by: Roberto Sassu <roberto.sassu@...wei.com>
> > Tested-by: syzbot+1cd571a672400ef3a930@...kaller.appspotmail.com
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> > ---
> >  mm/mmap.c | 69 +++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 52 insertions(+), 17 deletions(-)
> 
> Thanks for working on this Roberto, Kirill, and everyone else who had
> a hand in reviewing and testing.

Welcome!

> Reviewed-by: Paul Moore <paul@...l-moore.com>
> 
> Andrew, I see you're pulling this into the MM/hotfixes-unstable
> branch, do you also plan to send this up to Linus soon/next-week?  If
> so, great, if not let me know and I can send it up via the LSM tree.
> 
> We need to get clarity around Roberto's sign-off, but I think that is
> more of an administrative mistake rather than an intentional omission
> :)

Ops, I just thought that I would not need to add it, since I'm not the
author of the patch. Please add my:

Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>

Roberto