From 691115c3513ec83edf68ba6575ae85630bc94b8b Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 7 Sep 2018 12:16:24 -0700 Subject: [PATCH 1/6] vfs: require i_size <= SIZE_MAX in kernel_read_file() On 32-bit systems, the buffer allocated by kernel_read_file() is too small if the file size is > SIZE_MAX, due to truncation to size_t. Fortunately, since the 'count' argument to kernel_read() is also truncated to size_t, only the allocated space is filled; then, -EIO is returned since 'pos != i_size' after the read loop. But this is not obvious and seems incidental. We should be more explicit about this case. So, fail early if i_size > SIZE_MAX. Signed-off-by: Eric Biggers Signed-off-by: Mimi Zohar --- fs/exec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 1ebf6e5a521d..fc281b738a98 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -908,14 +908,14 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, goto out; i_size = i_size_read(file_inode(file)); - if (max_size > 0 && i_size > max_size) { - ret = -EFBIG; - goto out; - } if (i_size <= 0) { ret = -EINVAL; goto out; } + if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) { + ret = -EFBIG; + goto out; + } if (id != READING_FIRMWARE_PREALLOC_BUFFER) *buf = vmalloc(i_size); From b2724d5802a77b7fb47e84d9b88b80370eccbc64 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 7 Sep 2018 13:22:23 -0700 Subject: [PATCH 2/6] security/integrity: constify some read-only data Constify some static data that is never modified, so that it is placed in .rodata. Signed-off-by: Eric Biggers Signed-off-by: Mimi Zohar --- security/integrity/digsig.c | 2 +- security/integrity/evm/evm_crypto.c | 4 ++-- security/integrity/ima/ima.h | 2 +- security/integrity/ima/ima_api.c | 3 ++- security/integrity/ima/ima_fs.c | 3 ++- security/integrity/ima/ima_init.c | 2 +- security/integrity/ima/ima_main.c | 2 +- security/integrity/ima/ima_template.c | 11 ++++++----- 8 files changed, 16 insertions(+), 13 deletions(-) diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index 9bb0a7f2863e..879396fa3be0 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -26,7 +26,7 @@ static struct key *keyring[INTEGRITY_KEYRING_MAX]; -static const char *keyring_name[INTEGRITY_KEYRING_MAX] = { +static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = { #ifndef CONFIG_INTEGRITY_TRUSTED_KEYRING "_evm", "_ima", diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 8a3905bb02c7..8c25f949ebdb 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -27,7 +27,7 @@ #define EVMKEY "evm-key" #define MAX_KEY_SIZE 128 static unsigned char evmkey[MAX_KEY_SIZE]; -static int evmkey_len = MAX_KEY_SIZE; +static const int evmkey_len = MAX_KEY_SIZE; struct crypto_shash *hmac_tfm; static struct crypto_shash *evm_tfm[HASH_ALGO__LAST]; @@ -38,7 +38,7 @@ static DEFINE_MUTEX(mutex); static unsigned long evm_set_key_flags; -static char * const evm_hmac = "hmac(sha1)"; +static const char evm_hmac[] = "hmac(sha1)"; /** * evm_set_key() - set EVM HMAC key from the kernel diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 67db9d9454ca..cc12f3449a72 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -88,7 +88,7 @@ struct ima_template_desc { char *name; char *fmt; int num_fields; - struct ima_template_field **fields; + const struct ima_template_field **fields; }; struct ima_template_entry { diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index a02c5acfd403..99dd1d53fc35 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -51,7 +51,8 @@ int ima_alloc_init_template(struct ima_event_data *event_data, (*entry)->template_desc = template_desc; for (i = 0; i < template_desc->num_fields; i++) { - struct ima_template_field *field = template_desc->fields[i]; + const struct ima_template_field *field = + template_desc->fields[i]; u32 len; result = field->field_init(event_data, diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index ae9d5c766a3c..fe0ede883557 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -179,7 +179,8 @@ int ima_measurements_show(struct seq_file *m, void *v) /* 6th: template specific data */ for (i = 0; i < e->template_desc->num_fields; i++) { enum ima_show_type show = IMA_SHOW_BINARY; - struct ima_template_field *field = e->template_desc->fields[i]; + const struct ima_template_field *field = + e->template_desc->fields[i]; if (is_ima_template && strcmp(field->field_id, "d") == 0) show = IMA_SHOW_BINARY_NO_FIELD_LEN; diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index faac9ecaa0ae..59d834219cd6 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -25,7 +25,7 @@ #include "ima.h" /* name for boot aggregate entry */ -static const char *boot_aggregate_name = "boot_aggregate"; +static const char boot_aggregate_name[] = "boot_aggregate"; struct tpm_chip *ima_tpm_chip; /* Add the boot aggregate to the IMA measurement list and extend diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 2d31921fbda4..1b88d58e1325 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -440,7 +440,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id) return 0; } -static int read_idmap[READING_MAX_ID] = { +static const int read_idmap[READING_MAX_ID] = { [READING_FIRMWARE] = FIRMWARE_CHECK, [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK, [READING_MODULE] = MODULE_CHECK, diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 30db39b23804..b631b8bc7624 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -32,7 +32,7 @@ static struct ima_template_desc builtin_templates[] = { static LIST_HEAD(defined_templates); static DEFINE_SPINLOCK(template_list); -static struct ima_template_field supported_fields[] = { +static const struct ima_template_field supported_fields[] = { {.field_id = "d", .field_init = ima_eventdigest_init, .field_show = ima_show_template_digest}, {.field_id = "n", .field_init = ima_eventname_init, @@ -49,7 +49,7 @@ static struct ima_template_field supported_fields[] = { static struct ima_template_desc *ima_template; static struct ima_template_desc *lookup_template_desc(const char *name); static int template_desc_init_fields(const char *template_fmt, - struct ima_template_field ***fields, + const struct ima_template_field ***fields, int *num_fields); static int __init ima_template_setup(char *str) @@ -125,7 +125,8 @@ static struct ima_template_desc *lookup_template_desc(const char *name) return found ? template_desc : NULL; } -static struct ima_template_field *lookup_template_field(const char *field_id) +static const struct ima_template_field * +lookup_template_field(const char *field_id) { int i; @@ -153,11 +154,11 @@ static int template_fmt_size(const char *template_fmt) } static int template_desc_init_fields(const char *template_fmt, - struct ima_template_field ***fields, + const struct ima_template_field ***fields, int *num_fields) { const char *template_fmt_ptr; - struct ima_template_field *found_fields[IMA_TEMPLATE_NUM_FIELDS_MAX]; + const struct ima_template_field *found_fields[IMA_TEMPLATE_NUM_FIELDS_MAX]; int template_num_fields; int i, len; From 2ab5daf867d1f7898327962d59d3039206294d3a Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 3 Oct 2018 17:15:44 -0700 Subject: [PATCH 3/6] security/integrity: remove unnecessary 'init_keyring' variable The 'init_keyring' variable actually just gave the value of CONFIG_INTEGRITY_TRUSTED_KEYRING. We should check the config option directly instead. No change in behavior; this just simplifies the code. Signed-off-by: Eric Biggers Signed-off-by: Mimi Zohar --- security/integrity/digsig.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index 879396fa3be0..5eacba858e4b 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -37,12 +37,6 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = { "_module", }; -#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING -static bool init_keyring __initdata = true; -#else -static bool init_keyring __initdata; -#endif - #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY #define restrict_link_to_ima restrict_link_by_builtin_and_secondary_trusted #else @@ -85,7 +79,7 @@ int __init integrity_init_keyring(const unsigned int id) struct key_restriction *restriction; int err = 0; - if (!init_keyring) + if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING)) return 0; restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL); From 1e4c8dafbb6bf72fb5eca035b861e39c5896c2b7 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 7 Sep 2018 14:33:24 -0700 Subject: [PATCH 4/6] ima: fix showing large 'violations' or 'runtime_measurements_count' The 12 character temporary buffer is not necessarily long enough to hold a 'long' value. Increase it. Signed-off-by: Eric Biggers Cc: stable@vger.kernel.org Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_fs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index fe0ede883557..3183cc23d0f8 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -42,14 +42,14 @@ static int __init default_canonical_fmt_setup(char *str) __setup("ima_canonical_fmt", default_canonical_fmt_setup); static int valid_policy = 1; -#define TMPBUFLEN 12 + static ssize_t ima_show_htable_value(char __user *buf, size_t count, loff_t *ppos, atomic_long_t *val) { - char tmpbuf[TMPBUFLEN]; + char tmpbuf[32]; /* greater than largest 'long' string value */ ssize_t len; - len = scnprintf(tmpbuf, TMPBUFLEN, "%li\n", atomic_long_read(val)); + len = scnprintf(tmpbuf, sizeof(tmpbuf), "%li\n", atomic_long_read(val)); return simple_read_from_buffer(buf, count, ppos, tmpbuf, len); } From a408e4a86b36bf98ad15b9ada531cf0e5118ac67 Mon Sep 17 00:00:00 2001 From: Goldwyn Rodrigues Date: Tue, 9 Oct 2018 10:12:33 -0500 Subject: [PATCH 5/6] ima: open a new file instance if no read permissions Open a new file instance as opposed to changing file->f_mode when the file is not readable. This is done to accomodate overlayfs stacked file operations change. The real struct file is hidden behind the overlays struct file. So, any file->f_mode manipulations are not reflected on the real struct file. Open the file again in read mode if original file cannot be read, read and calculate the hash. Signed-off-by: Goldwyn Rodrigues Cc: stable@vger.kernel.org (linux-4.19) Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_crypto.c | 60 ++++++++++++++++++----------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index 7e7e7e7c250a..d9e7728027c6 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -210,7 +210,7 @@ static int ima_calc_file_hash_atfm(struct file *file, { loff_t i_size, offset; char *rbuf[2] = { NULL, }; - int rc, read = 0, rbuf_len, active = 0, ahash_rc = 0; + int rc, rbuf_len, active = 0, ahash_rc = 0; struct ahash_request *req; struct scatterlist sg[1]; struct crypto_wait wait; @@ -257,11 +257,6 @@ static int ima_calc_file_hash_atfm(struct file *file, &rbuf_size[1], 0); } - if (!(file->f_mode & FMODE_READ)) { - file->f_mode |= FMODE_READ; - read = 1; - } - for (offset = 0; offset < i_size; offset += rbuf_len) { if (!rbuf[1] && offset) { /* Not using two buffers, and it is not the first @@ -300,8 +295,6 @@ static int ima_calc_file_hash_atfm(struct file *file, /* wait for the last update request to complete */ rc = ahash_wait(ahash_rc, &wait); out3: - if (read) - file->f_mode &= ~FMODE_READ; ima_free_pages(rbuf[0], rbuf_size[0]); ima_free_pages(rbuf[1], rbuf_size[1]); out2: @@ -336,7 +329,7 @@ static int ima_calc_file_hash_tfm(struct file *file, { loff_t i_size, offset = 0; char *rbuf; - int rc, read = 0; + int rc; SHASH_DESC_ON_STACK(shash, tfm); shash->tfm = tfm; @@ -357,11 +350,6 @@ static int ima_calc_file_hash_tfm(struct file *file, if (!rbuf) return -ENOMEM; - if (!(file->f_mode & FMODE_READ)) { - file->f_mode |= FMODE_READ; - read = 1; - } - while (offset < i_size) { int rbuf_len; @@ -378,8 +366,6 @@ static int ima_calc_file_hash_tfm(struct file *file, if (rc) break; } - if (read) - file->f_mode &= ~FMODE_READ; kfree(rbuf); out: if (!rc) @@ -420,6 +406,8 @@ 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_flags = false; /* * For consistency, fail file's opened with the O_DIRECT flag on @@ -431,15 +419,41 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) return -EINVAL; } - i_size = i_size_read(file_inode(file)); - - if (ima_ahash_minsize && i_size >= ima_ahash_minsize) { - rc = ima_calc_file_ahash(file, hash); - if (!rc) - return 0; + /* Open a new file instance in O_RDONLY if we cannot read */ + if (!(file->f_mode & FMODE_READ)) { + int flags = file->f_flags & ~(O_WRONLY | O_APPEND | + 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_flags + * of original and continue + */ + pr_info_ratelimited("Unable to reopen file for reading.\n"); + f = file; + f->f_flags |= FMODE_READ; + modified_flags = true; + } else { + new_file_instance = true; + } } - return ima_calc_file_shash(file, hash); + i_size = i_size_read(file_inode(f)); + + if (ima_ahash_minsize && i_size >= ima_ahash_minsize) { + rc = ima_calc_file_ahash(f, hash); + if (!rc) + goto out; + } + + rc = ima_calc_file_shash(f, hash); +out: + if (new_file_instance) + fput(f); + else if (modified_flags) + f->f_flags &= ~FMODE_READ; + return rc; } /* From 34bccd61b139dbb452421aebf480e7af5f7a12ed Mon Sep 17 00:00:00 2001 From: Mimi Zohar Date: Thu, 11 Oct 2018 09:23:02 -0400 Subject: [PATCH 6/6] MAINTAINERS: add Jarkko as maintainer for trusted keys Trusted keys are variable length random numbers generated by the TPM, which are used as symmetric keys. The trusted key is never exported to userspace in the clear. Adding Jarrko, the TPM maintainer. Signed-off-by: Mimi Zohar --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 9ad052aeac39..da05e5057242 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8089,6 +8089,7 @@ F: security/keys/encrypted-keys/ KEYS-TRUSTED M: James Bottomley +M: Jarkko Sakkinen M: Mimi Zohar L: linux-integrity@vger.kernel.org L: keyrings@vger.kernel.org