On Mon, Oct 7, 2024 at 11:31 AM Roberto Sassu
[off-list ref] wrote:

On Wed, 2024-10-02 at 23:09 -0400, Paul Moore wrote:

quoted

On Sat, Sep 28, 2024 at 2:08 PM Shu Han [off-list ref] wrote:

quoted

quoted

======================================================
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.

quoted

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