| Message ID | 20191220074929.8191-1-janne.karhunen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,-,RFC] ima: export the measurement list when needed | expand |
On Fri, Dec 20, 2019 at 9:49 AM Janne Karhunen <janne.karhunen@gmail.com> wrote: > Some systems can end up carrying lots of entries in the ima > measurement list. Since every entry is using a bit of kernel > memory, add a new Kconfig variable to allow the sysadmin to > define the maximum measurement list size and the location > of the exported list. This patch now passes some basic test runs and looks to me it's doing things correctly, if others are willing to give it a spin. My basic test case for this is a simple bash for loop, creating thousands of files and using this trigger to free the list. Once we have the workqueue, the list free job should probably be triggered from the ima_store_measurement() when the htable length says so. This way the list will always stay truncated to the right size and no-one has to poll. When it comes to the oddity that kernel is creating files, the facts are: 1) the data cannot be kept in the memory forever, 2) the data cannot be thrown away. So, what are the options. I didn't see any other obvious way. Luckily there was a ready API for the job. -- Janne
[Cc'ing LSM mailing list for a wider audience] On Fri, 2019-12-20 at 09:49 +0200, Janne Karhunen wrote: > Some systems can end up carrying lots of entries in the ima > measurement list. Since every entry is using a bit of kernel > memory, add a new Kconfig variable to allow the sysadmin to > define the maximum measurement list size and the location > of the exported list. > > The list is written out in append mode, so the system will > keep writing new entries as long as it stays running or runs > out of space. File is also automatically truncated on startup. > > Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com> Continually adding new measurements, without limiting or removing the measurement list seems to becoming more of an issue. From Dave Safford's TLV patch description[1]: A second goal of the [TLV] patch set is to test the more radical idea of being able to copy the measurement list data out of the kernel. The data is verifiable with the TPM PCR value, and need not be kept in kernel memory. In some cases, this "memory leak" can grow large enough to cause issues, and this is a test of a potential way to solve that problem. The TLV version automatically removed the measurement list the first time the measurement list was read, which sounded very odd to me. In an offline discussion, Dave further clarified that reading the measurement list should be similar to how a trusted userspace application reads kernel messages. The difference being kernel messages are stored in a circular buffer and may be dropped. In the IMA measurement list case, the measurement list would grow until the trusted userspace application gets around to reading the measurement list. Should the kernel be involved in writing the IMA measurement list to a file or, as Dave suggested, this should be delegated to a userspace application? Mimi [1] https://lore.kernel.org/linux-integrity/BCA04D5D9A3B764C9B7405BBA4 D4A3C002569222@ALPMBAPA12.e2k.ad.ge.com/
On Fri, Dec 20, 2019 at 4:04 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > Should the kernel be involved in writing the IMA measurement list to a > file or, as Dave suggested, this should be delegated to a userspace > application? That is a good question. I went this way as it did not feel right to me that the kernel would depend on periodic, reliable userspace functionality to stay running (we would have a circular dependency). The thing is, once the kernel starts to run low on memory, it may kill that periodic daemon flushing the data for reasons unrelated to IMA.
On Sat, Dec 21, 2019 at 12:41 PM Janne Karhunen <janne.karhunen@gmail.com> wrote: > > Should the kernel be involved in writing the IMA measurement list to a > > file or, as Dave suggested, this should be delegated to a userspace > > application? > > That is a good question. I went this way as it did not feel right to > me that the kernel would depend on periodic, reliable userspace > functionality to stay running (we would have a circular dependency). > The thing is, once the kernel starts to run low on memory, it may kill > that periodic daemon flushing the data for reasons unrelated to IMA. Besides the dependency, I think the requirement should be that we can survive the basic test of 'while true; do touch $RANDOM; done' at least until we run out of allocated diskspace. While arranging this with userspace flushers is not impossible, it is order of magnitude more complex to do correctly than just letting the kernel write the file. Even if it feels somewhat unorthodox. Above patch survives that test case with 3 line addition via a workqueue. Once the admin points IMA to some mount, the above test case (while loop creating files full speed) will run a long, long time. Effectively this is really just kernel doing its own memory management as it should. Flush out the dirty pages you do not really need to stay running. -- Janne
On Sat, 2019-12-21 at 12:41 +0200, Janne Karhunen wrote: > On Fri, Dec 20, 2019 at 4:04 PM Mimi Zohar <zohar@linux.ibm.com> > wrote: > > > Should the kernel be involved in writing the IMA measurement list > > to a > > file or, as Dave suggested, this should be delegated to a userspace > > application? > > That is a good question. I went this way as it did not feel right to > me that the kernel would depend on periodic, reliable userspace > functionality to stay running (we would have a circular dependency). > The thing is, once the kernel starts to run low on memory, it may > kill > that periodic daemon flushing the data for reasons unrelated to IMA. > I'm happy with either way (kernel writing, or userspace reading) the data, but with the v1 patch, there is no way for userspace to force that the list be flushed - it only flushes on full. I think it is important for userspace to be able to trigger a flush, such as just prior to a kexec, or prior to an attestation. Perhaps you could simply remove the length test in ima_export_list(), and export anytime the filename is provided? This could simplify attestation clients, which could ask for different files each time (list.1, list.2...), for automatic log maintenance. Since the template format does not have sequence numbers, this would also help keep track which records have already been seen. dave
On Tue, Dec 24, 2019 at 5:35 PM <david.safford@gmail.com> wrote: > > That is a good question. I went this way as it did not feel right to > > me that the kernel would depend on periodic, reliable userspace > > functionality to stay running (we would have a circular dependency). > > The thing is, once the kernel starts to run low on memory, it may > > kill > > that periodic daemon flushing the data for reasons unrelated to IMA. > > > > I'm happy with either way (kernel writing, or userspace reading) the > data, but with the v1 patch, there is no way for userspace to force > that the list be flushed - it only flushes on full. I think it is > important for userspace to be able to trigger a flush, such as just > prior to a kexec, or prior to an attestation. Indeed, will add in v2. > Perhaps you could simply remove the length test in ima_export_list(), > and export anytime the filename is provided? This could simplify > attestation clients, which could ask for different files each time > (list.1, list.2...), for automatic log maintenance. Since the template > format does not have sequence numbers, this would also help keep > track which records have already been seen. Yes, will do something like this. Holidays cause some latency here, but I will send an update next week. -- Janne
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 897bafc59a33..3aefead28864 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -310,3 +310,13 @@ config IMA_APPRAISE_SIGNED_INIT default n help This option requires user-space init to be signed. + +config IMA_MEASUREMENT_LIST_SIZE + int + depends on IMA + default 2000 + help + This option defines the maximum amount of entries in the + measurement list. Once the limit is reached, the entire + list is exported to a user defined file and the kernel + list is freed. diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 19769bf5f6ab..78f0b706848d 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -151,20 +151,19 @@ int template_desc_init_fields(const char *template_fmt, struct ima_template_desc *ima_template_desc_current(void); struct ima_template_desc *lookup_template_desc(const char *name); bool ima_template_has_modsig(const struct ima_template_desc *ima_template); +void ima_free_template_entry(struct ima_template_entry *entry); int ima_restore_measurement_entry(struct ima_template_entry *entry); int ima_restore_measurement_list(loff_t bufsize, void *buf); int ima_measurements_show(struct seq_file *m, void *v); unsigned long ima_get_binary_runtime_size(void); int ima_init_template(void); void ima_init_template_list(void); +int ima_export_list(const char *from); int __init ima_init_digests(void); int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event, void *lsm_data); -/* - * used to protect h_table and sha_table - */ -extern spinlock_t ima_queue_lock; +extern struct mutex ima_extend_list_mutex; struct ima_h_table { atomic_long_t len; /* number of stored measurements in the list */ diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index 2000e8df0301..ad7daf7d2fbb 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -22,10 +22,17 @@ #include <linux/rcupdate.h> #include <linux/parser.h> #include <linux/vmalloc.h> +#include <linux/fs_struct.h> +#include <linux/syscalls.h> #include "ima.h" +#define secfs_mnt "/sys/kernel/security" +#define am_filename "/integrity/ima/ascii_runtime_measurements" + static DEFINE_MUTEX(ima_write_mutex); +static DEFINE_MUTEX(ima_list_mutex); +static char *ima_msmt_list_name; bool ima_canonical_fmt; static int __init default_canonical_fmt_setup(char *str) @@ -362,6 +369,7 @@ static struct dentry *ascii_runtime_measurements; static struct dentry *runtime_measurements_count; static struct dentry *violations; static struct dentry *ima_policy; +static struct dentry *ima_list_name; enum ima_fs_flags { IMA_FS_BUSY, @@ -449,6 +457,179 @@ static const struct file_operations ima_measure_policy_ops = { .llseek = generic_file_llseek, }; +static void ima_free_list(void) +{ + struct ima_queue_entry *qe, *e; + + list_for_each_entry_safe(qe, e, &ima_measurements, later) { + hlist_del_rcu(&qe->hnext); + atomic_long_dec(&ima_htable.len); + + list_del_rcu(&qe->later); + ima_free_template_entry(qe->entry); + kfree(qe); + } +} + +static int ima_unlink_file(const char *filename) +{ + struct filename *file; + + file = getname_kernel(filename); + if (IS_ERR(file)) + return -EINVAL; + + return do_unlinkat(AT_FDCWD, file); +} + +int ima_export_list(const char *from) +{ + static bool export_ok = true; + static bool init_export = true; + + struct file *file_out = NULL; + struct file *file_in = NULL; + const char *to = ima_msmt_list_name; + ssize_t bytesin, bytesout; + mm_segment_t fs; + struct path root; + loff_t offin = 0, offout = 0; + char data[512]; + long htable_len; + int err = 0; + + htable_len = atomic_long_read(&ima_htable.len); + if (htable_len <= CONFIG_IMA_MEASUREMENT_LIST_SIZE) + goto out_err; + if (to == NULL) + goto out_err; + if (from == NULL) + from = secfs_mnt am_filename; + if (!export_ok) { + err = -EFAULT; + goto out_err; + } + + pr_info("msmt list size (%ld/%ld) exceeded, exporting to %s\n", + htable_len, (long)CONFIG_IMA_MEASUREMENT_LIST_SIZE, to); + fs = get_fs(); + set_fs(KERNEL_DS); + + if (init_export) { + ima_unlink_file(to); + init_export = false; + } + /* + * Use the root of the init task.. + */ + task_lock(&init_task); + get_fs_root(init_task.fs, &root); + task_unlock(&init_task); + + file_out = file_open_root(root.dentry, root.mnt, to, + O_CREAT|O_WRONLY|O_APPEND|O_NOFOLLOW, + 0400); + if (IS_ERR(file_out)) { + err = PTR_ERR(file_out); + pr_err("failed to open %s, err %d\n", to, err); + file_out = NULL; + goto out_close; + } + file_in = file_open_root(root.dentry, root.mnt, from, O_RDONLY, 0); + if (IS_ERR(file_in)) { + err = PTR_ERR(file_in); + pr_err("failed to open %s, err %d\n", from, err); + file_in = NULL; + goto out_close; + } + mutex_lock(&ima_extend_list_mutex); + do { + bytesin = vfs_read(file_in, data, 512, &offin); + if (bytesin < 0) { + pr_err("read error at %lld\n", offin); + err = -EIO; + goto out_unlock; + } + bytesout = vfs_write(file_out, data, bytesin, &offout); + if (bytesout < 0) { + pr_err("write error at %lld\n", offout); + err = -EIO; + goto out_unlock; + } + if (bytesin != bytesout) { + /* + * If we fail writing, the recovery is a job for the + * admin. Keep piling things in the memory for now. + */ + export_ok = false; + err = bytesout; + goto out_unlock; + } + } while (bytesin == 512); + ima_free_list(); + +out_unlock: + mutex_unlock(&ima_extend_list_mutex); +out_close: + if (file_in) + filp_close(file_in, NULL); + if (file_out) + filp_close(file_out, NULL); + + path_put(&root); + set_fs(fs); +out_err: + return err; +} + +static ssize_t ima_write_list_name(struct file *filp, + const char __user *buf, + size_t count, loff_t *ppos) +{ + int err; + + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + if ((count <= 1) || (count >= 255)) + return -EINVAL; + + if (*buf != '/') + return -EINVAL; + + mutex_lock(&ima_list_mutex); + if (ima_msmt_list_name) + goto try_export; + + ima_msmt_list_name = kzalloc(count, GFP_KERNEL); + if (!ima_msmt_list_name) { + mutex_unlock(&ima_list_mutex); + return -ENOMEM; + } + err = copy_from_user(ima_msmt_list_name, buf, count); + if (err) { + kfree(ima_msmt_list_name); + ima_msmt_list_name = NULL; + mutex_unlock(&ima_list_mutex); + return -EFAULT; + } + if (ima_msmt_list_name[count-1] == '\n') + ima_msmt_list_name[count-1] = 0; + +try_export: + err = ima_export_list(NULL); + mutex_unlock(&ima_list_mutex); + if (err) { + pr_err("list export failed with %d\n", err); + return -1; + } + return count; +} + +static const struct file_operations ima_list_export_ops = { + .write = ima_write_list_name, +}; + int __init ima_fs_init(void) { ima_dir = securityfs_create_dir("ima", integrity_dir); @@ -493,6 +674,11 @@ int __init ima_fs_init(void) if (IS_ERR(ima_policy)) goto out; + ima_list_name = securityfs_create_file("list_name", 0200, ima_dir, + NULL, &ima_list_export_ops); + if (IS_ERR(ima_list_name)) + goto out; + return 0; out: securityfs_remove(violations); @@ -502,5 +688,7 @@ int __init ima_fs_init(void) securityfs_remove(ima_symlink); securityfs_remove(ima_dir); securityfs_remove(ima_policy); + securityfs_remove(ima_list_name); + return -1; } diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 1ce8b1701566..77c538ec8474 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -44,7 +44,7 @@ struct ima_h_table ima_htable = { * and extending the TPM PCR aggregate. Since tpm_extend can take * long (and the tpm driver uses a mutex), we can't use the spinlock. */ -static DEFINE_MUTEX(ima_extend_list_mutex); +DEFINE_MUTEX(ima_extend_list_mutex); /* lookup up the digest value in the hash table, and return the entry */ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
Some systems can end up carrying lots of entries in the ima measurement list. Since every entry is using a bit of kernel memory, add a new Kconfig variable to allow the sysadmin to define the maximum measurement list size and the location of the exported list. The list is written out in append mode, so the system will keep writing new entries as long as it stays running or runs out of space. File is also automatically truncated on startup. Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com> --- security/integrity/ima/Kconfig | 10 ++ security/integrity/ima/ima.h | 7 +- security/integrity/ima/ima_fs.c | 188 +++++++++++++++++++++++++++++ security/integrity/ima/ima_queue.c | 2 +- 4 files changed, 202 insertions(+), 5 deletions(-)