[v3,0/6] ima: Remove unnecessary inode locks

Message

Roberto Sassu Jan. 22, 2025, 5:24 p.m. UTC

From: Roberto Sassu <[e-mail]>

A recent syzbot report [1] showed a possible lock inversion between the
mmap lock and the inode lock. Paul Moore suggested to remove the inode lock
in IMA as a possible solution. A first patch set was made [2] to fulfill
that request, although incomplete due to not removing the inode lock around
the ima_appraise_measurement() call.

While the original report was fixed in a different way, by calling the
security_mmap_file() hook outside the mmap lock critical region in the
remap_file_pages() system call [3], the IMA patch set has benefits on its
own, since it merges two critical sections in one in process_measurement(),
and makes the latter and __ima_inode_hash() compete for the same lock.

Remove the inode lock in three phases (except from ima_update_xattr(), when
setting security.ima).

First, remove the S_IMA inode flag and the IS_IMA() macro, since S_IMA
needs to be set under the inode lock, and it is going to be removed. There
is no performance penalty in doing that, since the pointer of the inode
integrity metadata has been moved to the inode security blob when IMA was
made as a regular LSM [4], and retrieving such metadata can be done in
constant time (patch 1).

Second, move the mutex from the inode integrity metadata to the inode
security blob, so that the lock can be taken regardless of whether or not
the inode integrity metadata was allocated for that inode (patch 2).

Consequently, remove the inode lock just after the policy evaluation and
extend the critical region previously guarded by the integrity inode
metadata mutex to where the inode lock was.

Also, make sure that ima_inode_get() is called with the new mutex lock
held, to avoid non-atomic check/assignment of the integrity metadata in the
inode security blob (patch 3), and mark the pointer of inode integrity
metadata as a shared resource with READ_ONCE() and WRITE_ONCE() (patch 4).

Third, remove the inode lock around ima_appraise_measurement() by
postponing setting security.ima when IMA-Appraisal is in fix mode, to when
the file is closed (patch 5).

Finally, as a bug fix, do not reset the IMA_NEW_FILE flag when setting
inode attributes. By resetting the flag, IMA was not able to appraise new
files with modified metadata before security.ima was written to the disk
(patch 6).

This patch set applies on top of commit 4785ed362a24 ("ima: ignore suffixed
policy rule comments") of:

https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git


[1] https://lore.kernel.org/linux-security-module/[e-mail]/
[2] https://lore.kernel.org/linux-security-module/[e-mail]/
[3] https://lore.kernel.org/linux-security-module/[e-mail]/
[4] https://lore.kernel.org/linux-security-module/[e-mail]/

v2:
- Add recommendation to set evm=fix in the kernel command line (suggested
  by Mimi)
- Completely remove S_IMA definition (suggested by Jan Kara)
- Reverse mode and i_writecount checks in ima_check_last_writer()
  (suggested by Mimi)
- Drop O_PATH patch (ima_check_last_writer() was erroneously considering
  file descriptors with that open flag set due to a bug in the appraisal
  patch)

v1:
- New patches (1 suggested by Shu Han, 4-6)
- Remove ima_inode_get_iint() and ima_inode_set_iint() and access inode
  integrity metadata from the new ima_iint_cache_lock structure directly
- Return immediately in ima_inode_get() if the inode does not have a
  security blob (suggested by Paul Moore)

Roberto Sassu (6):
  fs: ima: Remove S_IMA and IS_IMA()
  ima: Remove inode lock
  ima: Detect if lock is held when iint pointer is set in inode security
    blob
  ima: Mark concurrent accesses to the iint pointer in the inode
    security blob
  ima: Defer fixing security.ima to __fput()
  ima: Reset IMA_NONACTION_RULE_FLAGS after post_setattr

 .../admin-guide/kernel-parameters.txt         |  3 +
 include/linux/fs.h                            |  2 -
 security/integrity/ima/ima.h                  | 33 +++----
 security/integrity/ima/ima_api.c              |  4 +-
 security/integrity/ima/ima_appraise.c         |  7 +-
 security/integrity/ima/ima_iint.c             | 95 +++++++++++++++----
 security/integrity/ima/ima_main.c             | 75 ++++++++-------
 7 files changed, 144 insertions(+), 75 deletions(-)