linux-kernel - Re: [syzbot] [integrity?] [lsm?] possible deadlock in process

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: <CAHC9VhSEMSwzxjXUHLCWXoGj3ds8pQJ-nH6WQuRDzBkx6Svotw@mail.gmail.com>
Date: Mon, 7 Oct 2024 12:35:46 -0400
From: Paul Moore <paul@...l-moore.com>
To: Roberto Sassu <roberto.sassu@...weicloud.com>
Cc: Shu Han <ebpqwerty472123@...il.com>, 
	syzbot <syzbot+1cd571a672400ef3a930@...kaller.appspotmail.com>, 
	akpm@...ux-foundation.org, dmitry.kasatkin@...il.com, 
	eric.snowberg@...cle.com, hughd@...gle.com, jmorris@...ei.org, 
	linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, linux-security-module@...r.kernel.org, 
	roberto.sassu@...wei.com, serge@...lyn.com, stephen.smalley.work@...il.com, 
	syzkaller-bugs@...glegroups.com, zohar@...ux.ibm.com
Subject: Re: [syzbot] [integrity?] [lsm?] possible deadlock in
 process_measurement (4)

On Mon, Oct 7, 2024 at 11:31 AM Roberto Sassu
<roberto.sassu@...weicloud.com> wrote:
> On Wed, 2024-10-02 at 23:09 -0400, Paul Moore wrote:
> > On Sat, Sep 28, 2024 at 2:08 PM Shu Han <ebpqwerty472123@...il.com> wrote:
> > >
> > > > ======================================================
> > > > WARNING: possible circular locking dependency detected
> > > > 6.11.0-syzkaller-10045-g97d8894b6f4c #0 Not tainted
> > > > ------------------------------------------------------
> > > > syz-executor369/5231 is trying to acquire lock:
> > > > ffff888072852370 (&sb->s_type->i_mutex_key#12){+.+.}-{3:3}, at: inode_lock include/linux/fs.h:815 [inline]
> > > > ffff888072852370 (&sb->s_type->i_mutex_key#12){+.+.}-{3:3}, at: process_measurement+0x439/0x1fb0 security/integrity/ima/ima_main.c:250
> > > >
> > > > but task is already holding lock:
> > > > ffff88807ac9a798 (&mm->mmap_lock){++++}-{3:3}, at: mmap_write_lock_killable include/linux/mmap_lock.h:122 [inline]
> > > > ffff88807ac9a798 (&mm->mmap_lock){++++}-{3:3}, at: __do_sys_remap_file_pages mm/mmap.c:1649 [inline]
> > > > ffff88807ac9a798 (&mm->mmap_lock){++++}-{3:3}, at: __se_sys_remap_file_pages+0x22d/0xa50 mm/mmap.c:1624
> > > >
> > > > which lock already depends on the new lock.
> > >
> > > This issue (if not a false positive?) is due to the possible `prot`
> > > change caused by the processing logic for READ_IMPLIES_EXEC in do_mmap(),
> > > so the remap_file_pages() must perform LSM check before calling do_mmap(),
> > > this is what the previous commit want to do.
> >
> > My apologies for the delay on this, I was traveling for a bit and
> > missed this issue while away.
> >
> > Looking quickly at the report, I don't believe this is a false positive.
> >
> > > The LSM check is required to know what the `prot` is, but `prot` must be
> > > obtained after holding the `mmap_write_lock`.
> > >
> > > If the `mmap_write_lock` is released after getting the `prot` and before
> > > the LSM call in remap_file_pages(), it may cause TOCTOU.
> >
> > Looking at the IMA code, specifically the process_measurement()
> > function which is called from the security_mmap_file() LSM hook, I'm
> > not sure why there is the inode_lock() protected region.  Mimi?
> > Roberto?  My best guess is that locking the inode may have been
> > necessary before we moved the IMA inode state into the inode's LSM
> > security blob, but I'm not certain.
> >
> > Mimi and Roberto, can we safely remove the inode locking in
> > process_measurement()?
>
> I discussed a bit with Mimi. Her concern was the duplicate iint
> structure creation during concurrent file accesses. Now that inode
> integrity metadata have been moved to the inode security blob, we can
> take the iint->mutex out of the ima_iint_cache structure, and store it
> directly in the security blob. In this way, we can remove the inode
> lock.
>
> Will write a patch and see if it passes our tests.

That's great, thanks Roberto.  Assuming all goes well we'll want to
backport this everywhere we merged the remap_file_pages() patch.

-- 
paul-moore.com