| Message ID | 20210705090922.3321178-4-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Provide more info about buffer measurement | expand |
On 7/5/2021 2:09 AM, Roberto Sassu wrote: > This patch adds the 'digest' and 'digest_len' parameters to > ima_measure_critical_data() and process_buffer_measurement(), so that > callers can get the digest of the passed buffer. > > These functions calculate the digest even if there is no suitable rule in > the IMA policy and, in this case, they simply return 1 before generating a > new measurement entry. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > include/linux/ima.h | 5 +-- > security/integrity/ima/ima.h | 2 +- > security/integrity/ima/ima_appraise.c | 2 +- > security/integrity/ima/ima_asymmetric_keys.c | 2 +- > security/integrity/ima/ima_init.c | 3 +- > security/integrity/ima/ima_main.c | 36 ++++++++++++++------ > security/integrity/ima/ima_queue_keys.c | 2 +- > security/selinux/ima.c | 6 ++-- > 8 files changed, 39 insertions(+), 19 deletions(-) Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> -lakshmi > > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 60492263aa64..b6ab66a546ae 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -38,7 +38,7 @@ extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size); > extern int ima_measure_critical_data(const char *event_label, > const char *event_name, > const void *buf, size_t buf_len, > - bool hash); > + bool hash, u8 *digest, size_t digest_len); > > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM > extern void ima_appraise_parse_cmdline(void); > @@ -147,7 +147,8 @@ static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) { > static inline int ima_measure_critical_data(const char *event_label, > const char *event_name, > const void *buf, size_t buf_len, > - bool hash) > + bool hash, u8 *digest, > + size_t digest_len) > { > return -ENOENT; > } > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 03db221324c3..2f4c20b16ad7 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -268,7 +268,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, > struct inode *inode, const void *buf, int size, > const char *eventname, enum ima_hooks func, > int pcr, const char *func_data, > - bool buf_hash); > + bool buf_hash, u8 *digest, size_t digest_len); > void ima_audit_measurement(struct integrity_iint_cache *iint, > const unsigned char *filename); > int ima_alloc_init_template(struct ima_event_data *event_data, > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index ef9dcfce45d4..63bec42c353f 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -357,7 +357,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint, > if ((rc == -EPERM) && (iint->flags & IMA_MEASURE)) > process_buffer_measurement(&init_user_ns, NULL, digest, digestsize, > "blacklisted-hash", NONE, > - pcr, NULL, false); > + pcr, NULL, false, NULL, 0); > } > > return rc; > diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c > index c985418698a4..f6aa0b47a772 100644 > --- a/security/integrity/ima/ima_asymmetric_keys.c > +++ b/security/integrity/ima/ima_asymmetric_keys.c > @@ -62,5 +62,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key, > */ > process_buffer_measurement(&init_user_ns, NULL, payload, payload_len, > keyring->description, KEY_CHECK, 0, > - keyring->description, false); > + keyring->description, false, NULL, 0); > } > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c > index 5076a7d9d23e..b26fa67476b4 100644 > --- a/security/integrity/ima/ima_init.c > +++ b/security/integrity/ima/ima_init.c > @@ -154,7 +154,8 @@ int __init ima_init(void) > ima_init_key_queue(); > > ima_measure_critical_data("kernel_info", "kernel_version", > - UTS_RELEASE, strlen(UTS_RELEASE), false); > + UTS_RELEASE, strlen(UTS_RELEASE), false, > + NULL, 0); > > return rc; > } > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index b512c06d8ee1..360266da5a10 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -838,17 +838,20 @@ int ima_post_load_data(char *buf, loff_t size, > * @pcr: pcr to extend the measurement > * @func_data: func specific data, may be NULL > * @buf_hash: measure buffer data hash > + * @digest: buffer digest will be written to > + * @digest_len: buffer length > * > * Based on policy, either the buffer data or buffer data hash is measured > * > - * Return: 0 if the buffer has been successfully measured, a negative value > - * otherwise. > + * Return: 0 if the buffer has been successfully measured, 1 if the digest > + * has been written to the passed location but not added to a measurement entry, > + * a negative value otherwise. > */ > int process_buffer_measurement(struct user_namespace *mnt_userns, > struct inode *inode, const void *buf, int size, > const char *eventname, enum ima_hooks func, > int pcr, const char *func_data, > - bool buf_hash) > + bool buf_hash, u8 *digest, size_t digest_len) > { > int ret = 0; > const char *audit_cause = "ENOMEM"; > @@ -869,7 +872,10 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, > int action = 0; > u32 secid; > > - if (!ima_policy_flag) > + if (digest && digest_len < digest_hash_len) > + return -EINVAL; > + > + if (!ima_policy_flag && !digest) > return -ENOENT; > > template = ima_template_desc_buf(); > @@ -891,7 +897,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, > action = ima_get_action(mnt_userns, inode, current_cred(), > secid, 0, func, &pcr, &template, > func_data); > - if (!(action & IMA_MEASURE)) > + if (!(action & IMA_MEASURE) && !digest) > return -ENOENT; > } > > @@ -922,6 +928,12 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, > event_data.buf_len = digest_hash_len; > } > > + if (digest) > + memcpy(digest, iint.ima_hash->digest, digest_hash_len); > + > + if (!ima_policy_flag || (func && !(action & IMA_MEASURE))) > + return 1; > + > ret = ima_alloc_init_template(&event_data, &entry, template); > if (ret < 0) { > audit_cause = "alloc_entry"; > @@ -964,7 +976,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) > > process_buffer_measurement(file_mnt_user_ns(f.file), file_inode(f.file), > buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0, > - NULL, false); > + NULL, false, NULL, 0); > fdput(f); > } > > @@ -975,26 +987,30 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) > * @buf: pointer to buffer data > * @buf_len: length of buffer data (in bytes) > * @hash: measure buffer data hash > + * @digest: buffer digest will be written to > + * @digest_len: buffer length > * > * Measure data critical to the integrity of the kernel into the IMA log > * and extend the pcr. Examples of critical data could be various data > * structures, policies, and states stored in kernel memory that can > * impact the integrity of the system. > * > - * Return: 0 if the buffer has been successfully measured, a negative value > - * otherwise. > + * Return: 0 if the buffer has been successfully measured, 1 if the digest > + * has been written to the passed location but not added to a measurement entry, > + * a negative value otherwise. > */ > int ima_measure_critical_data(const char *event_label, > const char *event_name, > const void *buf, size_t buf_len, > - bool hash) > + bool hash, u8 *digest, size_t digest_len) > { > if (!event_name || !event_label || !buf || !buf_len) > return -ENOPARAM; > > return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len, > event_name, CRITICAL_DATA, 0, > - event_label, hash); > + event_label, hash, digest, > + digest_len); > } > > static int __init init_ima(void) > diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c > index 979ef6c71f3d..93056c03bf5a 100644 > --- a/security/integrity/ima/ima_queue_keys.c > +++ b/security/integrity/ima/ima_queue_keys.c > @@ -165,7 +165,7 @@ void ima_process_queued_keys(void) > entry->keyring_name, > KEY_CHECK, 0, > entry->keyring_name, > - false); > + false, NULL, 0); > list_del(&entry->list); > ima_free_key_entry(entry); > } > diff --git a/security/selinux/ima.c b/security/selinux/ima.c > index 34d421861bfc..727c4e43219d 100644 > --- a/security/selinux/ima.c > +++ b/security/selinux/ima.c > @@ -86,7 +86,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state) > } > > ima_measure_critical_data("selinux", "selinux-state", > - state_str, strlen(state_str), false); > + state_str, strlen(state_str), false, > + NULL, 0); > > kfree(state_str); > > @@ -103,7 +104,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state) > } > > ima_measure_critical_data("selinux", "selinux-policy-hash", > - policy, policy_len, true); > + policy, policy_len, true, > + NULL, 0); > > vfree(policy); > } >
On Mon, Jul 5, 2021 at 5:09 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > This patch adds the 'digest' and 'digest_len' parameters to > ima_measure_critical_data() and process_buffer_measurement(), so that > callers can get the digest of the passed buffer. > > These functions calculate the digest even if there is no suitable rule in > the IMA policy and, in this case, they simply return 1 before generating a > new measurement entry. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > include/linux/ima.h | 5 +-- > security/integrity/ima/ima.h | 2 +- > security/integrity/ima/ima_appraise.c | 2 +- > security/integrity/ima/ima_asymmetric_keys.c | 2 +- > security/integrity/ima/ima_init.c | 3 +- > security/integrity/ima/ima_main.c | 36 ++++++++++++++------ > security/integrity/ima/ima_queue_keys.c | 2 +- > security/selinux/ima.c | 6 ++-- > 8 files changed, 39 insertions(+), 19 deletions(-) The SELinux changes are trivial and fall into that cross-subsystem-ACK-not-really-necessary category, but why not :) For the SELinux bits: Acked-by: Paul Moore <paul@paul-moore.com>
Hi Roberto, On Mon, 2021-07-05 at 11:09 +0200, Roberto Sassu wrote: > This patch adds the 'digest' and 'digest_len' parameters to > ima_measure_critical_data() and process_buffer_measurement(), so that > callers can get the digest of the passed buffer. > > These functions calculate the digest even if there is no suitable rule in > the IMA policy and, in this case, they simply return 1 before generating a > new measurement entry. I agree ima_measure_critical_data() is special. Both this patch description and 1/3 describe "what", not "why". Please provide the motivation for these changes at least in the cover letter, if not in the patch description. thanks, Mimi
diff --git a/include/linux/ima.h b/include/linux/ima.h index 60492263aa64..b6ab66a546ae 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -38,7 +38,7 @@ extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size); extern int ima_measure_critical_data(const char *event_label, const char *event_name, const void *buf, size_t buf_len, - bool hash); + bool hash, u8 *digest, size_t digest_len); #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM extern void ima_appraise_parse_cmdline(void); @@ -147,7 +147,8 @@ static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) { static inline int ima_measure_critical_data(const char *event_label, const char *event_name, const void *buf, size_t buf_len, - bool hash) + bool hash, u8 *digest, + size_t digest_len) { return -ENOENT; } diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 03db221324c3..2f4c20b16ad7 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -268,7 +268,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, struct inode *inode, const void *buf, int size, const char *eventname, enum ima_hooks func, int pcr, const char *func_data, - bool buf_hash); + bool buf_hash, u8 *digest, size_t digest_len); void ima_audit_measurement(struct integrity_iint_cache *iint, const unsigned char *filename); int ima_alloc_init_template(struct ima_event_data *event_data, diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index ef9dcfce45d4..63bec42c353f 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -357,7 +357,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint, if ((rc == -EPERM) && (iint->flags & IMA_MEASURE)) process_buffer_measurement(&init_user_ns, NULL, digest, digestsize, "blacklisted-hash", NONE, - pcr, NULL, false); + pcr, NULL, false, NULL, 0); } return rc; diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c index c985418698a4..f6aa0b47a772 100644 --- a/security/integrity/ima/ima_asymmetric_keys.c +++ b/security/integrity/ima/ima_asymmetric_keys.c @@ -62,5 +62,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key, */ process_buffer_measurement(&init_user_ns, NULL, payload, payload_len, keyring->description, KEY_CHECK, 0, - keyring->description, false); + keyring->description, false, NULL, 0); } diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 5076a7d9d23e..b26fa67476b4 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -154,7 +154,8 @@ int __init ima_init(void) ima_init_key_queue(); ima_measure_critical_data("kernel_info", "kernel_version", - UTS_RELEASE, strlen(UTS_RELEASE), false); + UTS_RELEASE, strlen(UTS_RELEASE), false, + NULL, 0); return rc; } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index b512c06d8ee1..360266da5a10 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -838,17 +838,20 @@ int ima_post_load_data(char *buf, loff_t size, * @pcr: pcr to extend the measurement * @func_data: func specific data, may be NULL * @buf_hash: measure buffer data hash + * @digest: buffer digest will be written to + * @digest_len: buffer length * * Based on policy, either the buffer data or buffer data hash is measured * - * Return: 0 if the buffer has been successfully measured, a negative value - * otherwise. + * Return: 0 if the buffer has been successfully measured, 1 if the digest + * has been written to the passed location but not added to a measurement entry, + * a negative value otherwise. */ int process_buffer_measurement(struct user_namespace *mnt_userns, struct inode *inode, const void *buf, int size, const char *eventname, enum ima_hooks func, int pcr, const char *func_data, - bool buf_hash) + bool buf_hash, u8 *digest, size_t digest_len) { int ret = 0; const char *audit_cause = "ENOMEM"; @@ -869,7 +872,10 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, int action = 0; u32 secid; - if (!ima_policy_flag) + if (digest && digest_len < digest_hash_len) + return -EINVAL; + + if (!ima_policy_flag && !digest) return -ENOENT; template = ima_template_desc_buf(); @@ -891,7 +897,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, action = ima_get_action(mnt_userns, inode, current_cred(), secid, 0, func, &pcr, &template, func_data); - if (!(action & IMA_MEASURE)) + if (!(action & IMA_MEASURE) && !digest) return -ENOENT; } @@ -922,6 +928,12 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, event_data.buf_len = digest_hash_len; } + if (digest) + memcpy(digest, iint.ima_hash->digest, digest_hash_len); + + if (!ima_policy_flag || (func && !(action & IMA_MEASURE))) + return 1; + ret = ima_alloc_init_template(&event_data, &entry, template); if (ret < 0) { audit_cause = "alloc_entry"; @@ -964,7 +976,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) process_buffer_measurement(file_mnt_user_ns(f.file), file_inode(f.file), buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0, - NULL, false); + NULL, false, NULL, 0); fdput(f); } @@ -975,26 +987,30 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) * @buf: pointer to buffer data * @buf_len: length of buffer data (in bytes) * @hash: measure buffer data hash + * @digest: buffer digest will be written to + * @digest_len: buffer length * * Measure data critical to the integrity of the kernel into the IMA log * and extend the pcr. Examples of critical data could be various data * structures, policies, and states stored in kernel memory that can * impact the integrity of the system. * - * Return: 0 if the buffer has been successfully measured, a negative value - * otherwise. + * Return: 0 if the buffer has been successfully measured, 1 if the digest + * has been written to the passed location but not added to a measurement entry, + * a negative value otherwise. */ int ima_measure_critical_data(const char *event_label, const char *event_name, const void *buf, size_t buf_len, - bool hash) + bool hash, u8 *digest, size_t digest_len) { if (!event_name || !event_label || !buf || !buf_len) return -ENOPARAM; return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len, event_name, CRITICAL_DATA, 0, - event_label, hash); + event_label, hash, digest, + digest_len); } static int __init init_ima(void) diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c index 979ef6c71f3d..93056c03bf5a 100644 --- a/security/integrity/ima/ima_queue_keys.c +++ b/security/integrity/ima/ima_queue_keys.c @@ -165,7 +165,7 @@ void ima_process_queued_keys(void) entry->keyring_name, KEY_CHECK, 0, entry->keyring_name, - false); + false, NULL, 0); list_del(&entry->list); ima_free_key_entry(entry); } diff --git a/security/selinux/ima.c b/security/selinux/ima.c index 34d421861bfc..727c4e43219d 100644 --- a/security/selinux/ima.c +++ b/security/selinux/ima.c @@ -86,7 +86,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state) } ima_measure_critical_data("selinux", "selinux-state", - state_str, strlen(state_str), false); + state_str, strlen(state_str), false, + NULL, 0); kfree(state_str); @@ -103,7 +104,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state) } ima_measure_critical_data("selinux", "selinux-policy-hash", - policy, policy_len, true); + policy, policy_len, true, + NULL, 0); vfree(policy); }
This patch adds the 'digest' and 'digest_len' parameters to ima_measure_critical_data() and process_buffer_measurement(), so that callers can get the digest of the passed buffer. These functions calculate the digest even if there is no suitable rule in the IMA policy and, in this case, they simply return 1 before generating a new measurement entry. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- include/linux/ima.h | 5 +-- security/integrity/ima/ima.h | 2 +- security/integrity/ima/ima_appraise.c | 2 +- security/integrity/ima/ima_asymmetric_keys.c | 2 +- security/integrity/ima/ima_init.c | 3 +- security/integrity/ima/ima_main.c | 36 ++++++++++++++------ security/integrity/ima/ima_queue_keys.c | 2 +- security/selinux/ima.c | 6 ++-- 8 files changed, 39 insertions(+), 19 deletions(-)