[v2,0/7] ima: Remove unnecessary inode locks

Message

Roberto Sassu Nov. 28, 2024, 10:06 a.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 make 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).

Second, 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).

While testing the new functionality, two bugs were found and corrected.
Discard in IMA files opened with the O_PATH open flags, since no data are
accessed (the file descriptor is used for different purposes). Otherwise,
IMA ended up trying to read the files anyway, causing a kernel warning to
be printed in the kernel log (patch 6).

Do not reset the IMA_NEW_FILE flag as a result of setting inode attributes,
as it was before the patch to reintroduce the inode integrity metadata
mutex. By resetting the flag, IMA was not able to appraise new files with
modified metadata before security.ima was written to the disk (patch 7).

[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]/

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 (7):
  fs: ima: Remove S_IMA and IS_IMA()
  ima: Remove inode lock
  ima: Ensure lock is held when setting iint pointer in inode security
    blob
  ima: Mark concurrent accesses to the iint pointer in the inode
    security blob
  ima: Set security.ima on file close when ima_appraise=fix
  ima: Discard files opened with O_PATH
  ima: Reset IMA_NONACTION_RULE_FLAGS after post_setattr

 include/linux/fs.h                    |  3 +-
 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     | 81 +++++++++++++----------
 6 files changed, 146 insertions(+), 77 deletions(-)