From b000d5cb954fe25ac1ea929ae6da321033ace927 Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Tue, 13 Oct 2020 10:18:04 +0200 Subject: ima: defer arch_ima_get_secureboot() call to IMA init time Chester reports that it is necessary to introduce a new way to pass the EFI secure boot status between the EFI stub and the core kernel on ARM systems. The usual way of obtaining this information is by checking the SecureBoot and SetupMode EFI variables, but this can only be done after the EFI variable workqueue is created, which occurs in a subsys_initcall(), whereas arch_ima_get_secureboot() is called much earlier by the IMA framework. However, the IMA framework itself is started as a late_initcall, and the only reason the call to arch_ima_get_secureboot() occurs so early is because it happens in the context of a __setup() callback that parses the ima_appraise= command line parameter. So let's refactor this code a little bit, by using a core_param() callback to capture the command line argument, and deferring any reasoning based on its contents to the IMA init routine. Cc: Chester Lin Cc: Dmitry Kasatkin Cc: James Morris Cc: "Serge E. Hallyn" Link: https://lore.kernel.org/linux-arm-kernel/20200904072905.25332-2-clin@suse.com/ Signed-off-by: Ard Biesheuvel Reported-by: kernel test robot [missing core_param()] [zohar@linux.ibm.com: included linux/module.h] Tested-by: Chester Lin Signed-off-by: Mimi Zohar --- include/linux/ima.h | 6 ++++++ security/integrity/ima/ima_appraise.c | 17 +++++++++++------ security/integrity/ima/ima_main.c | 1 + 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/include/linux/ima.h b/include/linux/ima.h index 8fa7bcfb2da2..ac3d82f962f2 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -31,6 +31,12 @@ extern void ima_post_path_mknod(struct dentry *dentry); extern int ima_file_hash(struct file *file, char *buf, size_t buf_size); extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size); +#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM +extern void ima_appraise_parse_cmdline(void); +#else +static inline void ima_appraise_parse_cmdline(void) {} +#endif + #ifdef CONFIG_IMA_KEXEC extern void ima_add_kexec_buffer(struct kimage *image); #endif diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 3dd8c2e4314e..8361941ee0a1 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -5,6 +5,7 @@ * Author: * Mimi Zohar */ +#include #include #include #include @@ -16,12 +17,19 @@ #include "ima.h" -static int __init default_appraise_setup(char *str) -{ #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM +static char *ima_appraise_cmdline_default __initdata; +core_param(ima_appraise, ima_appraise_cmdline_default, charp, 0); + +void __init ima_appraise_parse_cmdline(void) +{ + const char *str = ima_appraise_cmdline_default; bool sb_state = arch_ima_get_secureboot(); int appraisal_state = ima_appraise; + if (!str) + return; + if (strncmp(str, "off", 3) == 0) appraisal_state = 0; else if (strncmp(str, "log", 3) == 0) @@ -42,11 +50,8 @@ static int __init default_appraise_setup(char *str) } else { ima_appraise = appraisal_state; } -#endif - return 1; } - -__setup("ima_appraise=", default_appraise_setup); +#endif /* * is_ima_appraise_enabled - return appraise status diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 2d1af8899cab..a962b23e0429 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -904,6 +904,7 @@ static int __init init_ima(void) { int error; + ima_appraise_parse_cmdline(); ima_init_template_list(); hash_setup(CONFIG_IMA_DEFAULT_HASH); error = ima_init(); -- cgit v1.2.3-59-g8ed1b From dea87d0889dd663bd32e86824a0b35cd617ae1d0 Mon Sep 17 00:00:00 2001 From: Lakshmi Ramasubramanian Date: Thu, 12 Nov 2020 12:39:59 -0800 Subject: ima: select ima-buf template for buffer measurement The default IMA template used for all policy rules is the value set for CONFIG_IMA_DEFAULT_TEMPLATE if the policy rule does not specify a template. The default IMA template for buffer measurements should be 'ima-buf' - so that the measured buffer is correctly included in the IMA measurement log entry. With the default template format, buffer measurements are added to the measurement list, but do not include the buffer data, making it difficult, if not impossible, to validate. Including 'ima-buf' template records in the measurement list by default, should not impact existing attestation servers without 'ima-buf' template support. Initialize a global 'ima-buf' template and select that template, by default, for buffer measurements. Signed-off-by: Lakshmi Ramasubramanian Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_main.c | 24 +++++++++--------------- security/integrity/ima/ima_policy.c | 2 +- security/integrity/ima/ima_template.c | 26 ++++++++++++++++++++++++++ 4 files changed, 37 insertions(+), 16 deletions(-) diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 6ebefec616e4..8e8b1e3cb847 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -156,6 +156,7 @@ int template_desc_init_fields(const char *template_fmt, const struct ima_template_field ***fields, int *num_fields); struct ima_template_desc *ima_template_desc_current(void); +struct ima_template_desc *ima_template_desc_buf(void); struct ima_template_desc *lookup_template_desc(const char *name); bool ima_template_has_modsig(const struct ima_template_desc *ima_template); int ima_restore_measurement_entry(struct ima_template_entry *entry); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index a962b23e0429..68956e884403 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -413,7 +413,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) */ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) { - struct ima_template_desc *template; + struct ima_template_desc *template = NULL; struct file *file = vma->vm_file; char filename[NAME_MAX]; char *pathbuf = NULL; @@ -802,7 +802,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, .filename = eventname, .buf = buf, .buf_len = size}; - struct ima_template_desc *template = NULL; + struct ima_template_desc *template; struct { struct ima_digest_data hdr; char digest[IMA_MAX_DIGEST_SIZE]; @@ -814,6 +814,13 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, if (!ima_policy_flag) return; + template = ima_template_desc_buf(); + if (!template) { + ret = -EINVAL; + audit_cause = "ima_template_desc_buf"; + goto out; + } + /* * Both LSM hooks and auxilary based buffer measurements are * based on policy. To avoid code duplication, differentiate @@ -832,19 +839,6 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, if (!pcr) pcr = CONFIG_IMA_MEASURE_PCR_IDX; - if (!template) { - template = lookup_template_desc("ima-buf"); - ret = template_desc_init_fields(template->fmt, - &(template->fields), - &(template->num_fields)); - if (ret < 0) { - pr_err("template %s init failed, result: %d\n", - (strlen(template->name) ? - template->name : template->fmt), ret); - return; - } - } - iint.ima_hash = &hash.hdr; iint.ima_hash->algo = ima_hash_algo; iint.ima_hash->length = hash_digest_size[ima_hash_algo]; diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 9b5adeaa47fc..823a0c1379cb 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -628,7 +628,7 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid, struct ima_rule_entry *entry; int action = 0, actmask = flags | (flags << 1); - if (template_desc) + if (template_desc && !*template_desc) *template_desc = ima_template_desc_current(); rcu_read_lock(); diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 1e89e2d3851f..e22e510ae92d 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -55,6 +55,7 @@ static const struct ima_template_field supported_fields[] = { #define MAX_TEMPLATE_NAME_LEN sizeof("d-ng|n-ng|sig|buf|d-modisg|modsig") static struct ima_template_desc *ima_template; +static struct ima_template_desc *ima_buf_template; /** * ima_template_has_modsig - Check whether template has modsig-related fields. @@ -252,11 +253,36 @@ struct ima_template_desc *ima_template_desc_current(void) return ima_template; } +struct ima_template_desc *ima_template_desc_buf(void) +{ + if (!ima_buf_template) { + ima_init_template_list(); + ima_buf_template = lookup_template_desc("ima-buf"); + } + return ima_buf_template; +} + int __init ima_init_template(void) { struct ima_template_desc *template = ima_template_desc_current(); int result; + result = template_desc_init_fields(template->fmt, + &(template->fields), + &(template->num_fields)); + if (result < 0) { + pr_err("template %s init failed, result: %d\n", + (strlen(template->name) ? + template->name : template->fmt), result); + return result; + } + + template = ima_template_desc_buf(); + if (!template) { + pr_err("Failed to get ima-buf template\n"); + return -EINVAL; + } + result = template_desc_init_fields(template->fmt, &(template->fields), &(template->num_fields)); -- cgit v1.2.3-59-g8ed1b From 207cdd565dfc95a0a5185263a567817b7ebf5467 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Thu, 26 Nov 2020 11:34:56 +0100 Subject: ima: Don't modify file descriptor mode on the fly Commit a408e4a86b36b ("ima: open a new file instance if no read permissions") already introduced a second open to measure a file when the original file descriptor does not allow it. However, it didn't remove the existing method of changing the mode of the original file descriptor, which is still necessary if the current process does not have enough privileges to open a new one. Changing the mode isn't really an option, as the filesystem might need to do preliminary steps to make the read possible. Thus, this patch removes the code and keeps the second open as the only option to measure a file when it is unreadable with the original file descriptor. Cc: # 4.20.x: 0014cc04e8ec0 ima: Set file->f_mode Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension") Signed-off-by: Roberto Sassu Reviewed-by: Christoph Hellwig Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_crypto.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index 21989fa0c107..f6a7e9643b54 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -537,7 +537,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) loff_t i_size; int rc; struct file *f = file; - bool new_file_instance = false, modified_mode = false; + bool new_file_instance = false; /* * For consistency, fail file's opened with the O_DIRECT flag on @@ -555,18 +555,10 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL); flags |= O_RDONLY; f = dentry_open(&file->f_path, flags, file->f_cred); - if (IS_ERR(f)) { - /* - * Cannot open the file again, lets modify f_mode - * of original and continue - */ - pr_info_ratelimited("Unable to reopen file for reading.\n"); - f = file; - f->f_mode |= FMODE_READ; - modified_mode = true; - } else { - new_file_instance = true; - } + if (IS_ERR(f)) + return PTR_ERR(f); + + new_file_instance = true; } i_size = i_size_read(file_inode(f)); @@ -581,8 +573,6 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) out: if (new_file_instance) fput(f); - else if (modified_mode) - f->f_mode &= ~FMODE_READ; return rc; } -- cgit v1.2.3-59-g8ed1b