From: Roberto Sassu <[email protected]>
One of the IMA shortcomings over the years has been the availability of
reference digest values for appraisal. Recently, the situation improved
and some Linux distributions are including file signatures.
The digest_cache LSM takes a different approach. Instead of requiring
Linux distributions to include file signatures in their packages, it parses
the digests from signed RPM package headers and exposes an API for
integrity providers to query a digest.
That enables Linux distributions to immediately gain the ability to do
integrity checks with the existing packages, lowering the burden for
software vendors.
In addition, integrating IMA with the digest_cache LSMs has even more
benefits.
First, it allows generating a new-style masurement list including the RPM
package headers and the unknown files, which improves system performance
due to the lower usage of the TPM. The cost is the less accuracy of the
information reported, which might not suitable for everyone.
Second, performance improve for appraisal too. It has been found that
verifying the signatures of only the RPM package headers and doing a digest
lookup is much less computationally expensive than verifying individual
file signatures.
For reference, a preliminary performance evaluation has been published
here:
https://lore.kernel.org/linux-integrity/[email protected]/
Third, it makes a PCR predictable and suitable for TPM key sealing
policies.
Finally, it allows IMA to maintain a predictable PCR and to perform
appraisal from the very beginning of the boot, in the initial ram disk
(of course, it won't recognize automatically generated files, that don't
exist in the RPM packages).
This patch set has some prerequisites:
- KEYS: Introduce user asymmetric keys and signatures (PGP keys and sigs)
- security: Move IMA and EVM to the LSM infrastructure
- security: digest_cache LSM (+digest_cache_changed(), introduced later)
Integration of IMA with the digest_cache LSM is straightforward.
Patch 1 lets IMA know when the digest_cache LSM is reading a digest list,
to populate a digest cache.
Patch 2 allows nested IMA verification of digest lists read by the
digest_cache LSM.
Patch 3 allows the usage of digest caches with the IMA policy.
Patch 4 introduces new boot-time policies, to use digest caches from the
very beginning (it allows measurement/appraisal from the initial ram disk).
Patch 5 attaches the verification result of the digest list to the digest
cache being populated with that digest list.
Patch 6-7 enable the usage of digest caches respectively for measurement
and appraisal, at the condition that it is authorized with the IMA policy
and that the digest list itself was measured and appraised too.
Patch 8 detects digest cache changes and consequently resets the IMA
cached verification result.
Roberto Sassu (8):
ima: Introduce hook DIGEST_LIST_CHECK
ima: Nest iint mutex for DIGEST_LIST_CHECK hook
ima: Add digest_cache policy keyword
ima: Add digest_cache_measure and digest_cache_appraise boot-time
policies
ima: Record IMA verification result of digest lists in digest cache
ima: Use digest cache for measurement
ima: Use digest cache for appraisal
ima: Detect if digest cache changed since last measurement/appraisal
Documentation/ABI/testing/ima_policy | 6 +-
.../admin-guide/kernel-parameters.txt | 15 ++-
security/integrity/ima/Kconfig | 10 ++
security/integrity/ima/ima.h | 24 +++-
security/integrity/ima/ima_api.c | 21 +++-
security/integrity/ima/ima_appraise.c | 33 +++--
security/integrity/ima/ima_iint.c | 14 ++-
security/integrity/ima/ima_main.c | 81 ++++++++++--
security/integrity/ima/ima_policy.c | 118 +++++++++++++++++-
9 files changed, 285 insertions(+), 37 deletions(-)
--
2.34.1
From: Roberto Sassu <[email protected]>
Introduce a new hook to check the integrity of digest lists.
The new hook is invoked during a kernel read with file type
READING_DIGEST LIST, which is done by the digest_cache LSM when it is
populating a digest cache with a digest list.
Signed-off-by: Roberto Sassu <[email protected]>
---
Documentation/ABI/testing/ima_policy | 1 +
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_main.c | 3 ++-
security/integrity/ima/ima_policy.c | 3 +++
4 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index c2385183826c..22237fec5532 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -36,6 +36,7 @@ Description:
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
[KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
[SETXATTR_CHECK][MMAP_CHECK_REQPROT]
+ [DIGEST_LIST_CHECK]
mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
[[^]MAY_EXEC]
fsmagic:= hex value
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 11d7c0332207..cea4517e73ab 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -310,6 +310,7 @@ static inline unsigned int ima_hash_key(u8 *digest)
hook(KEY_CHECK, key) \
hook(CRITICAL_DATA, critical_data) \
hook(SETXATTR_CHECK, setxattr_check) \
+ hook(DIGEST_LIST_CHECK, digest_list_check) \
hook(MAX_CHECK, none)
#define __ima_hook_enumify(ENUM, str) ENUM,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c84e8c55333d..780627b0cde7 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -785,7 +785,8 @@ const int read_idmap[READING_MAX_ID] = {
[READING_MODULE] = MODULE_CHECK,
[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
- [READING_POLICY] = POLICY_CHECK
+ [READING_POLICY] = POLICY_CHECK,
+ [READING_DIGEST_LIST] = DIGEST_LIST_CHECK,
};
/**
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index c0556907c2e6..7cfd1860791f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1287,6 +1287,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
case MODULE_CHECK:
case KEXEC_KERNEL_CHECK:
case KEXEC_INITRAMFS_CHECK:
+ case DIGEST_LIST_CHECK:
if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
IMA_UID | IMA_FOWNER | IMA_FSUUID |
IMA_INMASK | IMA_EUID | IMA_PCR |
@@ -1530,6 +1531,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
entry->func = CRITICAL_DATA;
else if (strcmp(args[0].from, "SETXATTR_CHECK") == 0)
entry->func = SETXATTR_CHECK;
+ else if (strcmp(args[0].from, "DIGEST_LIST_CHECK") == 0)
+ entry->func = DIGEST_LIST_CHECK;
else
result = -EINVAL;
if (!result)
--
2.34.1
From: Roberto Sassu <[email protected]>
Add the 'digest_cache=' policy keyword, to enable the usage of digest
caches for specific IMA actions and purposes.
At the moment, it accepts only 'content' as value, as digest caches can be
only used only for measurement and appraisal of file content. In the
future, it might be possible to use them for file metadata too.
The 'digest_cache=' keyword can be specified for the subset of IMA hooks
listed in ima_digest_cache_func_allowed().
POLICY_CHECK has been excluded for measurement, because policy changes must
be visible in the IMA measurement list. For appraisal, instead, it might be
useful to load custom policies in the initial ram disk (no security.ima
xattr).
Add the digest_cache_mask member to the ima_rule_entry structure, and set
the flag IMA_DIGEST_CACHE_MEASURE_CONTENT if 'digest_cache=content' was
specified for a measure rule, IMA_DIGEST_CACHE_APPRAISE_CONTENT for an
appraise rule.
Propagate the mask down to ima_match_policy() and ima_get_action(), so that
process_measurement() can make the final decision on whether or not digest
caches should be used to measure/appraise the file being evaluated.
Since using digest caches changes the meaning of the IMA measurement list,
which will include only digest lists and unknown files, enforce specifying
'pcr=' with a non-standard value, when 'digest_cache=content' is specified
in a measure rule.
This removes the ambiguity on the meaning of the IMA measurement list.
Signed-off-by: Roberto Sassu <[email protected]>
---
Documentation/ABI/testing/ima_policy | 5 +-
security/integrity/ima/ima.h | 10 +++-
security/integrity/ima/ima_api.c | 6 ++-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_main.c | 8 +--
security/integrity/ima/ima_policy.c | 70 ++++++++++++++++++++++++++-
6 files changed, 89 insertions(+), 12 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 22237fec5532..be045fb60530 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,7 +29,7 @@ Description:
[obj_user=] [obj_role=] [obj_type=]]
option: [digest_type=] [template=] [permit_directio]
[appraise_type=] [appraise_flag=]
- [appraise_algos=] [keyrings=]
+ [appraise_algos=] [keyrings=] [digest_cache=]
base:
func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
[FIRMWARE_CHECK]
@@ -77,6 +77,9 @@ Description:
For example, "sha256,sha512" to only accept to appraise
files where the security.ima xattr was hashed with one
of these two algorithms.
+ digest_cache:= [content]
+ "content" means that the digest cache is used only
+ for file content measurement and/or appraisal.
default policy:
# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c9140a57b591..deee56d99d6f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -43,6 +43,10 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
#define NR_BANKS(chip) ((chip != NULL) ? chip->nr_allocated_banks : 0)
+/* Digest cache usage flags. */
+#define IMA_DIGEST_CACHE_MEASURE_CONTENT 0x0000000000000001
+#define IMA_DIGEST_CACHE_APPRAISE_CONTENT 0x0000000000000002
+
/* current content of the policy */
extern int ima_policy_flag;
@@ -367,7 +371,8 @@ int ima_get_action(struct mnt_idmap *idmap, struct inode *inode,
const struct cred *cred, u32 secid, int mask,
enum ima_hooks func, int *pcr,
struct ima_template_desc **template_desc,
- const char *func_data, unsigned int *allowed_algos);
+ const char *func_data, unsigned int *allowed_algos,
+ u64 *digest_cache_mask);
int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
void *buf, loff_t size, enum hash_algo algo,
@@ -398,7 +403,8 @@ int ima_match_policy(struct mnt_idmap *idmap, struct inode *inode,
const struct cred *cred, u32 secid, enum ima_hooks func,
int mask, int flags, int *pcr,
struct ima_template_desc **template_desc,
- const char *func_data, unsigned int *allowed_algos);
+ const char *func_data, unsigned int *allowed_algos,
+ u64 *digest_cache_mask);
void ima_init_policy(void);
void ima_update_policy(void);
void ima_update_policy_flags(void);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index b37d043d5748..87e286ace43c 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -173,6 +173,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
* @template_desc: pointer filled in if matched measure policy sets template=
* @func_data: func specific data, may be NULL
* @allowed_algos: allowlist of hash algorithms for the IMA xattr
+ * @digest_cache_mask: Actions and purposes for which digest cache is allowed
*
* The policy is defined in terms of keypairs:
* subj=, obj=, type=, func=, mask=, fsmagic=
@@ -190,7 +191,8 @@ int ima_get_action(struct mnt_idmap *idmap, struct inode *inode,
const struct cred *cred, u32 secid, int mask,
enum ima_hooks func, int *pcr,
struct ima_template_desc **template_desc,
- const char *func_data, unsigned int *allowed_algos)
+ const char *func_data, unsigned int *allowed_algos,
+ u64 *digest_cache_mask)
{
int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
@@ -198,7 +200,7 @@ int ima_get_action(struct mnt_idmap *idmap, struct inode *inode,
return ima_match_policy(idmap, inode, cred, secid, func, mask,
flags, pcr, template_desc, func_data,
- allowed_algos);
+ allowed_algos, digest_cache_mask);
}
static bool ima_get_verity_digest(struct ima_iint_cache *iint,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 3497741caea9..27ccc9a2c09f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -81,7 +81,7 @@ int ima_must_appraise(struct mnt_idmap *idmap, struct inode *inode,
security_current_getsecid_subj(&secid);
return ima_match_policy(idmap, inode, current_cred(), secid,
func, mask, IMA_APPRAISE | IMA_HASH, NULL,
- NULL, NULL, NULL);
+ NULL, NULL, NULL, NULL);
}
static int ima_fix_xattr(struct dentry *dentry, struct ima_iint_cache *iint)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 18285fc8ac07..e3ca80098c4c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -232,7 +232,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
*/
action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
mask, func, &pcr, &template_desc, NULL,
- &allowed_algos);
+ &allowed_algos, NULL);
violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
func == MMAP_CHECK_REQPROT) &&
(ima_policy_flag & IMA_MEASURE));
@@ -489,11 +489,11 @@ static int ima_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
inode = file_inode(vma->vm_file);
action = ima_get_action(file_mnt_idmap(vma->vm_file), inode,
current_cred(), secid, MAY_EXEC, MMAP_CHECK,
- &pcr, &template, NULL, NULL);
+ &pcr, &template, NULL, NULL, NULL);
action |= ima_get_action(file_mnt_idmap(vma->vm_file), inode,
current_cred(), secid, MAY_EXEC,
MMAP_CHECK_REQPROT, &pcr, &template, NULL,
- NULL);
+ NULL, NULL);
/* Is the mmap'ed file in policy? */
if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK)))
@@ -972,7 +972,7 @@ int process_buffer_measurement(struct mnt_idmap *idmap,
security_current_getsecid_subj(&secid);
action = ima_get_action(idmap, inode, current_cred(),
secid, 0, func, &pcr, &template,
- func_data, NULL);
+ func_data, NULL, NULL);
if (!(action & IMA_MEASURE) && !digest)
return -ENOENT;
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 7cfd1860791f..4ac83df8d255 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -122,6 +122,7 @@ struct ima_rule_entry {
struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
struct ima_rule_opt_list *label; /* Measure data grouped under this label */
struct ima_template_desc *template;
+ u64 digest_cache_mask; /* Actions and purposes for which digest cache is allowed */
};
/*
@@ -726,6 +727,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
* @template_desc: the template that should be used for this rule
* @func_data: func specific data, may be NULL
* @allowed_algos: allowlist of hash algorithms for the IMA xattr
+ * @digest_cache_mask: Actions and purposes for which digest cache is allowed
*
* Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
* conditions.
@@ -738,7 +740,8 @@ int ima_match_policy(struct mnt_idmap *idmap, struct inode *inode,
const struct cred *cred, u32 secid, enum ima_hooks func,
int mask, int flags, int *pcr,
struct ima_template_desc **template_desc,
- const char *func_data, unsigned int *allowed_algos)
+ const char *func_data, unsigned int *allowed_algos,
+ u64 *digest_cache_mask)
{
struct ima_rule_entry *entry;
int action = 0, actmask = flags | (flags << 1);
@@ -783,6 +786,9 @@ int ima_match_policy(struct mnt_idmap *idmap, struct inode *inode,
if (template_desc && entry->template)
*template_desc = entry->template;
+ if (digest_cache_mask)
+ *digest_cache_mask |= entry->digest_cache_mask;
+
if (!actmask)
break;
}
@@ -859,6 +865,30 @@ static int ima_appraise_flag(enum ima_hooks func)
return 0;
}
+static bool ima_digest_cache_func_allowed(struct ima_rule_entry *entry)
+{
+ switch (entry->func) {
+ case NONE:
+ case FILE_CHECK:
+ case MMAP_CHECK:
+ case MMAP_CHECK_REQPROT:
+ case BPRM_CHECK:
+ case CREDS_CHECK:
+ case FIRMWARE_CHECK:
+ case POLICY_CHECK:
+ case MODULE_CHECK:
+ case KEXEC_KERNEL_CHECK:
+ case KEXEC_INITRAMFS_CHECK:
+ /* Exception: always add policy updates to measurement list! */
+ if (entry->action == MEASURE && entry->func == POLICY_CHECK)
+ return false;
+
+ return true;
+ default:
+ return false;
+ }
+}
+
static void add_rules(struct ima_rule_entry *entries, int count,
enum policy_rule_list policy_rule)
{
@@ -1073,7 +1103,7 @@ enum policy_opt {
Opt_digest_type,
Opt_appraise_type, Opt_appraise_flag, Opt_appraise_algos,
Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
- Opt_label, Opt_err
+ Opt_label, Opt_digest_cache, Opt_err
};
static const match_table_t policy_tokens = {
@@ -1122,6 +1152,7 @@ static const match_table_t policy_tokens = {
{Opt_template, "template=%s"},
{Opt_keyrings, "keyrings=%s"},
{Opt_label, "label=%s"},
+ {Opt_digest_cache, "digest_cache=%s"},
{Opt_err, NULL}
};
@@ -1245,6 +1276,18 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
if (entry->action != MEASURE && entry->flags & IMA_PCR)
return false;
+ /* New-style measurements with digest cache cannot be on default PCR. */
+ if (entry->action == MEASURE &&
+ (entry->digest_cache_mask & IMA_DIGEST_CACHE_MEASURE_CONTENT)) {
+ if (!(entry->flags & IMA_PCR) ||
+ entry->pcr == CONFIG_IMA_MEASURE_PCR_IDX)
+ return false;
+ }
+
+ /* Digest caches can be used only for a subset of the IMA hooks. */
+ if (entry->digest_cache_mask && !ima_digest_cache_func_allowed(entry))
+ return false;
+
if (entry->action != APPRAISE &&
entry->flags & (IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED |
IMA_CHECK_BLACKLIST | IMA_VALIDATE_ALGOS))
@@ -1881,6 +1924,26 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
&(template_desc->num_fields));
entry->template = template_desc;
break;
+ case Opt_digest_cache:
+ ima_log_string(ab, "digest_cache", args[0].from);
+
+ result = -EINVAL;
+
+ if (!strcmp(args[0].from, "content")) {
+ switch (entry->action) {
+ case MEASURE:
+ entry->digest_cache_mask |= IMA_DIGEST_CACHE_MEASURE_CONTENT;
+ result = 0;
+ break;
+ case APPRAISE:
+ entry->digest_cache_mask |= IMA_DIGEST_CACHE_APPRAISE_CONTENT;
+ result = 0;
+ break;
+ default:
+ break;
+ }
+ }
+ break;
case Opt_err:
ima_log_string(ab, "UNKNOWN", p);
result = -EINVAL;
@@ -2271,6 +2334,9 @@ int ima_policy_show(struct seq_file *m, void *v)
seq_puts(m, "digest_type=verity ");
if (entry->flags & IMA_PERMIT_DIRECTIO)
seq_puts(m, "permit_directio ");
+ if ((entry->digest_cache_mask & IMA_DIGEST_CACHE_MEASURE_CONTENT) ||
+ (entry->digest_cache_mask & IMA_DIGEST_CACHE_APPRAISE_CONTENT))
+ seq_puts(m, "digest_cache=content ");
rcu_read_unlock();
seq_puts(m, "\n");
return 0;
--
2.34.1
From: Roberto Sassu <[email protected]>
Specify the 'digest_cache_measure' boot-time policy with 'ima_policy=' in
the kernel command line to add the following rule at the beginning of the
IMA policy, before other rules:
measure func=DIGEST_LIST_CHECK pcr=12
which will measure digest lists into PCR 12 (or the value in
CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX).
'digest_cache_measure' also adds 'digest_cache=content pcr=12' to the other
measure rules, if they have a compatible IMA hook. The PCR value still
comes from CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX.
Specify 'digest_cache_appraise' to add the following rule at the beginning,
before other rules:
appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig
which will appraise digest lists with IMA signatures or module-style
appended signatures.
'digest_cache_appraise' also adds 'digest_cache=content' to the other
appraise rules, if they have a compatible IMA hook.
Signed-off-by: Roberto Sassu <[email protected]>
---
.../admin-guide/kernel-parameters.txt | 15 ++++++-
security/integrity/ima/Kconfig | 10 +++++
security/integrity/ima/ima_policy.c | 45 +++++++++++++++++++
3 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 31b3a25680d0..a79967fcba7d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2011,7 +2011,8 @@
ima_policy= [IMA]
The builtin policies to load during IMA setup.
Format: "tcb | appraise_tcb | secure_boot |
- fail_securely | critical_data"
+ fail_securely | critical_data |
+ digest_cache_measure | digest_cache_appraise"
The "tcb" policy measures all programs exec'd, files
mmap'd for exec, and all files opened with the read
@@ -2033,6 +2034,18 @@
The "critical_data" policy measures kernel integrity
critical data.
+ The "digest_cache_measure" policy measures digest lists
+ into PCR 12 (can be changed with kernel config), enables
+ the digest cache to be used for the other selected
+ measure rules (if compatible), and measures the files
+ with digest not found in the digest list into PCR 12
+ (changeable).
+
+ The "digest_cache_appraise" policy appraises digest
+ lists with IMA signatures or module-style appended
+ signatures, and enables the digest cache to be used for
+ the other selected appraise rules (if compatible).
+
ima_tcb [IMA] Deprecated. Use ima_policy= instead.
Load a policy which meets the needs of the Trusted
Computing Base. This means IMA will measure all
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 475c32615006..6a481019fb6e 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -321,4 +321,14 @@ config IMA_DISABLE_HTABLE
help
This option disables htable to allow measurement of duplicate records.
+config IMA_DIGEST_CACHE_MEASURE_PCR_IDX
+ int
+ range 8 14
+ default 12
+ help
+ This option determines the TPM PCR register index that IMA uses to
+ maintain the integrity aggregate of the measurement list, when the
+ digest_cache LSM is used (different measurement style). If unsure,
+ use the default 12.
+
endif
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 4ac83df8d255..04127f962ef4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -254,6 +254,21 @@ static struct ima_rule_entry critical_data_rules[] __ro_after_init = {
{.action = MEASURE, .func = CRITICAL_DATA, .flags = IMA_FUNC},
};
+static struct ima_rule_entry measure_digest_cache_rule __ro_after_init = {
+#ifdef CONFIG_SECURITY_DIGEST_CACHE
+ .action = MEASURE, .func = DIGEST_LIST_CHECK,
+ .pcr = CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX,
+ .flags = IMA_FUNC | IMA_PCR
+#endif
+};
+
+static struct ima_rule_entry appraise_digest_cache_rule __ro_after_init = {
+#ifdef CONFIG_SECURITY_DIGEST_CACHE
+ .action = APPRAISE, .func = DIGEST_LIST_CHECK,
+ .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED,
+#endif
+};
+
/* An array of architecture specific rules */
static struct ima_rule_entry *arch_policy_entry __ro_after_init;
@@ -278,6 +293,8 @@ static bool ima_use_appraise_tcb __initdata;
static bool ima_use_secure_boot __initdata;
static bool ima_use_critical_data __initdata;
static bool ima_fail_unverifiable_sigs __ro_after_init;
+static bool ima_digest_cache_measure __ro_after_init;
+static bool ima_digest_cache_appraise __ro_after_init;
static int __init policy_setup(char *str)
{
char *p;
@@ -295,6 +312,10 @@ static int __init policy_setup(char *str)
ima_use_critical_data = true;
else if (strcmp(p, "fail_securely") == 0)
ima_fail_unverifiable_sigs = true;
+ else if (strcmp(p, "digest_cache_measure") == 0)
+ ima_digest_cache_measure = true;
+ else if (strcmp(p, "digest_cache_appraise") == 0)
+ ima_digest_cache_appraise = true;
else
pr_err("policy \"%s\" not found", p);
}
@@ -897,6 +918,20 @@ static void add_rules(struct ima_rule_entry *entries, int count,
for (i = 0; i < count; i++) {
struct ima_rule_entry *entry;
+ if (IS_ENABLED(CONFIG_SECURITY_DIGEST_CACHE) &&
+ entries[i].action == MEASURE && ima_digest_cache_measure &&
+ ima_digest_cache_func_allowed(&entries[i])) {
+ entries[i].digest_cache_mask |= IMA_DIGEST_CACHE_MEASURE_CONTENT;
+ entries[i].pcr = CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX;
+ entries[i].flags |= IMA_PCR;
+ }
+
+ if (IS_ENABLED(CONFIG_SECURITY_DIGEST_CACHE) &&
+ entries[i].action == APPRAISE &&
+ ima_digest_cache_appraise &&
+ ima_digest_cache_func_allowed(&entries[i]))
+ entries[i].digest_cache_mask |= IMA_DIGEST_CACHE_APPRAISE_CONTENT;
+
if (policy_rule & IMA_DEFAULT_POLICY)
list_add_tail(&entries[i].list, &ima_default_rules);
@@ -971,6 +1006,16 @@ void __init ima_init_policy(void)
{
int build_appraise_entries, arch_entries;
+ /*
+ * We need to load digest cache rules at the beginning, to avoid dont_
+ * rules causing ours to not be reached.
+ */
+ if (ima_digest_cache_measure)
+ add_rules(&measure_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
+
+ if (ima_digest_cache_appraise)
+ add_rules(&appraise_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
+
/* if !ima_policy, we load NO default rules */
if (ima_policy)
add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules),
--
2.34.1
From: Roberto Sassu <[email protected]>
The digest_cache LSM allows integrity providers to record how the digest
list being used to populate the digest cache was verified.
Integrity providers can register a kernel_post_read_file LSM hook
implementation, and call digest_cache_verif_set() providing the result of
the digest list verification, together with the digest list file
descriptor.
IMA calls digest_cache_verif_set() during the DIGEST_LIST_CHECK hook
(kernel read with file type READING_DIGEST_LIST), and attaches to the
digest cache a u64 variable with the IMA_DIGEST_CACHE_MEASURE_CONTENT and
IMA_DIGEST_CACHE_APPRAISE_CONTENT flags set, if the digest list was
respectively measured and appraised.
The same flags are set in another u64 variable, if 'digest_cache=content'
appears respectively in a measure or appraise rule.
The final decision on whether the digest cache can be used for measurement
and appraisal depends on the AND of these two variables, so it must have
been authorized with the IMA policy and the same action must have been done
on the digest list.
This prevents remote verifiers from receiving an incomplete IMA measurement
list, where measurements are skipped, but there isn't the digest list the
calculated file digest was search into. It also prevents successful
appraisal without appraising the digest list itself.
Signed-off-by: Roberto Sassu <[email protected]>
---
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_main.c | 19 ++++++++++++++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index deee56d99d6f..2dbcaf0a9402 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -20,6 +20,7 @@
#include <linux/hash.h>
#include <linux/tpm.h>
#include <linux/audit.h>
+#include <linux/digest_cache.h>
#include <crypto/hash_info.h>
#include "../integrity.h"
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e3ca80098c4c..3fc48214850a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -214,7 +214,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
char *pathbuf = NULL;
char filename[NAME_MAX];
const char *pathname = NULL;
- int rc = 0, action, must_appraise = 0;
+ int rc = 0, digest_cache_rc, action, must_appraise = 0;
int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
struct evm_ima_xattr_data *xattr_value = NULL;
struct modsig *modsig = NULL;
@@ -222,6 +222,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
bool violation_check;
enum hash_algo hash_algo;
unsigned int allowed_algos = 0;
+ u64 verif_mask = 0;
if (!ima_policy_flag || !S_ISREG(inode->i_mode))
return 0;
@@ -399,6 +400,22 @@ static int process_measurement(struct file *file, const struct cred *cred,
if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) &&
!(iint->flags & IMA_NEW_FILE))
rc = -EACCES;
+ if (!rc && func == DIGEST_LIST_CHECK) {
+ if (iint->flags & IMA_MEASURED)
+ verif_mask |= IMA_DIGEST_CACHE_MEASURE_CONTENT;
+ if (iint->flags & IMA_APPRAISED_SUBMASK)
+ verif_mask |= IMA_DIGEST_CACHE_APPRAISE_CONTENT;
+
+ /* Remember actions done on digest list for later use. */
+ digest_cache_rc = digest_cache_verif_set(file, "ima",
+ &verif_mask,
+ sizeof(verif_mask));
+ /* Ignore if fd doesn't have digest cache set (prefetching). */
+ if (digest_cache_rc && digest_cache_rc != -ENOENT)
+ pr_debug("Cannot set verification mask for %s, ret: %d, ignoring\n",
+ file_dentry(file)->d_name.name,
+ digest_cache_rc);
+ }
mutex_unlock(&iint->mutex);
kfree(xattr_value);
ima_free_modsig(modsig);
--
2.34.1
From: Roberto Sassu <[email protected]>
Introduce a new measurement style using digest caches, which can be
performed exclusively on non-standard PCRs, to avoid ambiguity.
While a measurement on the standard PCR means that a file was accessed and
had the measured content, a measurement with the digest cache means only
that the calculated file digest was not found in any of the measured digest
lists (any digest list used for the search must be measured, otherwise IMA
wouldn't use it).
The new measurement style does not tell: whether or not the file was
actually accessed (since its measurement is skipped even if it was); in
which sequence files were accessed. So, one has to guess that the system
might have possibly accessed any of the files whose digest is in the
measured digest lists, in any order.
However, it has the following benefits: the IMA measurement list can be
much shorter, system performance can be much better due to less PCR extend
operations (see the performance evaluation in the digest_cache LSM
documentation); the PCR can be predictable as long as the set of measured
digest lists does not change (which obviously happens during software
updates).
The PCR can be predictable because the digest_cache LSM has a prefetching
mechanism that reads digest lists in a deterministic order, until it
finds the digest list containing the digest calculated by IMA from an
accessed file. If IMA measures digest lists, the PCR is extended in a
deterministic order too.
Predictable PCR means that a TPM key can be made dependent on specific PCR
values (or a OR of them, depending on the key policy). Accessing a file
with an unknown digest immediately makes that TPM key unusable, requiring a
reboot to use it again.
This mechanism can be used for the so called implicit remote attestation,
where the ability of a system to respond to challenges based on the private
part of the TPM key means that the system has the expected PCR values
(which would mean that the integrity of the system is ok). This is opposed
to the explicit remote attestation, where a system has to send all its
measurements, to prove to a remote party about its integrity.
If the IMA policy allows the usage of the digest cache for the current file
access (except for DIGEST_LIST_CHECK hook, not supported), call
digest_cache_get() in process_measurement() to get a digest cache for that
file, and call digest_cache_lookup() to search the calculated file digest.
Doing the lookup is necessary to retrieve the digest cache containing the
digest, since digest_cache_get() might only return a directory digest
cache, useful only to iterate over the digest caches of the directory
entries.
If digest_cache_lookup() returns a positive value (digest cache reference
in the digest_cache_found_t form), call digest_cache_from_found_t() to get
an usable digest cache pointer, and digest_cache_verif_get() to get
the verification result of the corresponding digest list, and AND it with
the policy mask.
Then, pass the AND result to ima_store_measurement() and, if the result has
the IMA_DIGEST_CACHE_MEASURE_CONTENT flag set, behave as if the file was
successfully added to the IMA measurement list (i.e. set the IMA_MEASURED
flag and the PCR flag from the value specified in the matching policy
rule), but actually don't do it.
Finally, release the digest cache reference acquired with
digest_cache_get(), by calling digest_cache_put().
Signed-off-by: Roberto Sassu <[email protected]>
---
security/integrity/ima/ima.h | 3 ++-
security/integrity/ima/ima_api.c | 15 ++++++++++++++-
security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++++++---
3 files changed, 45 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 2dbcaf0a9402..cf04f5a22234 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -382,7 +382,8 @@ void ima_store_measurement(struct ima_iint_cache *iint, struct file *file,
const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
- struct ima_template_desc *template_desc);
+ struct ima_template_desc *template_desc,
+ u64 digest_cache_mask);
int process_buffer_measurement(struct mnt_idmap *idmap,
struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 87e286ace43c..b216f86c983d 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -345,7 +345,8 @@ void ima_store_measurement(struct ima_iint_cache *iint, struct file *file,
const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
- struct ima_template_desc *template_desc)
+ struct ima_template_desc *template_desc,
+ u64 digest_cache_mask)
{
static const char op[] = "add_template_measure";
static const char audit_cause[] = "ENOMEM";
@@ -369,6 +370,18 @@ void ima_store_measurement(struct ima_iint_cache *iint, struct file *file,
if (iint->measured_pcrs & (0x1 << pcr) && !modsig)
return;
+ /*
+ * If digest cache usage was authorized with the IMA policy, the digest
+ * list the digest cache was populated from was measured, and the file
+ * digest was found in the digest cache, mark the file as successfully
+ * measured.
+ */
+ if (digest_cache_mask & IMA_DIGEST_CACHE_MEASURE_CONTENT) {
+ iint->flags |= IMA_MEASURED;
+ iint->measured_pcrs |= (0x1 << pcr);
+ return;
+ }
+
result = ima_alloc_init_template(&event_data, &entry, template_desc);
if (result < 0) {
integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 3fc48214850a..48a09747ae7a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -222,7 +222,9 @@ static int process_measurement(struct file *file, const struct cred *cred,
bool violation_check;
enum hash_algo hash_algo;
unsigned int allowed_algos = 0;
- u64 verif_mask = 0;
+ u64 verif_mask = 0, *verif_mask_ptr, policy_mask = 0, allow_mask = 0;
+ struct digest_cache *digest_cache = NULL, *found_cache;
+ digest_cache_found_t found;
if (!ima_policy_flag || !S_ISREG(inode->i_mode))
return 0;
@@ -233,7 +235,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
*/
action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
mask, func, &pcr, &template_desc, NULL,
- &allowed_algos, NULL);
+ &allowed_algos, &policy_mask);
violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
func == MMAP_CHECK_REQPROT) &&
(ima_policy_flag & IMA_MEASURE));
@@ -364,10 +366,34 @@ static int process_measurement(struct file *file, const struct cred *cred,
if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */
pathname = ima_d_path(&file->f_path, &pathbuf, filename);
+ /*
+ * For now we don't support nested verification with digest caches.
+ * Since we allow IMA policy rules without func=, we have to enforce
+ * this restriction here.
+ */
+ if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
+ digest_cache = digest_cache_get(file_dentry(file));
+
+ if (digest_cache) {
+ found = digest_cache_lookup(file_dentry(file), digest_cache,
+ iint->ima_hash->digest,
+ iint->ima_hash->algo);
+ /* AND what is allowed by the policy, and what IMA verified. */
+ if (found) {
+ found_cache = digest_cache_from_found_t(found);
+ verif_mask_ptr = digest_cache_verif_get(found_cache,
+ "ima");
+ if (verif_mask_ptr)
+ allow_mask = policy_mask & *verif_mask_ptr;
+ }
+
+ digest_cache_put(digest_cache);
+ }
+
if (action & IMA_MEASURE)
ima_store_measurement(iint, file, pathname,
xattr_value, xattr_len, modsig, pcr,
- template_desc);
+ template_desc, allow_mask);
if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
rc = ima_check_blacklist(iint, modsig, pcr);
if (rc != -EPERM) {
--
2.34.1
From: Roberto Sassu <[email protected]>
Similarly to measurement, enable the new appraisal style too using digest
caches.
Instead of verifying individual file signatures, verify the signature of
lists of digests and search calculated file digests in those lists.
The benefits are that signed lists of digests already exist (e.g. RPM
package headers), although their format needs to be supported by the
digest_cache LSM, and appraisal with digest lists is computationally much
less expensive than with individual file signatures (see the performance
evaluation of the digest_cache LSM).
As for measurement, pass the AND of the policy mask and the digest list
verification mask to ima_appraise_measurement().
If EVM is disabled or the file does not have any protected xattr
(evm_verifyxattr() returns INTEGRITY_UNKNOWN or INTEGRITY_NOXATTRS), the
other appraisal methods (xattr and modsig) are not available, and the AND
of the masks has the IMA_DIGEST_CACHE_APPRAISE_CONTENT flag set, mark the
file as successfully appraised (i.e. set the integrity status to
INTEGRITY_PASS and return zero).
Signed-off-by: Roberto Sassu <[email protected]>
---
security/integrity/ima/ima.h | 6 ++++--
security/integrity/ima/ima_appraise.c | 31 +++++++++++++++++++++------
security/integrity/ima/ima_main.c | 3 ++-
3 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index cf04f5a22234..36faf2bc81b0 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -433,7 +433,8 @@ int ima_check_blacklist(struct ima_iint_cache *iint,
int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
struct file *file, const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
- int xattr_len, const struct modsig *modsig);
+ int xattr_len, const struct modsig *modsig,
+ u64 digest_cache_mask);
int ima_must_appraise(struct mnt_idmap *idmap, struct inode *inode,
int mask, enum ima_hooks func);
void ima_update_xattr(struct ima_iint_cache *iint, struct file *file);
@@ -458,7 +459,8 @@ static inline int ima_appraise_measurement(enum ima_hooks func,
const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
int xattr_len,
- const struct modsig *modsig)
+ const struct modsig *modsig,
+ u8 digest_cache_mask)
{
return INTEGRITY_UNKNOWN;
}
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 27ccc9a2c09f..dcea88d502a9 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -478,7 +478,8 @@ int ima_check_blacklist(struct ima_iint_cache *iint,
int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
struct file *file, const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
- int xattr_len, const struct modsig *modsig)
+ int xattr_len, const struct modsig *modsig,
+ u64 digest_cache_mask)
{
static const char op[] = "appraise_data";
const char *cause = "unknown";
@@ -488,12 +489,19 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
int rc = xattr_len;
bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig;
- /* If not appraising a modsig, we need an xattr. */
- if (!(inode->i_opflags & IOP_XATTR) && !try_modsig)
+ /*
+ * If not appraising a modsig/there is no digest cache match, we need
+ * an xattr.
+ */
+ if (!(inode->i_opflags & IOP_XATTR) && !try_modsig &&
+ !digest_cache_mask)
return INTEGRITY_UNKNOWN;
- /* If reading the xattr failed and there's no modsig, error out. */
- if (rc <= 0 && !try_modsig) {
+ /*
+ * If reading the xattr failed and there's no modsig/digest cache match,
+ * error out.
+ */
+ if (rc <= 0 && !try_modsig && !digest_cache_mask) {
if (rc && rc != -ENODATA)
goto out;
@@ -524,8 +532,11 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
case INTEGRITY_UNKNOWN:
break;
case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
- /* It's fine not to have xattrs when using a modsig. */
- if (try_modsig)
+ /*
+ * It's fine not to have xattrs when using a modsig or the
+ * digest cache.
+ */
+ if (try_modsig || digest_cache_mask)
break;
fallthrough;
case INTEGRITY_NOLABEL: /* No security.evm xattr. */
@@ -555,6 +566,12 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
rc == -ENOKEY))
rc = modsig_verify(func, modsig, &status, &cause);
+ if (!xattr_value && !try_modsig &&
+ (digest_cache_mask & IMA_DIGEST_CACHE_APPRAISE_CONTENT)) {
+ status = INTEGRITY_PASS;
+ rc = 0;
+ }
+
out:
/*
* File signatures on some filesystems can not be properly verified.
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 48a09747ae7a..a66522a22cbc 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -400,7 +400,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
inode_lock(inode);
rc = ima_appraise_measurement(func, iint, file,
pathname, xattr_value,
- xattr_len, modsig);
+ xattr_len, modsig,
+ allow_mask);
inode_unlock(inode);
}
if (!rc)
--
2.34.1
From: Roberto Sassu <[email protected]>
IMA invalidates the cached verification result on file content/metadata
update, so that the file is evaluated again at next access.
While until now checking modifications on the file was sufficient to
determine if the cached verification result is still valid, that no longer
applies if that verification result was obtained with digest caches.
In that case, it is also necessary to check modifications on the digest
lists and on the security.digest_list xattr of the files for which digest
caches are used.
The digest_cache LSM offers the digest_cache_changed() function, which
tells if a file would use a different digest cache than the one passed as
argument. digest_cache_get() might return a different digest cache if the
digest list was modified/deleted/renamed or the security.digest_list xattr
was modified.
Hold a digest cache reference in the IMA integrity metadata, when using it
for measurement/appraisal. At every file access, check if that reference is
still actual by passing it to digest_cache_changed(). If not, reset the
integrity status and do the verification again.
Finally, move the digest_cache_put() call from process_measurement() to
ima_iint_free(), unless the digest cache changed. In that case, still
release the reference in process_measurement().
Signed-off-by: Roberto Sassu <[email protected]>
---
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_iint.c | 3 +++
security/integrity/ima/ima_main.c | 22 ++++++++++++++++++----
3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 36faf2bc81b0..c25bde918cd5 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -192,6 +192,7 @@ struct ima_iint_cache {
enum integrity_status ima_read_status:4;
enum integrity_status ima_creds_status:4;
struct ima_digest_data *ima_hash;
+ struct digest_cache *digest_cache;
};
extern struct lsm_blob_sizes ima_blob_sizes;
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index b4f476fae437..fd369809809f 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -68,6 +68,7 @@ static void ima_iint_init_always(struct ima_iint_cache *iint,
iint->ima_read_status = INTEGRITY_UNKNOWN;
iint->ima_creds_status = INTEGRITY_UNKNOWN;
iint->measured_pcrs = 0;
+ iint->digest_cache = NULL;
mutex_init(&iint->mutex);
ima_iint_lockdep_annotate(iint, inode, nested);
}
@@ -75,6 +76,8 @@ static void ima_iint_init_always(struct ima_iint_cache *iint,
static void ima_iint_free(struct ima_iint_cache *iint)
{
kfree(iint->ima_hash);
+ if (iint->digest_cache)
+ digest_cache_put(iint->digest_cache);
mutex_destroy(&iint->mutex);
kmem_cache_free(ima_iint_cache, iint);
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a66522a22cbc..e1b2f5737753 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -301,6 +301,15 @@ static int process_measurement(struct file *file, const struct cred *cred,
}
}
+ /* Check if digest cache changed since last measurement/appraisal. */
+ if (iint->digest_cache &&
+ digest_cache_changed(inode, iint->digest_cache)) {
+ iint->flags &= ~IMA_DONE_MASK;
+ iint->measured_pcrs = 0;
+ digest_cache_put(iint->digest_cache);
+ iint->digest_cache = NULL;
+ }
+
/* Determine if already appraised/measured based on bitmask
* (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
* IMA_AUDIT, IMA_AUDITED)
@@ -371,8 +380,15 @@ static int process_measurement(struct file *file, const struct cred *cred,
* Since we allow IMA policy rules without func=, we have to enforce
* this restriction here.
*/
- if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
- digest_cache = digest_cache_get(file_dentry(file));
+ if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK) {
+ if (!iint->digest_cache) {
+ /* Released by ima_iint_free(). */
+ digest_cache = digest_cache_get(file_dentry(file));
+ iint->digest_cache = digest_cache;
+ } else {
+ digest_cache = iint->digest_cache;
+ }
+ }
if (digest_cache) {
found = digest_cache_lookup(file_dentry(file), digest_cache,
@@ -386,8 +402,6 @@ static int process_measurement(struct file *file, const struct cred *cred,
if (verif_mask_ptr)
allow_mask = policy_mask & *verif_mask_ptr;
}
-
- digest_cache_put(digest_cache);
}
if (action & IMA_MEASURE)
--
2.34.1
From: Roberto Sassu <[email protected]>
Invoking digest_cache_get() inside the iint->mutex critical region can
cause deadlocks due to the fact that IMA can be recursively invoked for
reading the digest list. The deadlock would occur if the digest_cache LSM
attempts to read the same inode that is already locked by IMA.
However, since the digest_cache LSM makes sure that the above situation
never happens, as it checks the inodes, it is safe to call
digest_cache_get() inside the critical region and nest the iint->mutex
when the DIGEST_LIST_CHECK hook is executed.
Add a lockdep subclass to the iint->mutex, that is 0 if the IMA hook
executed is not DIGEST_LIST_CHECK, and 1 when it is. Since lockdep allows
nesting with higher classes and subclasses, that effectively eliminates the
warning about the unsafe lock.
Pass the new lockdep subclass (nested variable) from ima_inode_get() to
ima_iint_init_always() and ima_iint_lockdep_annotate().
Signed-off-by: Roberto Sassu <[email protected]>
---
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_iint.c | 11 ++++++-----
security/integrity/ima/ima_main.c | 6 +++---
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index cea4517e73ab..c9140a57b591 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -216,7 +216,7 @@ static inline void ima_inode_set_iint(const struct inode *inode,
}
struct ima_iint_cache *ima_iint_find(struct inode *inode);
-struct ima_iint_cache *ima_inode_get(struct inode *inode);
+struct ima_iint_cache *ima_inode_get(struct inode *inode, bool nested);
void ima_inode_free(struct inode *inode);
void __init ima_iintcache_init(void);
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index e7c9c216c1c6..b4f476fae437 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -41,7 +41,7 @@ struct ima_iint_cache *ima_iint_find(struct inode *inode)
* See ovl_lockdep_annotate_inode_mutex_key() for more details.
*/
static inline void ima_iint_lockdep_annotate(struct ima_iint_cache *iint,
- struct inode *inode)
+ struct inode *inode, bool nested)
{
#ifdef CONFIG_LOCKDEP
static struct lock_class_key ima_iint_mutex_key[IMA_MAX_NESTING];
@@ -56,7 +56,7 @@ static inline void ima_iint_lockdep_annotate(struct ima_iint_cache *iint,
}
static void ima_iint_init_always(struct ima_iint_cache *iint,
- struct inode *inode)
+ struct inode *inode, bool nested)
{
iint->ima_hash = NULL;
iint->version = 0;
@@ -69,7 +69,7 @@ static void ima_iint_init_always(struct ima_iint_cache *iint,
iint->ima_creds_status = INTEGRITY_UNKNOWN;
iint->measured_pcrs = 0;
mutex_init(&iint->mutex);
- ima_iint_lockdep_annotate(iint, inode);
+ ima_iint_lockdep_annotate(iint, inode, nested);
}
static void ima_iint_free(struct ima_iint_cache *iint)
@@ -82,13 +82,14 @@ static void ima_iint_free(struct ima_iint_cache *iint)
/**
* ima_inode_get - Find or allocate an iint associated with an inode
* @inode: Pointer to the inode
+ * @nested: Whether or not the iint->mutex lock can be nested
*
* Find an iint associated with an inode, and allocate a new one if not found.
* Caller must lock i_mutex.
*
* Return: An iint on success, NULL on error.
*/
-struct ima_iint_cache *ima_inode_get(struct inode *inode)
+struct ima_iint_cache *ima_inode_get(struct inode *inode, bool nested)
{
struct ima_iint_cache *iint;
@@ -100,7 +101,7 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
if (!iint)
return NULL;
- ima_iint_init_always(iint, inode);
+ ima_iint_init_always(iint, inode, nested);
inode->i_flags |= S_IMA;
ima_inode_set_iint(inode, iint);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 780627b0cde7..18285fc8ac07 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -248,7 +248,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
inode_lock(inode);
if (action) {
- iint = ima_inode_get(inode);
+ iint = ima_inode_get(inode, func == DIGEST_LIST_CHECK);
if (!iint)
rc = -ENOMEM;
}
@@ -699,7 +699,7 @@ static void ima_post_create_tmpfile(struct mnt_idmap *idmap,
return;
/* Nothing to do if we can't allocate memory */
- iint = ima_inode_get(inode);
+ iint = ima_inode_get(inode, false);
if (!iint)
return;
@@ -731,7 +731,7 @@ static void ima_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
return;
/* Nothing to do if we can't allocate memory */
- iint = ima_inode_get(inode);
+ iint = ima_inode_get(inode, false);
if (!iint)
return;
--
2.34.1
On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> Add the 'digest_cache=' policy keyword, to enable the usage of digest
> caches for specific IMA actions and purposes.
>
> At the moment, it accepts only 'content' as value, as digest caches can be
> only used only for measurement and appraisal of file content. In the
> future, it might be possible to use them for file metadata too.
At least from this patch, it is unclear why 'digest_cache' requires an option.
The usage - measure, appraise - is based on 'action'. From an IMA perspective,
does the file content make a difference? And if it did, then file 'data' would
parallel file 'metadata'.
Without having to pass around "digest_cache_mask" would simplify this patch.
Mimi
> The 'digest_cache=' keyword can be specified for the subset of IMA hooks
> listed in ima_digest_cache_func_allowed().
>
> POLICY_CHECK has been excluded for measurement, because policy changes must
> be visible in the IMA measurement list. For appraisal, instead, it might be
> useful to load custom policies in the initial ram disk (no security.ima
> xattr).
>
> Add the digest_cache_mask member to the ima_rule_entry structure, and set
> the flag IMA_DIGEST_CACHE_MEASURE_CONTENT if 'digest_cache=content' was
> specified for a measure rule, IMA_DIGEST_CACHE_APPRAISE_CONTENT for an
> appraise rule.
>
> Propagate the mask down to ima_match_policy() and ima_get_action(), so that
> process_measurement() can make the final decision on whether or not digest
> caches should be used to measure/appraise the file being evaluated.
>
> Since using digest caches changes the meaning of the IMA measurement list,
> which will include only digest lists and unknown files, enforce specifying
> 'pcr=' with a non-standard value, when 'digest_cache=content' is specified
> in a measure rule.
>
> This removes the ambiguity on the meaning of the IMA measurement list.
>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> Documentation/ABI/testing/ima_policy | 5 +-
> security/integrity/ima/ima.h | 10 +++-
> security/integrity/ima/ima_api.c | 6 ++-
> security/integrity/ima/ima_appraise.c | 2 +-
> security/integrity/ima/ima_main.c | 8 +--
> security/integrity/ima/ima_policy.c | 70 ++++++++++++++++++++++++++-
> 6 files changed, 89 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index 22237fec5532..be045fb60530 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -29,7 +29,7 @@ Description:
> [obj_user=] [obj_role=] [obj_type=]]
> option: [digest_type=] [template=] [permit_directio]
> [appraise_type=] [appraise_flag=]
> - [appraise_algos=] [keyrings=]
> + [appraise_algos=] [keyrings=] [digest_cache=]
> base:
> func:=
> [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
> @@ -77,6 +77,9 @@ Description:
> For example, "sha256,sha512" to only accept to appraise
> files where the security.ima xattr was hashed with one
> of these two algorithms.
> + digest_cache:= [content]
> + "content" means that the digest cache is used only
> + for file content measurement and/or appraisal.
>
> default policy:
> # PROC_SUPER_MAGIC
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index c9140a57b591..deee56d99d6f 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -43,6 +43,10 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10
> };
>
> #define NR_BANKS(chip) ((chip != NULL) ? chip->nr_allocated_banks : 0)
>
> +/* Digest cache usage flags. */
> +#define IMA_DIGEST_CACHE_MEASURE_CONTENT 0x0000000000000001
> +#define IMA_DIGEST_CACHE_APPRAISE_CONTENT 0x0000000000000002
> +
> /* current content of the policy */
> extern int ima_policy_flag;
>
> @@ -367,7 +371,8 @@ int ima_get_action(struct mnt_idmap *idmap, struct inode
> *inode,
> const struct cred *cred, u32 secid, int mask,
> enum ima_hooks func, int *pcr,
> struct ima_template_desc **template_desc,
> - const char *func_data, unsigned int *allowed_algos);
> + const char *func_data, unsigned int *allowed_algos,
> + u64 *digest_cache_mask);
> int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
> int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
> void *buf, loff_t size, enum hash_algo algo,
> @@ -398,7 +403,8 @@ int ima_match_policy(struct mnt_idmap *idmap, struct inode
> *inode,
> const struct cred *cred, u32 secid, enum ima_hooks func,
> int mask, int flags, int *pcr,
> struct ima_template_desc **template_desc,
> - const char *func_data, unsigned int *allowed_algos);
> + const char *func_data, unsigned int *allowed_algos,
> + u64 *digest_cache_mask);
> void ima_init_policy(void);
> void ima_update_policy(void);
> void ima_update_policy_flags(void);
> diff --git a/security/integrity/ima/ima_api.c
> b/security/integrity/ima/ima_api.c
> index b37d043d5748..87e286ace43c 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -173,6 +173,7 @@ void ima_add_violation(struct file *file, const unsigned
> char *filename,
> * @template_desc: pointer filled in if matched measure policy sets template=
> * @func_data: func specific data, may be NULL
> * @allowed_algos: allowlist of hash algorithms for the IMA xattr
> + * @digest_cache_mask: Actions and purposes for which digest cache is allowed
> *
> * The policy is defined in terms of keypairs:
> * subj=, obj=, type=, func=, mask=, fsmagic=
> @@ -190,7 +191,8 @@ int ima_get_action(struct mnt_idmap *idmap, struct inode
> *inode,
> const struct cred *cred, u32 secid, int mask,
> enum ima_hooks func, int *pcr,
> struct ima_template_desc **template_desc,
> - const char *func_data, unsigned int *allowed_algos)
> + const char *func_data, unsigned int *allowed_algos,
> + u64 *digest_cache_mask)
> {
> int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
>
> @@ -198,7 +200,7 @@ int ima_get_action(struct mnt_idmap *idmap, struct inode
> *inode,
>
> return ima_match_policy(idmap, inode, cred, secid, func, mask,
> flags, pcr, template_desc, func_data,
> - allowed_algos);
> + allowed_algos, digest_cache_mask);
> }
>
> static bool ima_get_verity_digest(struct ima_iint_cache *iint,
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 3497741caea9..27ccc9a2c09f 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -81,7 +81,7 @@ int ima_must_appraise(struct mnt_idmap *idmap, struct inode
> *inode,
> security_current_getsecid_subj(&secid);
> return ima_match_policy(idmap, inode, current_cred(), secid,
> func, mask, IMA_APPRAISE | IMA_HASH, NULL,
> - NULL, NULL, NULL);
> + NULL, NULL, NULL, NULL);
> }
>
> static int ima_fix_xattr(struct dentry *dentry, struct ima_iint_cache *iint)
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 18285fc8ac07..e3ca80098c4c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -232,7 +232,7 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
> */
> action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
> mask, func, &pcr, &template_desc, NULL,
> - &allowed_algos);
> + &allowed_algos, NULL);
> violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
> func == MMAP_CHECK_REQPROT) &&
> (ima_policy_flag & IMA_MEASURE));
> @@ -489,11 +489,11 @@ static int ima_file_mprotect(struct vm_area_struct *vma,
> unsigned long reqprot,
> inode = file_inode(vma->vm_file);
> action = ima_get_action(file_mnt_idmap(vma->vm_file), inode,
> current_cred(), secid, MAY_EXEC, MMAP_CHECK,
> - &pcr, &template, NULL, NULL);
> + &pcr, &template, NULL, NULL, NULL);
> action |= ima_get_action(file_mnt_idmap(vma->vm_file), inode,
> current_cred(), secid, MAY_EXEC,
> MMAP_CHECK_REQPROT, &pcr, &template, NULL,
> - NULL);
> + NULL, NULL);
>
> /* Is the mmap'ed file in policy? */
> if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK)))
> @@ -972,7 +972,7 @@ int process_buffer_measurement(struct mnt_idmap *idmap,
> security_current_getsecid_subj(&secid);
> action = ima_get_action(idmap, inode, current_cred(),
> secid, 0, func, &pcr, &template,
> - func_data, NULL);
> + func_data, NULL, NULL);
> if (!(action & IMA_MEASURE) && !digest)
> return -ENOENT;
> }
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 7cfd1860791f..4ac83df8d255 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -122,6 +122,7 @@ struct ima_rule_entry {
> struct ima_rule_opt_list *keyrings; /* Measure keys added to these
> keyrings */
> struct ima_rule_opt_list *label; /* Measure data grouped under this
> label */
> struct ima_template_desc *template;
> + u64 digest_cache_mask; /* Actions and purposes for which digest cache
> is allowed */
> };
>
> /*
> @@ -726,6 +727,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum
> ima_hooks func)
> * @template_desc: the template that should be used for this rule
> * @func_data: func specific data, may be NULL
> * @allowed_algos: allowlist of hash algorithms for the IMA xattr
> + * @digest_cache_mask: Actions and purposes for which digest cache is allowed
Purpose
> *
> * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
> * conditions.
> @@ -738,7 +740,8 @@ int ima_match_policy(struct mnt_idmap *idmap, struct inode
> *inode,
> const struct cred *cred, u32 secid, enum ima_hooks func,
> int mask, int flags, int *pcr,
> struct ima_template_desc **template_desc,
> - const char *func_data, unsigned int *allowed_algos)
> + const char *func_data, unsigned int *allowed_algos,
> + u64 *digest_cache_mask)
> {
> struct ima_rule_entry *entry;
> int action = 0, actmask = flags | (flags << 1);
> @@ -783,6 +786,9 @@ int ima_match_policy(struct mnt_idmap *idmap, struct inode
> *inode,
> if (template_desc && entry->template)
> *template_desc = entry->template;
>
> + if (digest_cache_mask)
> + *digest_cache_mask |= entry->digest_cache_mask;
> +
> if (!actmask)
> break;
> }
> @@ -859,6 +865,30 @@ static int ima_appraise_flag(enum ima_hooks func)
> return 0;
> }
>
> +static bool ima_digest_cache_func_allowed(struct ima_rule_entry *entry)
> +{
> + switch (entry->func) {
> + case NONE:
> + case FILE_CHECK:
> + case MMAP_CHECK:
> + case MMAP_CHECK_REQPROT:
> + case BPRM_CHECK:
> + case CREDS_CHECK:
> + case FIRMWARE_CHECK:
> + case POLICY_CHECK:
> + case MODULE_CHECK:
> + case KEXEC_KERNEL_CHECK:
> + case KEXEC_INITRAMFS_CHECK:
> + /* Exception: always add policy updates to measurement list! */
> + if (entry->action == MEASURE && entry->func == POLICY_CHECK)
> + return false;
> +
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> static void add_rules(struct ima_rule_entry *entries, int count,
> enum policy_rule_list policy_rule)
> {
> @@ -1073,7 +1103,7 @@ enum policy_opt {
> Opt_digest_type,
> Opt_appraise_type, Opt_appraise_flag, Opt_appraise_algos,
> Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
> - Opt_label, Opt_err
> + Opt_label, Opt_digest_cache, Opt_err
> };
>
> static const match_table_t policy_tokens = {
> @@ -1122,6 +1152,7 @@ static const match_table_t policy_tokens = {
> {Opt_template, "template=%s"},
> {Opt_keyrings, "keyrings=%s"},
> {Opt_label, "label=%s"},
> + {Opt_digest_cache, "digest_cache=%s"},
> {Opt_err, NULL}
> };
>
> @@ -1245,6 +1276,18 @@ static bool ima_validate_rule(struct ima_rule_entry
> *entry)
> if (entry->action != MEASURE && entry->flags & IMA_PCR)
> return false;
>
> + /* New-style measurements with digest cache cannot be on default PCR. */
> + if (entry->action == MEASURE &&
> + (entry->digest_cache_mask & IMA_DIGEST_CACHE_MEASURE_CONTENT)) {
> + if (!(entry->flags & IMA_PCR) ||
> + entry->pcr == CONFIG_IMA_MEASURE_PCR_IDX)
> + return false;
> + }
> +
> + /* Digest caches can be used only for a subset of the IMA hooks. */
> + if (entry->digest_cache_mask && !ima_digest_cache_func_allowed(entry))
> + return false;
> +
> if (entry->action != APPRAISE &&
> entry->flags & (IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED |
> IMA_CHECK_BLACKLIST | IMA_VALIDATE_ALGOS))
> @@ -1881,6 +1924,26 @@ static int ima_parse_rule(char *rule, struct
> ima_rule_entry *entry)
> &(template_desc->num_fields));
> entry->template = template_desc;
> break;
> + case Opt_digest_cache:
> + ima_log_string(ab, "digest_cache", args[0].from);
> +
> + result = -EINVAL;
> +
> + if (!strcmp(args[0].from, "content")) {
> + switch (entry->action) {
> + case MEASURE:
> + entry->digest_cache_mask |=
> IMA_DIGEST_CACHE_MEASURE_CONTENT;
> + result = 0;
> + break;
> + case APPRAISE:
> + entry->digest_cache_mask |=
> IMA_DIGEST_CACHE_APPRAISE_CONTENT;
> + result = 0;
> + break;
> + default:
> + break;
> + }
> + }
> + break;
> case Opt_err:
> ima_log_string(ab, "UNKNOWN", p);
> result = -EINVAL;
> @@ -2271,6 +2334,9 @@ int ima_policy_show(struct seq_file *m, void *v)
> seq_puts(m, "digest_type=verity ");
> if (entry->flags & IMA_PERMIT_DIRECTIO)
> seq_puts(m, "permit_directio ");
> + if ((entry->digest_cache_mask & IMA_DIGEST_CACHE_MEASURE_CONTENT) ||
> + (entry->digest_cache_mask & IMA_DIGEST_CACHE_APPRAISE_CONTENT))
> + seq_puts(m, "digest_cache=content ");
> rcu_read_unlock();
> seq_puts(m, "\n");
> return 0;
On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> Invoking digest_cache_get() inside the iint->mutex critical region can
> cause deadlocks due to the fact that IMA can be recursively invoked for
> reading the digest list. The deadlock would occur if the digest_cache LSM
> attempts to read the same inode that is already locked by IMA.
>
> However, since the digest_cache LSM makes sure that the above situation
> never happens, as it checks the inodes, it is safe to call
> digest_cache_get() inside the critical region and nest the iint->mutex
> when the DIGEST_LIST_CHECK hook is executed.
>
> Add a lockdep subclass to the iint->mutex, that is 0 if the IMA hook
> executed is not DIGEST_LIST_CHECK, and 1 when it is. Since lockdep allows
> nesting with higher classes and subclasses, that effectively eliminates the
> warning about the unsafe lock.
>
> Pass the new lockdep subclass (nested variable) from ima_inode_get() to
> ima_iint_init_always() and ima_iint_lockdep_annotate().
>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> security/integrity/ima/ima.h | 2 +-
> security/integrity/ima/ima_iint.c | 11 ++++++-----
> security/integrity/ima/ima_main.c | 6 +++---
> 3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index cea4517e73ab..c9140a57b591 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -216,7 +216,7 @@ static inline void ima_inode_set_iint(const struct inode
> *inode,
> }
>
> struct ima_iint_cache *ima_iint_find(struct inode *inode);
> -struct ima_iint_cache *ima_inode_get(struct inode *inode);
> +struct ima_iint_cache *ima_inode_get(struct inode *inode, bool nested);
> void ima_inode_free(struct inode *inode);
> void __init ima_iintcache_init(void);
>
> diff --git a/security/integrity/ima/ima_iint.c
> b/security/integrity/ima/ima_iint.c
> index e7c9c216c1c6..b4f476fae437 100644
> --- a/security/integrity/ima/ima_iint.c
> +++ b/security/integrity/ima/ima_iint.c
> @@ -41,7 +41,7 @@ struct ima_iint_cache *ima_iint_find(struct inode *inode)
> * See ovl_lockdep_annotate_inode_mutex_key() for more details.
> */
> static inline void ima_iint_lockdep_annotate(struct ima_iint_cache *iint,
> - struct inode *inode)
> + struct inode *inode, bool nested)
> {
> #ifdef CONFIG_LOCKDEP
> static struct lock_class_key ima_iint_mutex_key[IMA_MAX_NESTING];
"nested" is being pushed all the way down to here, perhaps I'm missing
something, but I don't see it being used in any of the patches.
Mimi
> @@ -56,7 +56,7 @@ static inline void ima_iint_lockdep_annotate(struct
> ima_iint_cache *iint,
> }
>
> static void ima_iint_init_always(struct ima_iint_cache *iint,
> - struct inode *inode)
> + struct inode *inode, bool nested)
> {
> iint->ima_hash = NULL;
> iint->version = 0;
> @@ -69,7 +69,7 @@ static void ima_iint_init_always(struct ima_iint_cache
> *iint,
> iint->ima_creds_status = INTEGRITY_UNKNOWN;
> iint->measured_pcrs = 0;
> mutex_init(&iint->mutex);
> - ima_iint_lockdep_annotate(iint, inode);
> + ima_iint_lockdep_annotate(iint, inode, nested);
> }
>
> static void ima_iint_free(struct ima_iint_cache *iint)
> @@ -82,13 +82,14 @@ static void ima_iint_free(struct ima_iint_cache *iint)
> /**
> * ima_inode_get - Find or allocate an iint associated with an inode
> * @inode: Pointer to the inode
> + * @nested: Whether or not the iint->mutex lock can be nested
> *
> * Find an iint associated with an inode, and allocate a new one if not
> found.
> * Caller must lock i_mutex.
> *
> * Return: An iint on success, NULL on error.
> */
> -struct ima_iint_cache *ima_inode_get(struct inode *inode)
> +struct ima_iint_cache *ima_inode_get(struct inode *inode, bool nested)
> {
> struct ima_iint_cache *iint;
>
> @@ -100,7 +101,7 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
> if (!iint)
> return NULL;
>
> - ima_iint_init_always(iint, inode);
> + ima_iint_init_always(iint, inode, nested);
>
> inode->i_flags |= S_IMA;
> ima_inode_set_iint(inode, iint);
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 780627b0cde7..18285fc8ac07 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -248,7 +248,7 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
> inode_lock(inode);
>
> if (action) {
> - iint = ima_inode_get(inode);
> + iint = ima_inode_get(inode, func == DIGEST_LIST_CHECK);
> if (!iint)
> rc = -ENOMEM;
> }
> @@ -699,7 +699,7 @@ static void ima_post_create_tmpfile(struct mnt_idmap
> *idmap,
> return;
>
> /* Nothing to do if we can't allocate memory */
> - iint = ima_inode_get(inode);
> + iint = ima_inode_get(inode, false);
> if (!iint)
> return;
>
> @@ -731,7 +731,7 @@ static void ima_post_path_mknod(struct mnt_idmap *idmap,
> struct dentry *dentry)
> return;
>
> /* Nothing to do if we can't allocate memory */
> - iint = ima_inode_get(inode);
> + iint = ima_inode_get(inode, false);
> if (!iint)
> return;
>
On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> Specify the 'digest_cache_measure' boot-time policy with 'ima_policy=' in
> the kernel command line
The 'built-in' policies may be specified on the boot command line. Please
update Subject line, to user the term "built-in" as well as here.
> to add the following rule at the beginning of the
> IMA policy, before other rules:
Comments below...
>
> measure func=DIGEST_LIST_CHECK pcr=12
>
> which will measure digest lists into PCR 12 (or the value in
> CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX).
>
> 'digest_cache_measure' also adds 'digest_cache=content pcr=12' to the other
> measure rules, if they have a compatible IMA hook. The PCR value still
> comes from CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX.
>
> Specify 'digest_cache_appraise' to add the following rule at the beginning,
> before other rules:
>
> appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig
>
> which will appraise digest lists with IMA signatures or module-style
> appended signatures.
>
> 'digest_cache_appraise' also adds 'digest_cache=content' to the other
> appraise rules, if they have a compatible IMA hook.
>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> .../admin-guide/kernel-parameters.txt | 15 ++++++-
> security/integrity/ima/Kconfig | 10 +++++
> security/integrity/ima/ima_policy.c | 45 +++++++++++++++++++
> 3 files changed, 69 insertions(+), 1 deletion(-)
[...]
> @@ -971,6 +1006,16 @@ void __init ima_init_policy(void)
> {
> int build_appraise_entries, arch_entries;
>
> + /*
> + * We need to load digest cache rules at the beginning, to avoid dont_
> + * rules causing ours to not be reached.
> + */
"lockdown" trusts IMA to measure and appraise kernel modules, if the rule
exists. Placing the digest_cache first breaks this trust.
From a trusted and secure boot perspective, the architecture specific policy
rules should not be ignored. Putting the digest_cache before any other rules
will limit others from being able to use digest_cache.
Instead of putting the digest_cache_{measure,appraise} built-in policies first,
skip loading the dont_measure_rules.
Mimi
> + if (ima_digest_cache_measure)
> + add_rules(&measure_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
> +
> + if (ima_digest_cache_appraise)
> + add_rules(&appraise_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
> +
> /* if !ima_policy, we load NO default rules */
> if (ima_policy)
> add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules),
On Thu, 2024-03-07 at 14:42 -0500, Mimi Zohar wrote:
> On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <[email protected]>
> >
> > Invoking digest_cache_get() inside the iint->mutex critical region can
> > cause deadlocks due to the fact that IMA can be recursively invoked for
> > reading the digest list. The deadlock would occur if the digest_cache LSM
> > attempts to read the same inode that is already locked by IMA.
> >
> > However, since the digest_cache LSM makes sure that the above situation
> > never happens, as it checks the inodes, it is safe to call
> > digest_cache_get() inside the critical region and nest the iint->mutex
> > when the DIGEST_LIST_CHECK hook is executed.
> >
> > Add a lockdep subclass to the iint->mutex, that is 0 if the IMA hook
> > executed is not DIGEST_LIST_CHECK, and 1 when it is. Since lockdep allows
> > nesting with higher classes and subclasses, that effectively eliminates the
> > warning about the unsafe lock.
> >
> > Pass the new lockdep subclass (nested variable) from ima_inode_get() to
> > ima_iint_init_always() and ima_iint_lockdep_annotate().
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
> > ---
> > security/integrity/ima/ima.h | 2 +-
> > security/integrity/ima/ima_iint.c | 11 ++++++-----
> > security/integrity/ima/ima_main.c | 6 +++---
> > 3 files changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index cea4517e73ab..c9140a57b591 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -216,7 +216,7 @@ static inline void ima_inode_set_iint(const struct inode
> > *inode,
> > }
> >
> > struct ima_iint_cache *ima_iint_find(struct inode *inode);
> > -struct ima_iint_cache *ima_inode_get(struct inode *inode);
> > +struct ima_iint_cache *ima_inode_get(struct inode *inode, bool nested);
> > void ima_inode_free(struct inode *inode);
> > void __init ima_iintcache_init(void);
> >
> > diff --git a/security/integrity/ima/ima_iint.c
> > b/security/integrity/ima/ima_iint.c
> > index e7c9c216c1c6..b4f476fae437 100644
> > --- a/security/integrity/ima/ima_iint.c
> > +++ b/security/integrity/ima/ima_iint.c
> > @@ -41,7 +41,7 @@ struct ima_iint_cache *ima_iint_find(struct inode *inode)
> > * See ovl_lockdep_annotate_inode_mutex_key() for more details.
> > */
> > static inline void ima_iint_lockdep_annotate(struct ima_iint_cache *iint,
> > - struct inode *inode)
> > + struct inode *inode, bool nested)
> > {
> > #ifdef CONFIG_LOCKDEP
> > static struct lock_class_key ima_iint_mutex_key[IMA_MAX_NESTING];
>
>
> "nested" is being pushed all the way down to here, perhaps I'm missing
> something, but I don't see it being used in any of the patches.
Must have gone away during a conflict resolution...
That should have been:
@@ -85,12 +85,13 @@ static inline void iint_lockdep_annotate(struct integrity_iint_cache *iint,
if (WARN_ON_ONCE(depth < 0 || depth >= IMA_MAX_NESTING))
depth = 0;
- lockdep_set_class(&iint->mutex, &iint_mutex_key[depth]);
+ lockdep_set_class_and_subclass(&iint->mutex, &iint_mutex_key[depth],
+ nested);
#endif
}
static void iint_init_always(struct integrity_iint_cache *iint,
- struct inode *inode)
+ struct inode *inode, bool nested)
{
iint->ima_hash = NULL;
iint->version = 0;
Thanks
Roberto
> Mimi
>
> > @@ -56,7 +56,7 @@ static inline void ima_iint_lockdep_annotate(struct
> > ima_iint_cache *iint,
> > }
> >
> > static void ima_iint_init_always(struct ima_iint_cache *iint,
> > - struct inode *inode)
> > + struct inode *inode, bool nested)
> > {
> > iint->ima_hash = NULL;
> > iint->version = 0;
> > @@ -69,7 +69,7 @@ static void ima_iint_init_always(struct ima_iint_cache
> > *iint,
> > iint->ima_creds_status = INTEGRITY_UNKNOWN;
> > iint->measured_pcrs = 0;
> > mutex_init(&iint->mutex);
> > - ima_iint_lockdep_annotate(iint, inode);
> > + ima_iint_lockdep_annotate(iint, inode, nested);
> > }
> >
> > static void ima_iint_free(struct ima_iint_cache *iint)
> > @@ -82,13 +82,14 @@ static void ima_iint_free(struct ima_iint_cache *iint)
> > /**
> > * ima_inode_get - Find or allocate an iint associated with an inode
> > * @inode: Pointer to the inode
> > + * @nested: Whether or not the iint->mutex lock can be nested
> > *
> > * Find an iint associated with an inode, and allocate a new one if not
> > found.
> > * Caller must lock i_mutex.
> > *
> > * Return: An iint on success, NULL on error.
> > */
> > -struct ima_iint_cache *ima_inode_get(struct inode *inode)
> > +struct ima_iint_cache *ima_inode_get(struct inode *inode, bool nested)
> > {
> > struct ima_iint_cache *iint;
> >
> > @@ -100,7 +101,7 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
> > if (!iint)
> > return NULL;
> >
> > - ima_iint_init_always(iint, inode);
> > + ima_iint_init_always(iint, inode, nested);
> >
> > inode->i_flags |= S_IMA;
> > ima_inode_set_iint(inode, iint);
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index 780627b0cde7..18285fc8ac07 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -248,7 +248,7 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> > inode_lock(inode);
> >
> > if (action) {
> > - iint = ima_inode_get(inode);
> > + iint = ima_inode_get(inode, func == DIGEST_LIST_CHECK);
> > if (!iint)
> > rc = -ENOMEM;
> > }
> > @@ -699,7 +699,7 @@ static void ima_post_create_tmpfile(struct mnt_idmap
> > *idmap,
> > return;
> >
> > /* Nothing to do if we can't allocate memory */
> > - iint = ima_inode_get(inode);
> > + iint = ima_inode_get(inode, false);
> > if (!iint)
> > return;
> >
> > @@ -731,7 +731,7 @@ static void ima_post_path_mknod(struct mnt_idmap *idmap,
> > struct dentry *dentry)
> > return;
> >
> > /* Nothing to do if we can't allocate memory */
> > - iint = ima_inode_get(inode);
> > + iint = ima_inode_get(inode, false);
> > if (!iint)
> > return;
> >
On Thu, 2024-03-07 at 14:43 -0500, Mimi Zohar wrote:
> On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <[email protected]>
> >
> > Add the 'digest_cache=' policy keyword, to enable the usage of digest
> > caches for specific IMA actions and purposes.
> >
> > At the moment, it accepts only 'content' as value, as digest caches can be
> > only used only for measurement and appraisal of file content. In the
> > future, it might be possible to use them for file metadata too.
>
> At least from this patch, it is unclear why 'digest_cache' requires an option.
> The usage - measure, appraise - is based on 'action'. From an IMA perspective,
> does the file content make a difference? And if it did, then file 'data' would
> parallel file 'metadata'.
I wanted to express the fact that digest caches, if available, can only
be used to appraise file data, if there is no metadata (similarly to
module-style appended signatures).
That would prevent for example the scenario where appraisal of file
data is successful without having verified current metadata, and EVM
attaches to the file a valid HMAC on file close, based on the current
xattr value (trust at first use).
An IMA rule with 'digest_cache=metadata' would take a different code
path. It would make IMA send to evm_verifyxattr() the calculated file
digest (since there is no security.ima), and let EVM calculate and
search the digest of file metadata in the digest cache.
I didn't go that far yet, but this is more or less what I would like to
do, also based on my old implementation of the IMA Digest Lists
extension (which supports file metadata lookup).
> Without having to pass around "digest_cache_mask" would simplify this patch.
We need to pass it anyway, to let process_measurement() know that it
can use digest caches. Or we can make 'flags' in ima_iint_cache a u64,
and introduce new flags.
Roberto
> Mimi
>
> > The 'digest_cache=' keyword can be specified for the subset of IMA hooks
> > listed in ima_digest_cache_func_allowed().
> >
> > POLICY_CHECK has been excluded for measurement, because policy changes must
> > be visible in the IMA measurement list. For appraisal, instead, it might be
> > useful to load custom policies in the initial ram disk (no security.ima
> > xattr).
> >
> > Add the digest_cache_mask member to the ima_rule_entry structure, and set
> > the flag IMA_DIGEST_CACHE_MEASURE_CONTENT if 'digest_cache=content' was
> > specified for a measure rule, IMA_DIGEST_CACHE_APPRAISE_CONTENT for an
> > appraise rule.
> >
> > Propagate the mask down to ima_match_policy() and ima_get_action(), so that
> > process_measurement() can make the final decision on whether or not digest
> > caches should be used to measure/appraise the file being evaluated.
> >
> > Since using digest caches changes the meaning of the IMA measurement list,
> > which will include only digest lists and unknown files, enforce specifying
> > 'pcr=' with a non-standard value, when 'digest_cache=content' is specified
> > in a measure rule.
> >
> > This removes the ambiguity on the meaning of the IMA measurement list.
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
> > ---
> > Documentation/ABI/testing/ima_policy | 5 +-
> > security/integrity/ima/ima.h | 10 +++-
> > security/integrity/ima/ima_api.c | 6 ++-
> > security/integrity/ima/ima_appraise.c | 2 +-
> > security/integrity/ima/ima_main.c | 8 +--
> > security/integrity/ima/ima_policy.c | 70 ++++++++++++++++++++++++++-
> > 6 files changed, 89 insertions(+), 12 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/ima_policy
> > b/Documentation/ABI/testing/ima_policy
> > index 22237fec5532..be045fb60530 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -29,7 +29,7 @@ Description:
> > [obj_user=] [obj_role=] [obj_type=]]
> > option: [digest_type=] [template=] [permit_directio]
> > [appraise_type=] [appraise_flag=]
> > - [appraise_algos=] [keyrings=]
> > + [appraise_algos=] [keyrings=] [digest_cache=]
> > base:
> > func:=
> > [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> > [FIRMWARE_CHECK]
> > @@ -77,6 +77,9 @@ Description:
> > For example, "sha256,sha512" to only accept to appraise
> > files where the security.ima xattr was hashed with one
> > of these two algorithms.
> > + digest_cache:= [content]
> > + "content" means that the digest cache is used only
> > + for file content measurement and/or appraisal.
> >
> > default policy:
> > # PROC_SUPER_MAGIC
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index c9140a57b591..deee56d99d6f 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -43,6 +43,10 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10
> > };
> >
> > #define NR_BANKS(chip) ((chip != NULL) ? chip->nr_allocated_banks : 0)
> >
> > +/* Digest cache usage flags. */
> > +#define IMA_DIGEST_CACHE_MEASURE_CONTENT 0x0000000000000001
> > +#define IMA_DIGEST_CACHE_APPRAISE_CONTENT 0x0000000000000002
> > +
> > /* current content of the policy */
> > extern int ima_policy_flag;
> >
> > @@ -367,7 +371,8 @@ int ima_get_action(struct mnt_idmap *idmap, struct inode
> > *inode,
> > const struct cred *cred, u32 secid, int mask,
> > enum ima_hooks func, int *pcr,
> > struct ima_template_desc **template_desc,
> > - const char *func_data, unsigned int *allowed_algos);
> > + const char *func_data, unsigned int *allowed_algos,
> > + u64 *digest_cache_mask);
> > int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
> > int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
> > void *buf, loff_t size, enum hash_algo algo,
> > @@ -398,7 +403,8 @@ int ima_match_policy(struct mnt_idmap *idmap, struct inode
> > *inode,
> > const struct cred *cred, u32 secid, enum ima_hooks func,
> > int mask, int flags, int *pcr,
> > struct ima_template_desc **template_desc,
> > - const char *func_data, unsigned int *allowed_algos);
> > + const char *func_data, unsigned int *allowed_algos,
> > + u64 *digest_cache_mask);
> > void ima_init_policy(void);
> > void ima_update_policy(void);
> > void ima_update_policy_flags(void);
> > diff --git a/security/integrity/ima/ima_api.c
> > b/security/integrity/ima/ima_api.c
> > index b37d043d5748..87e286ace43c 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -173,6 +173,7 @@ void ima_add_violation(struct file *file, const unsigned
> > char *filename,
> > * @template_desc: pointer filled in if matched measure policy sets template=
> > * @func_data: func specific data, may be NULL
> > * @allowed_algos: allowlist of hash algorithms for the IMA xattr
> > + * @digest_cache_mask: Actions and purposes for which digest cache is allowed
> > *
> > * The policy is defined in terms of keypairs:
> > * subj=, obj=, type=, func=, mask=, fsmagic=
> > @@ -190,7 +191,8 @@ int ima_get_action(struct mnt_idmap *idmap, struct inode
> > *inode,
> > const struct cred *cred, u32 secid, int mask,
> > enum ima_hooks func, int *pcr,
> > struct ima_template_desc **template_desc,
> > - const char *func_data, unsigned int *allowed_algos)
> > + const char *func_data, unsigned int *allowed_algos,
> > + u64 *digest_cache_mask)
> > {
> > int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
> >
> > @@ -198,7 +200,7 @@ int ima_get_action(struct mnt_idmap *idmap, struct inode
> > *inode,
> >
> > return ima_match_policy(idmap, inode, cred, secid, func, mask,
> > flags, pcr, template_desc, func_data,
> > - allowed_algos);
> > + allowed_algos, digest_cache_mask);
> > }
> >
> > static bool ima_get_verity_digest(struct ima_iint_cache *iint,
> > diff --git a/security/integrity/ima/ima_appraise.c
> > b/security/integrity/ima/ima_appraise.c
> > index 3497741caea9..27ccc9a2c09f 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -81,7 +81,7 @@ int ima_must_appraise(struct mnt_idmap *idmap, struct inode
> > *inode,
> > security_current_getsecid_subj(&secid);
> > return ima_match_policy(idmap, inode, current_cred(), secid,
> > func, mask, IMA_APPRAISE | IMA_HASH, NULL,
> > - NULL, NULL, NULL);
> > + NULL, NULL, NULL, NULL);
> > }
> >
> > static int ima_fix_xattr(struct dentry *dentry, struct ima_iint_cache *iint)
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index 18285fc8ac07..e3ca80098c4c 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -232,7 +232,7 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> > */
> > action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
> > mask, func, &pcr, &template_desc, NULL,
> > - &allowed_algos);
> > + &allowed_algos, NULL);
> > violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
> > func == MMAP_CHECK_REQPROT) &&
> > (ima_policy_flag & IMA_MEASURE));
> > @@ -489,11 +489,11 @@ static int ima_file_mprotect(struct vm_area_struct *vma,
> > unsigned long reqprot,
> > inode = file_inode(vma->vm_file);
> > action = ima_get_action(file_mnt_idmap(vma->vm_file), inode,
> > current_cred(), secid, MAY_EXEC, MMAP_CHECK,
> > - &pcr, &template, NULL, NULL);
> > + &pcr, &template, NULL, NULL, NULL);
> > action |= ima_get_action(file_mnt_idmap(vma->vm_file), inode,
> > current_cred(), secid, MAY_EXEC,
> > MMAP_CHECK_REQPROT, &pcr, &template, NULL,
> > - NULL);
> > + NULL, NULL);
> >
> > /* Is the mmap'ed file in policy? */
> > if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK)))
> > @@ -972,7 +972,7 @@ int process_buffer_measurement(struct mnt_idmap *idmap,
> > security_current_getsecid_subj(&secid);
> > action = ima_get_action(idmap, inode, current_cred(),
> > secid, 0, func, &pcr, &template,
> > - func_data, NULL);
> > + func_data, NULL, NULL);
> > if (!(action & IMA_MEASURE) && !digest)
> > return -ENOENT;
> > }
> > diff --git a/security/integrity/ima/ima_policy.c
> > b/security/integrity/ima/ima_policy.c
> > index 7cfd1860791f..4ac83df8d255 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -122,6 +122,7 @@ struct ima_rule_entry {
> > struct ima_rule_opt_list *keyrings; /* Measure keys added to these
> > keyrings */
> > struct ima_rule_opt_list *label; /* Measure data grouped under this
> > label */
> > struct ima_template_desc *template;
> > + u64 digest_cache_mask; /* Actions and purposes for which digest cache
> > is allowed */
> > };
> >
> > /*
> > @@ -726,6 +727,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum
> > ima_hooks func)
> > * @template_desc: the template that should be used for this rule
> > * @func_data: func specific data, may be NULL
> > * @allowed_algos: allowlist of hash algorithms for the IMA xattr
> > + * @digest_cache_mask: Actions and purposes for which digest cache is allowed
>
> Purpose
> > *
> > * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
> > * conditions.
> > @@ -738,7 +740,8 @@ int ima_match_policy(struct mnt_idmap *idmap, struct inode
> > *inode,
> > const struct cred *cred, u32 secid, enum ima_hooks func,
> > int mask, int flags, int *pcr,
> > struct ima_template_desc **template_desc,
> > - const char *func_data, unsigned int *allowed_algos)
> > + const char *func_data, unsigned int *allowed_algos,
> > + u64 *digest_cache_mask)
> > {
> > struct ima_rule_entry *entry;
> > int action = 0, actmask = flags | (flags << 1);
> > @@ -783,6 +786,9 @@ int ima_match_policy(struct mnt_idmap *idmap, struct inode
> > *inode,
> > if (template_desc && entry->template)
> > *template_desc = entry->template;
> >
> > + if (digest_cache_mask)
> > + *digest_cache_mask |= entry->digest_cache_mask;
> > +
> > if (!actmask)
> > break;
> > }
> > @@ -859,6 +865,30 @@ static int ima_appraise_flag(enum ima_hooks func)
> > return 0;
> > }
> >
> > +static bool ima_digest_cache_func_allowed(struct ima_rule_entry *entry)
> > +{
> > + switch (entry->func) {
> > + case NONE:
> > + case FILE_CHECK:
> > + case MMAP_CHECK:
> > + case MMAP_CHECK_REQPROT:
> > + case BPRM_CHECK:
> > + case CREDS_CHECK:
> > + case FIRMWARE_CHECK:
> > + case POLICY_CHECK:
> > + case MODULE_CHECK:
> > + case KEXEC_KERNEL_CHECK:
> > + case KEXEC_INITRAMFS_CHECK:
> > + /* Exception: always add policy updates to measurement list! */
> > + if (entry->action == MEASURE && entry->func == POLICY_CHECK)
> > + return false;
> > +
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > static void add_rules(struct ima_rule_entry *entries, int count,
> > enum policy_rule_list policy_rule)
> > {
> > @@ -1073,7 +1103,7 @@ enum policy_opt {
> > Opt_digest_type,
> > Opt_appraise_type, Opt_appraise_flag, Opt_appraise_algos,
> > Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
> > - Opt_label, Opt_err
> > + Opt_label, Opt_digest_cache, Opt_err
> > };
> >
> > static const match_table_t policy_tokens = {
> > @@ -1122,6 +1152,7 @@ static const match_table_t policy_tokens = {
> > {Opt_template, "template=%s"},
> > {Opt_keyrings, "keyrings=%s"},
> > {Opt_label, "label=%s"},
> > + {Opt_digest_cache, "digest_cache=%s"},
> > {Opt_err, NULL}
> > };
> >
> > @@ -1245,6 +1276,18 @@ static bool ima_validate_rule(struct ima_rule_entry
> > *entry)
> > if (entry->action != MEASURE && entry->flags & IMA_PCR)
> > return false;
> >
> > + /* New-style measurements with digest cache cannot be on default PCR. */
> > + if (entry->action == MEASURE &&
> > + (entry->digest_cache_mask & IMA_DIGEST_CACHE_MEASURE_CONTENT)) {
> > + if (!(entry->flags & IMA_PCR) ||
> > + entry->pcr == CONFIG_IMA_MEASURE_PCR_IDX)
> > + return false;
> > + }
> > +
> > + /* Digest caches can be used only for a subset of the IMA hooks. */
> > + if (entry->digest_cache_mask && !ima_digest_cache_func_allowed(entry))
> > + return false;
> > +
> > if (entry->action != APPRAISE &&
> > entry->flags & (IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED |
> > IMA_CHECK_BLACKLIST | IMA_VALIDATE_ALGOS))
> > @@ -1881,6 +1924,26 @@ static int ima_parse_rule(char *rule, struct
> > ima_rule_entry *entry)
> > &(template_desc->num_fields));
> > entry->template = template_desc;
> > break;
> > + case Opt_digest_cache:
> > + ima_log_string(ab, "digest_cache", args[0].from);
> > +
> > + result = -EINVAL;
> > +
> > + if (!strcmp(args[0].from, "content")) {
> > + switch (entry->action) {
> > + case MEASURE:
> > + entry->digest_cache_mask |=
> > IMA_DIGEST_CACHE_MEASURE_CONTENT;
> > + result = 0;
> > + break;
> > + case APPRAISE:
> > + entry->digest_cache_mask |=
> > IMA_DIGEST_CACHE_APPRAISE_CONTENT;
> > + result = 0;
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > + break;
> > case Opt_err:
> > ima_log_string(ab, "UNKNOWN", p);
> > result = -EINVAL;
> > @@ -2271,6 +2334,9 @@ int ima_policy_show(struct seq_file *m, void *v)
> > seq_puts(m, "digest_type=verity ");
> > if (entry->flags & IMA_PERMIT_DIRECTIO)
> > seq_puts(m, "permit_directio ");
> > + if ((entry->digest_cache_mask & IMA_DIGEST_CACHE_MEASURE_CONTENT) ||
> > + (entry->digest_cache_mask & IMA_DIGEST_CACHE_APPRAISE_CONTENT))
> > + seq_puts(m, "digest_cache=content ");
> > rcu_read_unlock();
> > seq_puts(m, "\n");
> > return 0;
>
On Thu, 2024-03-07 at 15:17 -0500, Mimi Zohar wrote:
> On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <[email protected]>
> >
> > Specify the 'digest_cache_measure' boot-time policy with 'ima_policy=' in
> > the kernel command line
>
> The 'built-in' policies may be specified on the boot command line. Please
> update Subject line, to user the term "built-in" as well as here.
Ok, will do.
> > to add the following rule at the beginning of the
> > IMA policy, before other rules:
>
> Comments below...
>
> >
> > measure func=DIGEST_LIST_CHECK pcr=12
> >
> > which will measure digest lists into PCR 12 (or the value in
> > CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX).
> >
> > 'digest_cache_measure' also adds 'digest_cache=content pcr=12' to the other
> > measure rules, if they have a compatible IMA hook. The PCR value still
> > comes from CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX.
> >
> > Specify 'digest_cache_appraise' to add the following rule at the beginning,
> > before other rules:
> >
> > appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig
> >
> > which will appraise digest lists with IMA signatures or module-style
> > appended signatures.
> >
> > 'digest_cache_appraise' also adds 'digest_cache=content' to the other
> > appraise rules, if they have a compatible IMA hook.
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
> > ---
> > .../admin-guide/kernel-parameters.txt | 15 ++++++-
> > security/integrity/ima/Kconfig | 10 +++++
> > security/integrity/ima/ima_policy.c | 45 +++++++++++++++++++
> > 3 files changed, 69 insertions(+), 1 deletion(-)
>
> [...]
>
> > @@ -971,6 +1006,16 @@ void __init ima_init_policy(void)
> > {
> > int build_appraise_entries, arch_entries;
> >
> > + /*
> > + * We need to load digest cache rules at the beginning, to avoid dont_
> > + * rules causing ours to not be reached.
> > + */
>
> "lockdown" trusts IMA to measure and appraise kernel modules, if the rule
> exists. Placing the digest_cache first breaks this trust.
The new rules don't prevent other rules to be reached, since they are
'do' and not 'don_t' rules.
If the kernel reads a file with file ID READING_MODULE, that would
still be matched by rules with 'func=MODULE_CHECK', even if there are
rules with 'func=DIGEST_LIST_CHECK', which will be instead matched when
there is a kernel read with file ID READING_DIGEST_LIST.
We can talk about the rule modification. Speaking of appraising kernel
modules, setting 'ima_policy=digest_cache_appraise' in the kernel
command line would have the effect of changing:
appraise func=MODULE_CHECK appraise_type=imasig|modsig
to:
appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig
appraise func=MODULE_CHECK appraise_type=imasig|modsig digest_cache=content
The effect of this would be that, if the kernel does not have
security.ima or an appended signature, appraisal will be still
successful by verifying the signature (in the xattr or appended) of the
digest list, and looking up the digest of the kernel module in that
digest list.
> From a trusted and secure boot perspective, the architecture specific policy
> rules should not be ignored.
I'm still missing how the architecture-specific policy would be
ignored.
> Putting the digest_cache before any other rules
> will limit others from being able to use digest_cache.
Sorry, didn't understand.
Let me just remark that measuring/appraising a digest list is a
necessary condition for using the digest cache built from that digest
list.
Not doing that has the same effect of a negative digest lookup, even if
that digest was in the digest list.
> Instead of putting the digest_cache_{measure,appraise} built-in policies first,
> skip loading the dont_measure_rules.
It does not seem a good idea. We still want to avoid
measurements/appraisal in the pseudo filesystems.
Roberto
> Mimi
>
> > + if (ima_digest_cache_measure)
> > + add_rules(&measure_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
> > +
> > + if (ima_digest_cache_appraise)
> > + add_rules(&appraise_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
> > +
> > /* if !ima_policy, we load NO default rules */
> > if (ima_policy)
> > add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules),
On Fri, 2024-03-08 at 10:05 +0100, Roberto Sassu wrote:
> On Thu, 2024-03-07 at 14:43 -0500, Mimi Zohar wrote:
> > On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <[email protected]>
> > >
> > > Add the 'digest_cache=' policy keyword, to enable the usage of digest
> > > caches for specific IMA actions and purposes.
> > >
> > > At the moment, it accepts only 'content' as value, as digest caches can be
> > > only used only for measurement and appraisal of file content. In the
> > > future, it might be possible to use them for file metadata too.
> >
> > At least from this patch, it is unclear why 'digest_cache' requires an
> > option.
> > The usage - measure, appraise - is based on 'action'. From an IMA
> > perspective,
> > does the file content make a difference? And if it did, then file 'data'
> > would
> > parallel file 'metadata'.
>
> I wanted to express the fact that digest caches, if available, can only
> be used to appraise file data, if there is no metadata (similarly to
> module-style appended signatures).
>
> That would prevent for example the scenario where appraisal of file
> data is successful without having verified current metadata, and EVM
> attaches to the file a valid HMAC on file close, based on the current
> xattr value (trust at first use).
Correct. There's no requirement for 'security.ima' to exist to calculate the EVM
HMAC. Before using EVM HMAC, the filesystem needs to be properly labeled by
walking the filesystem. The HMAC is calculated based on the existing file
metadata. The first use is during a setup stage and subsequently for new files.
>
> An IMA rule with 'digest_cache=metadata' would take a different code
> path. It would make IMA send to evm_verifyxattr() the calculated file
> digest (since there is no security.ima), and let EVM calculate and
> search the digest of file metadata in the digest cache.
Ok. So no 'security.evm' either, but a metadata digest cache.
I understand the need to provide EVM with the file hash in this case, but as
much as possible, IMA and EVM should be independent of each other.
>
> I didn't go that far yet, but this is more or less what I would like to
> do, also based on my old implementation of the IMA Digest Lists
> extension (which supports file metadata lookup).
>
> > Without having to pass around "digest_cache_mask" would simplify this patch.
>
> We need to pass it anyway, to let process_measurement() know that it
> can use digest caches. Or we can make 'flags' in ima_iint_cache a u64,
> and introduce new flags.
It's bad enough that the function parameters change when there's actual data.
Yes, please increase the size of 'flags' and introduce new flags.
thanks,
Mimi
> > > The 'digest_cache=' keyword can be specified for the subset of IMA hooks
> > > listed in ima_digest_cache_func_allowed().
> > >
> > > POLICY_CHECK has been excluded for measurement, because policy changes
> > > must
> > > be visible in the IMA measurement list. For appraisal, instead, it might
> > > be
> > > useful to load custom policies in the initial ram disk (no security.ima
> > > xattr).
> > >
> > > Add the digest_cache_mask member to the ima_rule_entry structure, and set
> > > the flag IMA_DIGEST_CACHE_MEASURE_CONTENT if 'digest_cache=content' was
> > > specified for a measure rule, IMA_DIGEST_CACHE_APPRAISE_CONTENT for an
> > > appraise rule.
> > >
> > > Propagate the mask down to ima_match_policy() and ima_get_action(), so
> > > that
> > > process_measurement() can make the final decision on whether or not digest
> > > caches should be used to measure/appraise the file being evaluated.
> > >
> > > Since using digest caches changes the meaning of the IMA measurement list,
> > > which will include only digest lists and unknown files, enforce specifying
> > > 'pcr=' with a non-standard value, when 'digest_cache=content' is specified
> > > in a measure rule.
> > >
> > > This removes the ambiguity on the meaning of the IMA measurement list.
> > >
> > > Signed-off-by: Roberto Sassu <[email protected]>
> > >
> > > @@ -971,6 +1006,16 @@ void __init ima_init_policy(void)
> > > {
> > > int build_appraise_entries, arch_entries;
> > >
> > > + /*
> > > + * We need to load digest cache rules at the beginning, to avoid dont_
> > > + * rules causing ours to not be reached.
> > > + */
> >
> > "lockdown" trusts IMA to measure and appraise kernel modules, if the rule
> > exists. Placing the digest_cache first breaks this trust.
>
> The new rules don't prevent other rules to be reached, since they are
> 'do' and not 'don_t' rules.
My mistake. These are just the rules for measuring or appraising the digest
cache lists themselves, not the actual policy rules for using the digest_cache.
This should be fine.
Perhaps update the comment to reflect initramfs usage.
thanks,
Mimi
Hi Roberto,
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 3fc48214850a..48a09747ae7a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -222,7 +222,9 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
> bool violation_check;
> enum hash_algo hash_algo;
> unsigned int allowed_algos = 0;
> - u64 verif_mask = 0;
> + u64 verif_mask = 0, *verif_mask_ptr, policy_mask = 0, allow_mask = 0;
> + struct digest_cache *digest_cache = NULL, *found_cache;
> + digest_cache_found_t found;
>
> if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> return 0;
> @@ -233,7 +235,7 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
> */
> action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
> mask, func, &pcr, &template_desc, NULL,
> - &allowed_algos, NULL);
> + &allowed_algos, &policy_mask);
> violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
> func == MMAP_CHECK_REQPROT) &&
> (ima_policy_flag & IMA_MEASURE));
> @@ -364,10 +366,34 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
> if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */
> pathname = ima_d_path(&file->f_path, &pathbuf, filename);
>
> + /*
> + * For now we don't support nested verification with digest caches.
I haven't reviewed the digest_cache LSM patch set yet. What does 'nested' mean
in this context? Why mention it here?
> + * Since we allow IMA policy rules without func=, we have to enforce
> + * this restriction here.
> + */
> + if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
> + digest_cache = digest_cache_get(file_dentry(file));
So whether or not a DIGEST_LIST_CHECK policy rule even exists,
digest_cache_get() will be called. Similarly, even if a digest_cache list
hasn't been measured or appraised, digest_cache_get() will be called.
Basically every file in policy will check the digest_cache.
> +
> + if (digest_cache) {
> + found = digest_cache_lookup(file_dentry(file), digest_cache,
> + iint->ima_hash->digest,
> + iint->ima_hash->algo);
> + /* AND what is allowed by the policy, and what IMA verified. */
> + if (found) {
> + found_cache = digest_cache_from_found_t(found);
> + verif_mask_ptr = digest_cache_verif_get(found_cache,
> + "ima");
Instead of using "verif_{set,get}' consider using '{set,get}_usage', where usage
here means measure or appraise.
> + if (verif_mask_ptr)
> + allow_mask = policy_mask & *verif_mask_ptr;
> + }
> +
> + digest_cache_put(digest_cache);
> + }
> +
I'm wondering if it makes sense to create IMA wrappers for each of the
digest_cache functions - checking the digest_cache for the hash, setting the
digest_cache permitted usage, etc - and put all of them in a separate
ima_digest_cache.c file. The file would only be included in the Makefile if
digest_cache is configured.
In this file you could define a static local global variable to detect whether
the digest_cache is ready to be used. Only after successfully measuring and
appraising a digest_cache list, based on policy, set the variable.
> if (action & IMA_MEASURE)
> ima_store_measurement(iint, file, pathname,
> xattr_value, xattr_len, modsig, pcr,
> - template_desc);
> + template_desc, allow_mask);
'allowed_usage'?
> if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
> rc = ima_check_blacklist(iint, modsig, pcr);
> if (rc != -EPERM) {
thanks,
Mimi
On Fri, 2024-03-08 at 11:08 -0500, Mimi Zohar wrote:
> Hi Roberto,
>
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index 3fc48214850a..48a09747ae7a 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -222,7 +222,9 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> > bool violation_check;
> > enum hash_algo hash_algo;
> > unsigned int allowed_algos = 0;
> > - u64 verif_mask = 0;
> > + u64 verif_mask = 0, *verif_mask_ptr, policy_mask = 0, allow_mask = 0;
> > + struct digest_cache *digest_cache = NULL, *found_cache;
> > + digest_cache_found_t found;
> >
> > if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> > return 0;
> > @@ -233,7 +235,7 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> > */
> > action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
> > mask, func, &pcr, &template_desc, NULL,
> > - &allowed_algos, NULL);
> > + &allowed_algos, &policy_mask);
> > violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
> > func == MMAP_CHECK_REQPROT) &&
> > (ima_policy_flag & IMA_MEASURE));
> > @@ -364,10 +366,34 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> > if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */
> > pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> >
> > + /*
> > + * For now we don't support nested verification with digest caches.
>
> I haven't reviewed the digest_cache LSM patch set yet. What does 'nested' mean
> in this context? Why mention it here?
This is the reason for the check func != DIGEST_LIST_CHECK. Before I
had this check in validate_rule(), but then realized that I would have
to reject rules without 'func='.
Not doing this check means that we could use digest caches also for
verifying digest lists (this is the meaning of nested verification).
This possibility is actually not remote, we would need this
functionality to verify Debian-based distributions.
Now, leaving aside the fact that Debian packages still use MD5 for
checksums (I heard that they would like to switch to SHA256 but didn't
verify if it happened), this would be the chain of verification:
/bin/cat -> md5sums -> coreutils (DEB pkg) -> Packages.gz -> Release
Release is the only file signed with PGP.
I already tried to verify a chain, it works, but I didn't want to
include too many functionality at the beginning. DEB format is not yet
supported, for RPMs chained verification it is not needed.
It would be fantastic if md5sums (or sha256sums) is directly signed
tough.
> > + * Since we allow IMA policy rules without func=, we have to enforce
> > + * this restriction here.
> > + */
> > + if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
> > + digest_cache = digest_cache_get(file_dentry(file));
>
> So whether or not a DIGEST_LIST_CHECK policy rule even exists,
> digest_cache_get() will be called. Similarly, even if a digest_cache list
> hasn't been measured or appraised, digest_cache_get() will be called.
>
> Basically every file in policy will check the digest_cache.
Only if there is 'digest_cache=content' in the matching rule.
> > +
> > + if (digest_cache) {
> > + found = digest_cache_lookup(file_dentry(file), digest_cache,
> > + iint->ima_hash->digest,
> > + iint->ima_hash->algo);
> > + /* AND what is allowed by the policy, and what IMA verified. */
> > + if (found) {
> > + found_cache = digest_cache_from_found_t(found);
> > + verif_mask_ptr = digest_cache_verif_get(found_cache,
> > + "ima");
>
> Instead of using "verif_{set,get}' consider using '{set,get}_usage', where usage
> here means measure or appraise.
Usage might make sense for IMA, not sure for any other user.
> > + if (verif_mask_ptr)
> > + allow_mask = policy_mask & *verif_mask_ptr;
> > + }
> > +
> > + digest_cache_put(digest_cache);
> > + }
> > +
>
> I'm wondering if it makes sense to create IMA wrappers for each of the
> digest_cache functions - checking the digest_cache for the hash, setting the
> digest_cache permitted usage, etc - and put all of them in a separate
> ima_digest_cache.c file. The file would only be included in the Makefile if
> digest_cache is configured.
It would be fine for me.
> In this file you could define a static local global variable to detect whether
> the digest_cache is ready to be used. Only after successfully measuring and
> appraising a digest_cache list, based on policy, set the variable.
Ok, something similar to ima_policy_flag.
> > if (action & IMA_MEASURE)
> > ima_store_measurement(iint, file, pathname,
> > xattr_value, xattr_len, modsig, pcr,
> > - template_desc);
> > + template_desc, allow_mask);
>
> 'allowed_usage'?
Sure, I started renaming them.
Thanks
Roberto
> > if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
> > rc = ima_check_blacklist(iint, modsig, pcr);
> > if (rc != -EPERM) {
>
> thanks,
>
> Mimi
Hi Roberto,
> b/security/integrity/ima/ima_main.c
> index a66522a22cbc..e1b2f5737753 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -301,6 +301,15 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
> }
> }
>
> + /* Check if digest cache changed since last measurement/appraisal. */
> + if (iint->digest_cache &&
> + digest_cache_changed(inode, iint->digest_cache)) {
> + iint->flags &= ~IMA_DONE_MASK;
> + iint->measured_pcrs = 0;
> + digest_cache_put(iint->digest_cache);
> + iint->digest_cache = NULL;
> + }
> +
> /* Determine if already appraised/measured based on bitmask
> * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
> * IMA_AUDIT, IMA_AUDITED)
> @@ -371,8 +380,15 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
> * Since we allow IMA policy rules without func=, we have to enforce
> * this restriction here.
> */
> - if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
> - digest_cache = digest_cache_get(file_dentry(file));
> + if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK) {
> + if (!iint->digest_cache) {
> + /* Released by ima_iint_free(). */
> + digest_cache = digest_cache_get(file_dentry(file));
> + iint->digest_cache = digest_cache;
> + } else {
> + digest_cache = iint->digest_cache;
> + }
Simple cleanup:
if (!iint->digest_cache)
iint->digest_cache =digest_cache_get(file_dentry(file));
digest_cache = iint->digest_cache;
> + }
>
> if (digest_cache) {
> found = digest_cache_lookup(file_dentry(file), digest_cache,
> @@ -386,8 +402,6 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
> if (verif_mask_ptr)
> allow_mask = policy_mask & *verif_mask_ptr;
> }
> -
> - digest_cache_put(digest_cache);
Keeping a reference to the digest_cache list for each file in the iint cache
until the file is re-accessed, might take a while to free.
I'm wondering if it necessary to keep a reference to the digest_cache. Or is it
possible to just compare the existing iint->digest_cache pointer with the
current digest_cache pointer?
thanks,
Mimi
> }
>
> if (action & IMA_MEASURE)
On Fri, 2024-03-08 at 12:35 -0500, Mimi Zohar wrote:
> Hi Roberto,
>
> > b/security/integrity/ima/ima_main.c
> > index a66522a22cbc..e1b2f5737753 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -301,6 +301,15 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> > }
> > }
> >
> > + /* Check if digest cache changed since last measurement/appraisal. */
> > + if (iint->digest_cache &&
> > + digest_cache_changed(inode, iint->digest_cache)) {
> > + iint->flags &= ~IMA_DONE_MASK;
> > + iint->measured_pcrs = 0;
> > + digest_cache_put(iint->digest_cache);
> > + iint->digest_cache = NULL;
> > + }
> > +
> > /* Determine if already appraised/measured based on bitmask
> > * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
> > * IMA_AUDIT, IMA_AUDITED)
> > @@ -371,8 +380,15 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> > * Since we allow IMA policy rules without func=, we have to enforce
> > * this restriction here.
> > */
> > - if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
> > - digest_cache = digest_cache_get(file_dentry(file));
> > + if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK) {
> > + if (!iint->digest_cache) {
> > + /* Released by ima_iint_free(). */
> > + digest_cache = digest_cache_get(file_dentry(file));
> > + iint->digest_cache = digest_cache;
> > + } else {
> > + digest_cache = iint->digest_cache;
> > + }
>
> Simple cleanup:
> if (!iint->digest_cache)
> iint->digest_cache =digest_cache_get(file_dentry(file));
>
> digest_cache = iint->digest_cache;
Thanks.
> > + }
> >
> > if (digest_cache) {
> > found = digest_cache_lookup(file_dentry(file), digest_cache,
> > @@ -386,8 +402,6 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> > if (verif_mask_ptr)
> > allow_mask = policy_mask & *verif_mask_ptr;
> > }
> > -
> > - digest_cache_put(digest_cache);
>
> Keeping a reference to the digest_cache list for each file in the iint cache
> until the file is re-accessed, might take a while to free.
Yes, that is the drawback...
> I'm wondering if it necessary to keep a reference to the digest_cache. Or is it
> possible to just compare the existing iint->digest_cache pointer with the
> current digest_cache pointer?
If the pointer value is the same, it does not guarantee that it is the
same digest cache used for the previous verification. It might have
been freed and reallocated.
Maybe, if the digest_cache LSM is able to notify to IMA that the digest
cache changed, so that IMA resets its flags in the integrity metadata,
we would not need to pin it.
Roberto
> thanks,
>
> Mimi
>
> > }
> >
> > if (action & IMA_MEASURE)
>
On Mon, 2024-03-11 at 10:11 +0100, Roberto Sassu wrote:
>
> > > @@ -386,8 +402,6 @@ static int process_measurement(struct file *file,
> > > const
> > > struct cred *cred,
> > > if (verif_mask_ptr)
> > > allow_mask = policy_mask & *verif_mask_ptr;
> > > }
> > > -
> > > - digest_cache_put(digest_cache);
> >
> > Keeping a reference to the digest_cache list for each file in the iint cache
> > until the file is re-accessed, might take a while to free.
>
> Yes, that is the drawback...
>
> > I'm wondering if it necessary to keep a reference to the digest_cache. Or
> > is it
> > possible to just compare the existing iint->digest_cache pointer with the
> > current digest_cache pointer?
>
> If the pointer value is the same, it does not guarantee that it is the
> same digest cache used for the previous verification. It might have
> been freed and reallocated.
Agreed.
>
> Maybe, if the digest_cache LSM is able to notify to IMA that the digest
> cache changed, so that IMA resets its flags in the integrity metadata,
> we would not need to pin it.
Yes, something similar to the "ima_lsm_policy_notifier".
Mimi
On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> Specify the 'digest_cache_measure' boot-time policy with 'ima_policy=' in
> the kernel command line to add the following rule at the beginning of the
> IMA policy, before other rules:
>
> measure func=DIGEST_LIST_CHECK pcr=12
>
> which will measure digest lists into PCR 12 (or the value in
> CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX).
>
> 'digest_cache_measure' also adds 'digest_cache=content pcr=12' to the other
> measure rules, if they have a compatible IMA hook. The PCR value still
> comes from CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX.
>
> Specify 'digest_cache_appraise' to add the following rule at the beginning,
> before other rules:
>
> appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig
>
> which will appraise digest lists with IMA signatures or module-style
> appended signatures.
>
> 'digest_cache_appraise' also adds 'digest_cache=content' to the other
> appraise rules, if they have a compatible IMA hook.
Defining two new built-in policies - digest_cache_measure, digest_cache_appraise
- in a single patch would be acceptable, if there wasn't anything else going on.
Changing other policy rules should not be made in this patch. A clear
explanation as to why other policy rules need to be modified is needed. It
shouldn't be hidden here.
thanks,
Mimi
>
> Signed-off-by: Roberto Sassu <[email protected]>
>
Roberto, please consider renaming this patch.
IMA is informing the digest_cache LSM of the digest_list verification result.
Instead of "ima: Record IMA verification result of digest lists in digest
cache", it should be "ima: inform digest_cache LSM of digest list verification
result".
Mimi