From e044374a8a0a99e46f4e6d6751d3042b6d9cc12e Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Thu, 5 Oct 2023 14:15:58 +0300
Subject: [PATCH 1/4] ima: annotate iint mutex to avoid lockdep false positive
warnings
It is not clear that IMA should be nested at all, but as long is it
measures files both on overlayfs and on underlying fs, we need to
annotate the iint mutex to avoid lockdep false positives related to
IMA + overlayfs, same as overlayfs annotates the inode mutex.
Reported-and-tested-by: syzbot+b42fe626038981fb7bfa@syzkaller.appspotmail.com
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
security/integrity/iint.c | 48 ++++++++++++++++++++++++++++++---------
1 file changed, 37 insertions(+), 11 deletions(-)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index a462df827de2..27ea19fb1f54 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -66,9 +66,32 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
return iint;
}
-static void iint_free(struct integrity_iint_cache *iint)
+#define IMA_MAX_NESTING (FILESYSTEM_MAX_STACK_DEPTH+1)
+
+/*
+ * It is not clear that IMA should be nested at all, but as long is it measures
+ * files both on overlayfs and on underlying fs, we need to annotate the iint
+ * mutex to avoid lockdep false positives related to IMA + overlayfs.
+ * See ovl_lockdep_annotate_inode_mutex_key() for more details.
+ */
+static inline void iint_lockdep_annotate(struct integrity_iint_cache *iint,
+ struct inode *inode)
+{
+#ifdef CONFIG_LOCKDEP
+ static struct lock_class_key iint_mutex_key[IMA_MAX_NESTING];
+
+ int depth = inode->i_sb->s_stack_depth;
+
+ if (WARN_ON_ONCE(depth < 0 || depth >= IMA_MAX_NESTING))
+ depth = 0;
+
+ lockdep_set_class(&iint->mutex, &iint_mutex_key[depth]);
+#endif
+}
+
+static void iint_init_always(struct integrity_iint_cache *iint,
+ struct inode *inode)
{
- kfree(iint->ima_hash);
iint->ima_hash = NULL;
iint->version = 0;
iint->flags = 0UL;
@@ -80,6 +103,14 @@ static void iint_free(struct integrity_iint_cache *iint)
iint->ima_creds_status = INTEGRITY_UNKNOWN;
iint->evm_status = INTEGRITY_UNKNOWN;
iint->measured_pcrs = 0;
+ mutex_init(&iint->mutex);
+ iint_lockdep_annotate(iint, inode);
+}
+
+static void iint_free(struct integrity_iint_cache *iint)
+{
+ kfree(iint->ima_hash);
+ mutex_destroy(&iint->mutex);
kmem_cache_free(iint_cache, iint);
}
@@ -104,6 +135,8 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
if (!iint)
return NULL;
+ iint_init_always(iint, inode);
+
write_lock(&integrity_iint_lock);
p = &integrity_iint_tree.rb_node;
@@ -153,25 +186,18 @@ void integrity_inode_free(struct inode *inode)
iint_free(iint);
}
-static void init_once(void *foo)
+static void iint_init_once(void *foo)
{
struct integrity_iint_cache *iint = (struct integrity_iint_cache *) foo;
memset(iint, 0, sizeof(*iint));
- iint->ima_file_status = INTEGRITY_UNKNOWN;
- iint->ima_mmap_status = INTEGRITY_UNKNOWN;
- iint->ima_bprm_status = INTEGRITY_UNKNOWN;
- iint->ima_read_status = INTEGRITY_UNKNOWN;
- iint->ima_creds_status = INTEGRITY_UNKNOWN;
- iint->evm_status = INTEGRITY_UNKNOWN;
- mutex_init(&iint->mutex);
}
static int __init integrity_iintcache_init(void)
{
iint_cache =
kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
- 0, SLAB_PANIC, init_once);
+ 0, SLAB_PANIC, iint_init_once);
return 0;
}
DEFINE_LSM(integrity) = {
From 7b5c3086d1f85448a2a81947b685119c6c9894c8 Mon Sep 17 00:00:00 2001
From: Prasad Pandit <pjp@fedoraproject.org>
Date: Sun, 22 Oct 2023 12:17:23 +0530
Subject: [PATCH 2/4] integrity: fix indentation of config attributes
Fix indentation of config attributes. Attributes are generally
indented with a leading tab(\t) character.
Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
security/integrity/Kconfig | 44 +++++++++++++++++++-------------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 232191ee09e3..1e151e6a5d3f 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -34,10 +34,10 @@ config INTEGRITY_ASYMMETRIC_KEYS
bool "Enable asymmetric keys support"
depends on INTEGRITY_SIGNATURE
default n
- select ASYMMETRIC_KEY_TYPE
- select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
- select CRYPTO_RSA
- select X509_CERTIFICATE_PARSER
+ select ASYMMETRIC_KEY_TYPE
+ select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+ select CRYPTO_RSA
+ select X509_CERTIFICATE_PARSER
help
This option enables digital signature verification using
asymmetric keys.
@@ -53,14 +53,14 @@ config INTEGRITY_TRUSTED_KEYRING
keyring.
config INTEGRITY_PLATFORM_KEYRING
- bool "Provide keyring for platform/firmware trusted keys"
- depends on INTEGRITY_ASYMMETRIC_KEYS
- depends on SYSTEM_BLACKLIST_KEYRING
- help
- Provide a separate, distinct keyring for platform trusted keys, which
- the kernel automatically populates during initialization from values
- provided by the platform for verifying the kexec'ed kerned image
- and, possibly, the initramfs signature.
+ bool "Provide keyring for platform/firmware trusted keys"
+ depends on INTEGRITY_ASYMMETRIC_KEYS
+ depends on SYSTEM_BLACKLIST_KEYRING
+ help
+ Provide a separate, distinct keyring for platform trusted keys, which
+ the kernel automatically populates during initialization from values
+ provided by the platform for verifying the kexec'ed kerned image
+ and, possibly, the initramfs signature.
config INTEGRITY_MACHINE_KEYRING
bool "Provide a keyring to which Machine Owner Keys may be added"
@@ -71,10 +71,10 @@ config INTEGRITY_MACHINE_KEYRING
select INTEGRITY_CA_MACHINE_KEYRING if LOAD_PPC_KEYS
select INTEGRITY_CA_MACHINE_KEYRING_MAX if LOAD_PPC_KEYS
help
- If set, provide a keyring to which Machine Owner Keys (MOK) may
- be added. This keyring shall contain just MOK keys. Unlike keys
- in the platform keyring, keys contained in the .machine keyring will
- be trusted within the kernel.
+ If set, provide a keyring to which Machine Owner Keys (MOK) may
+ be added. This keyring shall contain just MOK keys. Unlike keys
+ in the platform keyring, keys contained in the .machine keyring will
+ be trusted within the kernel.
config INTEGRITY_CA_MACHINE_KEYRING
bool "Enforce Machine Keyring CA Restrictions"
@@ -99,14 +99,14 @@ config INTEGRITY_CA_MACHINE_KEYRING_MAX
.platform keyring.
config LOAD_UEFI_KEYS
- depends on INTEGRITY_PLATFORM_KEYRING
- depends on EFI
- def_bool y
+ depends on INTEGRITY_PLATFORM_KEYRING
+ depends on EFI
+ def_bool y
config LOAD_IPL_KEYS
- depends on INTEGRITY_PLATFORM_KEYRING
- depends on S390
- def_bool y
+ depends on INTEGRITY_PLATFORM_KEYRING
+ depends on S390
+ def_bool y
config LOAD_PPC_KEYS
bool "Enable loading of platform and blacklisted keys for POWER"
From b46503068cb9ed63ff1d8250f143354ead0b16eb Mon Sep 17 00:00:00 2001
From: Mimi Zohar <zohar@linux.ibm.com>
Date: Sun, 15 Oct 2023 20:18:03 -0400
Subject: [PATCH 3/4] certs: Only allow certs signed by keys on the builtin
keyring
Originally the secondary trusted keyring provided a keyring to which extra
keys may be added, provided those keys were not blacklisted and were
vouched for by a key built into the kernel or already in the secondary
trusted keyring.
On systems with the machine keyring configured, additional keys may also
be vouched for by a key on the machine keyring.
Prevent loading additional certificates directly onto the secondary
keyring, vouched for by keys on the machine keyring, yet allow these
certificates to be loaded onto other trusted keyrings.
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
certs/Kconfig | 16 +++++++++++++++-
crypto/asymmetric_keys/restrict.c | 4 ++++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/certs/Kconfig b/certs/Kconfig
index 1f109b070877..62036974367c 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -88,7 +88,21 @@ config SECONDARY_TRUSTED_KEYRING
help
If set, provide a keyring to which extra keys may be added, provided
those keys are not blacklisted and are vouched for by a key built
- into the kernel or already in the secondary trusted keyring.
+ into the kernel, machine keyring (if configured), or already in the
+ secondary trusted keyring.
+
+config SECONDARY_TRUSTED_KEYRING_SIGNED_BY_BUILTIN
+ bool "Only allow additional certs signed by keys on the builtin trusted keyring"
+ depends on SECONDARY_TRUSTED_KEYRING
+ help
+ If set, only certificates signed by keys on the builtin trusted
+ keyring may be loaded onto the secondary trusted keyring.
+
+ Note: The machine keyring, if configured, will be linked to the
+ secondary keyring. When enabling this option, it is recommended
+ to also configure INTEGRITY_CA_MACHINE_KEYRING_MAX to prevent
+ linking code signing keys with imputed trust to the secondary
+ trusted keyring.
config SYSTEM_BLACKLIST_KEYRING
bool "Provide system-wide ring of blacklisted keys"
diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 6b69ea40da23..afcd4d101ac5 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -102,6 +102,10 @@ int restrict_link_by_signature(struct key *dest_keyring,
if (use_builtin_keys && !test_bit(KEY_FLAG_BUILTIN, &key->flags))
ret = -ENOKEY;
+ else if (IS_BUILTIN(CONFIG_SECONDARY_TRUSTED_KEYRING_SIGNED_BY_BUILTIN) &&
+ !strcmp(dest_keyring->description, ".secondary_trusted_keys") &&
+ !test_bit(KEY_FLAG_BUILTIN, &key->flags))
+ ret = -ENOKEY;
else
ret = verify_signature(key, sig);
key_put(key);
From b836c4d29f2744200b2af41e14bf50758dddc818 Mon Sep 17 00:00:00 2001
From: Mimi Zohar <zohar@linux.ibm.com>
Date: Wed, 18 Oct 2023 14:47:02 -0400
Subject: [PATCH 4/4] ima: detect changes to the backing overlay file
Commit 18b44bc5a672 ("ovl: Always reevaluate the file signature for
IMA") forced signature re-evaulation on every file access.
Instead of always re-evaluating the file's integrity, detect a change
to the backing file, by comparing the cached file metadata with the
backing file's metadata. Verifying just the i_version has not changed
is insufficient. In addition save and compare the i_ino and s_dev
as well.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Tested-by: Eric Snowberg <eric.snowberg@oracle.com>
Tested-by: Raul E Rangel <rrangel@chromium.org>
Cc: stable@vger.kernel.org
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
fs/overlayfs/super.c | 2 +-
security/integrity/ima/ima_api.c | 5 +++++
security/integrity/ima/ima_main.c | 16 +++++++++++++++-
security/integrity/integrity.h | 2 ++
4 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 3fa2416264a4..c71d185980c0 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1489,7 +1489,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
ovl_trusted_xattr_handlers;
sb->s_fs_info = ofs;
sb->s_flags |= SB_POSIXACL;
- sb->s_iflags |= SB_I_SKIP_SYNC | SB_I_IMA_UNVERIFIABLE_SIGNATURE;
+ sb->s_iflags |= SB_I_SKIP_SYNC;
err = -ENOMEM;
root_dentry = ovl_get_root(sb, ctx->upper.dentry, oe);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 452e80b541e5..597ea0c4d72f 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -243,6 +243,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
{
const char *audit_cause = "failed";
struct inode *inode = file_inode(file);
+ struct inode *real_inode = d_real_inode(file_dentry(file));
const char *filename = file->f_path.dentry->d_name.name;
struct ima_max_digest_data hash;
struct kstat stat;
@@ -302,6 +303,10 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
iint->ima_hash = tmpbuf;
memcpy(iint->ima_hash, &hash, length);
iint->version = i_version;
+ if (real_inode != inode) {
+ iint->real_ino = real_inode->i_ino;
+ iint->real_dev = real_inode->i_sb->s_dev;
+ }
/* Possibly temporary failure due to type of read (eg. O_DIRECT) */
if (!result)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 365db0e43d7c..cc1217ac2c6f 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -25,6 +25,7 @@
#include <linux/xattr.h>
#include <linux/ima.h>
#include <linux/fs.h>
+#include <linux/iversion.h>
#include "ima.h"
@@ -207,7 +208,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
u32 secid, char *buf, loff_t size, int mask,
enum ima_hooks func)
{
- struct inode *inode = file_inode(file);
+ struct inode *backing_inode, *inode = file_inode(file);
struct integrity_iint_cache *iint = NULL;
struct ima_template_desc *template_desc = NULL;
char *pathbuf = NULL;
@@ -284,6 +285,19 @@ static int process_measurement(struct file *file, const struct cred *cred,
iint->measured_pcrs = 0;
}
+ /* Detect and re-evaluate changes made to the backing file. */
+ backing_inode = d_real_inode(file_dentry(file));
+ if (backing_inode != inode &&
+ (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) {
+ if (!IS_I_VERSION(backing_inode) ||
+ backing_inode->i_sb->s_dev != iint->real_dev ||
+ backing_inode->i_ino != iint->real_ino ||
+ !inode_eq_iversion(backing_inode, iint->version)) {
+ iint->flags &= ~IMA_DONE_MASK;
+ iint->measured_pcrs = 0;
+ }
+ }
+
/* Determine if already appraised/measured based on bitmask
* (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
* IMA_AUDIT, IMA_AUDITED)
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index d7553c93f5c0..9561db7cf6b4 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -164,6 +164,8 @@ struct integrity_iint_cache {
unsigned long flags;
unsigned long measured_pcrs;
unsigned long atomic_flags;
+ unsigned long real_ino;
+ dev_t real_dev;
enum integrity_status ima_file_status:4;
enum integrity_status ima_mmap_status:4;
enum integrity_status ima_bprm_status:4;