Re: [syzbot] [integrity?] [lsm?] possible deadlock in process_measurement (4)

[Roberto Sassu]

On Wed, 2024-10-02 at 23:09 -0400, Paul Moore wrote:
> On Sat, Sep 28, 2024 at 2:08 PM Shu Han <ebpqwerty472123@xxxxxxxxx> 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.

Roberto