[RFC] ima: export the measurement list when needed

[RFC] ima: export the measurement list when needed

[Janne Karhunen]

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    | 178 +++++++++++++++++++++++++++++
 security/integrity/ima/ima_queue.c |   2 +-
 4 files changed, 192 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 897bafc59a33..dbfb76c7b347 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 5000
+	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..cc9a8bbb723d 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(void);
 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..d938d3921c43 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[255];
 
 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,169 @@ 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;
+
+	mutex_lock(&ima_extend_list_mutex);
+	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);
+	}
+	mutex_unlock(&ima_extend_list_mutex);
+}
+
+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(void)
+{
+	static bool export_ok = true;
+	static bool init_export = true;
+
+	struct file *file_out = NULL;
+	struct file *file_in = NULL;
+	struct path root;
+	ssize_t bytesin, bytesout;
+	mm_segment_t fs;
+	loff_t offin = 0, offout = 0;
+	char data[512];
+	long htable_len;
+	int err = 0;
+
+	mutex_lock(&ima_list_mutex);
+	htable_len = atomic_long_read(&ima_htable.len);
+	if (htable_len <= CONFIG_IMA_MEASUREMENT_LIST_SIZE)
+		goto out_unlock;
+
+	if (strlen(ima_msmt_list_name) == 0) {
+		err = -ENOENT;
+		goto out_unlock;
+	}
+	if (!export_ok) {
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
+	fs = get_fs();
+	set_fs(KERNEL_DS);
+
+	if (init_export) {
+		pr_info("ima: list size (%ld/%ld) exceeded, exporting to %s\n",
+			htable_len, (long)CONFIG_IMA_MEASUREMENT_LIST_SIZE,
+			ima_msmt_list_name);
+
+		ima_unlink_file(ima_msmt_list_name);
+		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, ima_msmt_list_name,
+				  O_CREAT|O_WRONLY|O_APPEND|O_NOFOLLOW|O_EXCL,
+				  0400);
+	if (IS_ERR(file_out)) {
+		err = PTR_ERR(file_out);
+		goto out;
+	}
+	file_in = file_open_root(root.dentry, root.mnt, secfs_mnt am_filename,
+				 O_RDONLY, 0);
+	if (IS_ERR(file_in)) {
+		err = PTR_ERR(file_in);
+		goto out;
+	}
+	do {
+		bytesin = vfs_read(file_in, data, 512, &offin);
+		if (bytesin < 0) {
+			pr_err("ima: read error at %lld\n", offin);
+			err = -EIO;
+			goto out;
+		}
+		bytesout = vfs_write(file_out, data, bytesin, &offout);
+		if (bytesout < 0) {
+			pr_err("ima: write error at %lld\n", offout);
+			err = -EIO;
+			goto out;
+		}
+		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;
+		}
+	} while (bytesin == 512);
+	ima_free_list();
+
+out:
+	filp_close(file_in, NULL);
+	filp_close(file_out, NULL);
+
+	path_put(&root);
+	set_fs(fs);
+
+out_unlock:
+	mutex_unlock(&ima_list_mutex);
+	return err;
+}
+
+static ssize_t ima_write_list_name(struct file *filp,
+				   const char __user *buf,
+				   size_t count, loff_t *ppos)
+{
+	int err, len;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if ((count <= 1) || (count >= 255))
+		return -EINVAL;
+
+	if (*buf != '/')
+		return -EINVAL;
+
+	if (*ima_msmt_list_name == '/')
+		goto try_export;
+
+	err = copy_from_user(ima_msmt_list_name, buf, count);
+	if (err) {
+		memset(ima_msmt_list_name, 0, 255);
+		return -EFAULT;
+	}
+	len = strnlen(ima_msmt_list_name, 255);
+	if (ima_msmt_list_name[len-1] == '\n')
+		ima_msmt_list_name[len-1] = 0;
+
+try_export:
+	err = ima_export_list();
+	if (err) {
+		pr_err("ima: 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 +664,11 @@ int __init ima_fs_init(void)
 	if (IS_ERR(ima_policy))
 		goto out;
 
+	ima_list_name = securityfs_create_file("list_name", 0600, ima_dir,
+					       NULL, &ima_list_export_ops);
+	if (IS_ERR(ima_list_name))
+		goto out;
+
 	return 0;
 out:
 	securityfs_remove(violations);
@@ -502,5 +678,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,
-- 
2.17.1

Re: [RFC] ima: export the measurement list when needed

[Janne Karhunen]

Hi,

Have in mind that below is the first trial draft that booted and
seemingly accomplished the task once, it was not really tested at all
yet. I will make a polished and tested version if people like the
concept.

Note that the code (almost) supports pushing and pulling of the
entries. This variant is a simple pull given that the list size is
above the defined limits. Pushing can be put in place if the recursion
with the list extend_list_mutex is cleared, maybe this could be done
via another patch later on when we have a workqueue for the export
task? The workqueue might be the best context for the export job since
clearing the list is a heavy operation (and it's not entirely correct
here AFAIK, there is no rcu sync before the template free).


--
Janne

On Wed, Dec 18, 2019 at 2:53 PM 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.
>
> 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    | 178 +++++++++++++++++++++++++++++
>  security/integrity/ima/ima_queue.c |   2 +-
>  4 files changed, 192 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 897bafc59a33..dbfb76c7b347 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 5000
> +       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..cc9a8bbb723d 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(void);
>  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..d938d3921c43 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[255];
>
>  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,169 @@ 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;
> +
> +       mutex_lock(&ima_extend_list_mutex);
> +       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);
> +       }
> +       mutex_unlock(&ima_extend_list_mutex);
> +}
> +
> +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(void)
> +{
> +       static bool export_ok = true;
> +       static bool init_export = true;
> +
> +       struct file *file_out = NULL;
> +       struct file *file_in = NULL;
> +       struct path root;
> +       ssize_t bytesin, bytesout;
> +       mm_segment_t fs;
> +       loff_t offin = 0, offout = 0;
> +       char data[512];
> +       long htable_len;
> +       int err = 0;
> +
> +       mutex_lock(&ima_list_mutex);
> +       htable_len = atomic_long_read(&ima_htable.len);
> +       if (htable_len <= CONFIG_IMA_MEASUREMENT_LIST_SIZE)
> +               goto out_unlock;
> +
> +       if (strlen(ima_msmt_list_name) == 0) {
> +               err = -ENOENT;
> +               goto out_unlock;
> +       }
> +       if (!export_ok) {
> +               err = -EINVAL;
> +               goto out_unlock;
> +       }
> +
> +       fs = get_fs();
> +       set_fs(KERNEL_DS);
> +
> +       if (init_export) {
> +               pr_info("ima: list size (%ld/%ld) exceeded, exporting to %s\n",
> +                       htable_len, (long)CONFIG_IMA_MEASUREMENT_LIST_SIZE,
> +                       ima_msmt_list_name);
> +
> +               ima_unlink_file(ima_msmt_list_name);
> +               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, ima_msmt_list_name,
> +                                 O_CREAT|O_WRONLY|O_APPEND|O_NOFOLLOW|O_EXCL,
> +                                 0400);
> +       if (IS_ERR(file_out)) {
> +               err = PTR_ERR(file_out);
> +               goto out;
> +       }
> +       file_in = file_open_root(root.dentry, root.mnt, secfs_mnt am_filename,
> +                                O_RDONLY, 0);
> +       if (IS_ERR(file_in)) {
> +               err = PTR_ERR(file_in);
> +               goto out;
> +       }
> +       do {
> +               bytesin = vfs_read(file_in, data, 512, &offin);
> +               if (bytesin < 0) {
> +                       pr_err("ima: read error at %lld\n", offin);
> +                       err = -EIO;
> +                       goto out;
> +               }
> +               bytesout = vfs_write(file_out, data, bytesin, &offout);
> +               if (bytesout < 0) {
> +                       pr_err("ima: write error at %lld\n", offout);
> +                       err = -EIO;
> +                       goto out;
> +               }
> +               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;
> +               }
> +       } while (bytesin == 512);
> +       ima_free_list();
> +
> +out:
> +       filp_close(file_in, NULL);
> +       filp_close(file_out, NULL);
> +
> +       path_put(&root);
> +       set_fs(fs);
> +
> +out_unlock:
> +       mutex_unlock(&ima_list_mutex);
> +       return err;
> +}
> +
> +static ssize_t ima_write_list_name(struct file *filp,
> +                                  const char __user *buf,
> +                                  size_t count, loff_t *ppos)
> +{
> +       int err, len;
> +
> +       if (!capable(CAP_SYS_ADMIN))
> +               return -EPERM;
> +
> +       if ((count <= 1) || (count >= 255))
> +               return -EINVAL;
> +
> +       if (*buf != '/')
> +               return -EINVAL;
> +
> +       if (*ima_msmt_list_name == '/')
> +               goto try_export;
> +
> +       err = copy_from_user(ima_msmt_list_name, buf, count);
> +       if (err) {
> +               memset(ima_msmt_list_name, 0, 255);
> +               return -EFAULT;
> +       }
> +       len = strnlen(ima_msmt_list_name, 255);
> +       if (ima_msmt_list_name[len-1] == '\n')
> +               ima_msmt_list_name[len-1] = 0;
> +
> +try_export:
> +       err = ima_export_list();
> +       if (err) {
> +               pr_err("ima: 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 +664,11 @@ int __init ima_fs_init(void)
>         if (IS_ERR(ima_policy))
>                 goto out;
>
> +       ima_list_name = securityfs_create_file("list_name", 0600, ima_dir,
> +                                              NULL, &ima_list_export_ops);
> +       if (IS_ERR(ima_list_name))
> +               goto out;
> +
>         return 0;
>  out:
>         securityfs_remove(violations);
> @@ -502,5 +678,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,
> --
> 2.17.1
>

Re: [RFC] ima: export the measurement list when needed

[Janne Karhunen]

Hi Raphael,

Thanks a lot. I understand it very well that the mm tricks surprise
many. Moreover, I certainly understand that it might be
over-engineering in the case it hits a trap with the mm, but the
approach does have clear benefits in case it works (most importantly
we would never use any kernel memory). Let's see what you come up
with. Thanks again,


--
Janne

On Wed, Sep 2, 2020 at 12:32 AM Raphael Gianotti
<raphgi@linux.microsoft.com> wrote:
>
> Hi Janne,
>
> As I mentioned previously I know very little about the approach you had
> mentioned. I read a little more on it and I think now I understand it a
> little better and yes, it should actually work.
>
>   I will spend some time familiarizing myself with how this would work
> so I can actually compare both approaches myself on a technical level
> and have a more well informed opinion on the matter before moving on
> with either sending an RFC for the first approach (based on your code
> from last year) or starting some work using the new approach you brought up.
>
> Thanks,
>
> -Raphael
>
> On 8/31/2020 11:52 PM, Janne Karhunen wrote:
> > Hi Raphael,
> >
> > It depends how well you make it, if you do it right you would not lose
> > them. If the pagefile has a readable format it should all be safe,
> > right?
> >
> >
> > --
> > Janne
> >
> > On Mon, Aug 31, 2020 at 7:49 PM Raphael Gianotti
> > <raphgi@linux.microsoft.com> wrote:
> >> Hi Janne,
> >>
> >> Thanks for your response, I didn't reply right away as I hadn't used mm
> >> and vmarea via vfs_tmpfile before, so I wanted to read some code to
> >> familiarize myself with it. Correct me if I am misunderstanding the
> >> approach you mentioned, but in it, we would still lose the logs accross
> >> kexec/cold boots as we do today, is that correct? It feels like this
> >> approach would solely solve the issue where we can potentially run out
> >> of memory for ima logs.
> >>
> >> For the original approach, I have a prototype version that I intend to
> >> send as an RFC soon (I will link you and it's based off of your original
> >> RFC from late last year).
> >>
> >> - Raphael
> >>
> >> On 8/26/2020 7:12 AM, Janne Karhunen wrote:
> >>> Hi,
> >>>
> >>> Come to think of it, there could be a MM trap though as I'm not sure
> >>> this has been done before. This new file vmarea would sit in the
> >>> kernel virtual memory area somewhere above the page_offset and the mm
> >>> code might assume that there is no need to look for dirty pages there
> >>> when running the PTE scan. But that shouldn't be more than one line
> >>> patch if that is the only trap..
> >>>
> >>>
> >>> --
> >>> Janne
> >>>
> >>> On Wed, Aug 26, 2020 at 4:40 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> >>>> Hi,
> >>>>
> >>>> Attached a variant of the patch from that time that only does the
> >>>> element free and relies on the userspace reading the data.
> >>>>
> >>>> The reason why I stopped working on this at the time was that below
> >>>> was sufficient for my needs. However, after a discussion between Mimi
> >>>> and myself (based on a suggestion by Amir Goldstein) we realized that
> >>>> we could do something considerably more elegant through vfs_tmpfile.
> >>>> It's also much more work.
> >>>>
> >>>> The way this could probably work the best is if we would implement a
> >>>> new allocator that would pull pages from a vmarea tied to a
> >>>> vfs_tmpfile and the file could be opened by the kernel itself during
> >>>> the ima init. Now if all the measurement list data blobs would be
> >>>> allocated through this allocator the pages would never be permanently
> >>>> resident in the kernel, they would only visit the memory for a while
> >>>> when someone reads the data. If done this way the allocator probably
> >>>> does not even need a 'free' and the mm code would do all the real work
> >>>> pushing the data out.
> >>>>
> >>>> The benefits would be that no-one would ever have to poll from
> >>>> userspace (kernel does not depend on the userspace to work) and we
> >>>> would never OOM because of IMA as long as the filesystem is writable.
> >>>> Also we would never lose any data as long as the file system is
> >>>> functioning.
> >>>>
> >>>> Thoughts?
> >>>>
> >>>>
> >>>> --
> >>>> Janne
> >>>>
> >>>> On Wed, Aug 26, 2020 at 11:14 AM Janne Karhunen
> >>>> <janne.karhunen@gmail.com> wrote:
> >>>>> Hi Raphael,
> >>>>>
> >>>>> Sorry I missed the reply. I'm not working on this right now, feel free
> >>>>> to grab. Please copy me with the results, thank you.
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Janne
> >>>>>
> >>>>> On Tue, Aug 18, 2020 at 12:30 AM Raphael Gianotti
> >>>>> <raphgi@linux.microsoft.com> wrote:
> >>>>>> Hi Janne,
> >>>>>>
> >>>>>> Subject: Re: [RFC] ima: export the measurement list when needed
> >>>>>>> Date: Wed, 18 Dec 2019 17:11:22 +0200
> >>>>>>> From: Janne Karhunen <janne.karhunen@gmail.com>
> >>>>>>> To: linux-integrity@vger.kernel.org, Mimi Zohar <zohar@linux.ibm.com>
> >>>>>>> CC: Ken Goldman <kgold@linux.ibm.com>, david.safford@gmail.com,
> >>>>>>> monty.wiseman@ge.com
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> Have in mind that below is the first trial draft that booted and
> >>>>>>> seemingly accomplished the task once, it was not really tested at all
> >>>>>>> yet. I will make a polished and tested version if people like the
> >>>>>>> concept.
> >>>>>>>
> >>>>>>> Note that the code (almost) supports pushing and pulling of the
> >>>>>>> entries. This variant is a simple pull given that the list size is
> >>>>>>> above the defined limits. Pushing can be put in place if the recursion
> >>>>>>> with the list extend_list_mutex is cleared, maybe this could be done
> >>>>>>> via another patch later on when we have a workqueue for the export
> >>>>>>> task? The workqueue might be the best context for the export job since
> >>>>>>> clearing the list is a heavy operation (and it's not entirely correct
> >>>>>>> here AFAIK, there is no rcu sync before the template free).
> >>>>>>>
> >>>>>>>
> >>>>>>> -- Janne
> >>>>>>>
> >>>>>>> On Wed, Dec 18, 2019 at 2:53 PM 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.
> >>>>>>>>
> >>>>>>>> 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    | 178 +++++++++++++++++++++++++++++
> >>>>>>>>    security/integrity/ima/ima_queue.c |   2 +-
> >>>>>>>>    4 files changed, 192 insertions(+), 5 deletions(-)
> >>>>>> I've been looking into a solution to this same issue you started some
> >>>>>> work on. I was wondering if you are still working on it. I was
> >>>>>> considering taking your initial prototyping on this and extending it
> >>>>>> into a final solution, but I wanted to reply here first and check if you
> >>>>>> are currently working on this.
> >>>>>>

Re: [RFC] ima: export the measurement list when needed

[Raphael Gianotti]

Hi Janne,

As I mentioned previously I know very little about the approach you had 
mentioned. I read a little more on it and I think now I understand it a 
little better and yes, it should actually work.

  I will spend some time familiarizing myself with how this would work 
so I can actually compare both approaches myself on a technical level 
and have a more well informed opinion on the matter before moving on 
with either sending an RFC for the first approach (based on your code 
from last year) or starting some work using the new approach you brought up.

Thanks,

-Raphael

On 8/31/2020 11:52 PM, Janne Karhunen wrote:
> Hi Raphael,
>
> It depends how well you make it, if you do it right you would not lose
> them. If the pagefile has a readable format it should all be safe,
> right?
>
>
> --
> Janne
>
> On Mon, Aug 31, 2020 at 7:49 PM Raphael Gianotti
> <raphgi@linux.microsoft.com> wrote:
>> Hi Janne,
>>
>> Thanks for your response, I didn't reply right away as I hadn't used mm
>> and vmarea via vfs_tmpfile before, so I wanted to read some code to
>> familiarize myself with it. Correct me if I am misunderstanding the
>> approach you mentioned, but in it, we would still lose the logs accross
>> kexec/cold boots as we do today, is that correct? It feels like this
>> approach would solely solve the issue where we can potentially run out
>> of memory for ima logs.
>>
>> For the original approach, I have a prototype version that I intend to
>> send as an RFC soon (I will link you and it's based off of your original
>> RFC from late last year).
>>
>> - Raphael
>>
>> On 8/26/2020 7:12 AM, Janne Karhunen wrote:
>>> Hi,
>>>
>>> Come to think of it, there could be a MM trap though as I'm not sure
>>> this has been done before. This new file vmarea would sit in the
>>> kernel virtual memory area somewhere above the page_offset and the mm
>>> code might assume that there is no need to look for dirty pages there
>>> when running the PTE scan. But that shouldn't be more than one line
>>> patch if that is the only trap..
>>>
>>>
>>> --
>>> Janne
>>>
>>> On Wed, Aug 26, 2020 at 4:40 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
>>>> Hi,
>>>>
>>>> Attached a variant of the patch from that time that only does the
>>>> element free and relies on the userspace reading the data.
>>>>
>>>> The reason why I stopped working on this at the time was that below
>>>> was sufficient for my needs. However, after a discussion between Mimi
>>>> and myself (based on a suggestion by Amir Goldstein) we realized that
>>>> we could do something considerably more elegant through vfs_tmpfile.
>>>> It's also much more work.
>>>>
>>>> The way this could probably work the best is if we would implement a
>>>> new allocator that would pull pages from a vmarea tied to a
>>>> vfs_tmpfile and the file could be opened by the kernel itself during
>>>> the ima init. Now if all the measurement list data blobs would be
>>>> allocated through this allocator the pages would never be permanently
>>>> resident in the kernel, they would only visit the memory for a while
>>>> when someone reads the data. If done this way the allocator probably
>>>> does not even need a 'free' and the mm code would do all the real work
>>>> pushing the data out.
>>>>
>>>> The benefits would be that no-one would ever have to poll from
>>>> userspace (kernel does not depend on the userspace to work) and we
>>>> would never OOM because of IMA as long as the filesystem is writable.
>>>> Also we would never lose any data as long as the file system is
>>>> functioning.
>>>>
>>>> Thoughts?
>>>>
>>>>
>>>> --
>>>> Janne
>>>>
>>>> On Wed, Aug 26, 2020 at 11:14 AM Janne Karhunen
>>>> <janne.karhunen@gmail.com> wrote:
>>>>> Hi Raphael,
>>>>>
>>>>> Sorry I missed the reply. I'm not working on this right now, feel free
>>>>> to grab. Please copy me with the results, thank you.
>>>>>
>>>>>
>>>>> --
>>>>> Janne
>>>>>
>>>>> On Tue, Aug 18, 2020 at 12:30 AM Raphael Gianotti
>>>>> <raphgi@linux.microsoft.com> wrote:
>>>>>> Hi Janne,
>>>>>>
>>>>>> Subject: Re: [RFC] ima: export the measurement list when needed
>>>>>>> Date: Wed, 18 Dec 2019 17:11:22 +0200
>>>>>>> From: Janne Karhunen <janne.karhunen@gmail.com>
>>>>>>> To: linux-integrity@vger.kernel.org, Mimi Zohar <zohar@linux.ibm.com>
>>>>>>> CC: Ken Goldman <kgold@linux.ibm.com>, david.safford@gmail.com,
>>>>>>> monty.wiseman@ge.com
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Have in mind that below is the first trial draft that booted and
>>>>>>> seemingly accomplished the task once, it was not really tested at all
>>>>>>> yet. I will make a polished and tested version if people like the
>>>>>>> concept.
>>>>>>>
>>>>>>> Note that the code (almost) supports pushing and pulling of the
>>>>>>> entries. This variant is a simple pull given that the list size is
>>>>>>> above the defined limits. Pushing can be put in place if the recursion
>>>>>>> with the list extend_list_mutex is cleared, maybe this could be done
>>>>>>> via another patch later on when we have a workqueue for the export
>>>>>>> task? The workqueue might be the best context for the export job since
>>>>>>> clearing the list is a heavy operation (and it's not entirely correct
>>>>>>> here AFAIK, there is no rcu sync before the template free).
>>>>>>>
>>>>>>>
>>>>>>> -- Janne
>>>>>>>
>>>>>>> On Wed, Dec 18, 2019 at 2:53 PM 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.
>>>>>>>>
>>>>>>>> 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    | 178 +++++++++++++++++++++++++++++
>>>>>>>>    security/integrity/ima/ima_queue.c |   2 +-
>>>>>>>>    4 files changed, 192 insertions(+), 5 deletions(-)
>>>>>> I've been looking into a solution to this same issue you started some
>>>>>> work on. I was wondering if you are still working on it. I was
>>>>>> considering taking your initial prototyping on this and extending it
>>>>>> into a final solution, but I wanted to reply here first and check if you
>>>>>> are currently working on this.
>>>>>>

Re: [RFC] ima: export the measurement list when needed

[Janne Karhunen]

Hi Raphael,

It depends how well you make it, if you do it right you would not lose
them. If the pagefile has a readable format it should all be safe,
right?


--
Janne

On Mon, Aug 31, 2020 at 7:49 PM Raphael Gianotti
<raphgi@linux.microsoft.com> wrote:
>
> Hi Janne,
>
> Thanks for your response, I didn't reply right away as I hadn't used mm
> and vmarea via vfs_tmpfile before, so I wanted to read some code to
> familiarize myself with it. Correct me if I am misunderstanding the
> approach you mentioned, but in it, we would still lose the logs accross
> kexec/cold boots as we do today, is that correct? It feels like this
> approach would solely solve the issue where we can potentially run out
> of memory for ima logs.
>
> For the original approach, I have a prototype version that I intend to
> send as an RFC soon (I will link you and it's based off of your original
> RFC from late last year).
>
> - Raphael
>
> On 8/26/2020 7:12 AM, Janne Karhunen wrote:
> > Hi,
> >
> > Come to think of it, there could be a MM trap though as I'm not sure
> > this has been done before. This new file vmarea would sit in the
> > kernel virtual memory area somewhere above the page_offset and the mm
> > code might assume that there is no need to look for dirty pages there
> > when running the PTE scan. But that shouldn't be more than one line
> > patch if that is the only trap..
> >
> >
> > --
> > Janne
> >
> > On Wed, Aug 26, 2020 at 4:40 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> >> Hi,
> >>
> >> Attached a variant of the patch from that time that only does the
> >> element free and relies on the userspace reading the data.
> >>
> >> The reason why I stopped working on this at the time was that below
> >> was sufficient for my needs. However, after a discussion between Mimi
> >> and myself (based on a suggestion by Amir Goldstein) we realized that
> >> we could do something considerably more elegant through vfs_tmpfile.
> >> It's also much more work.
> >>
> >> The way this could probably work the best is if we would implement a
> >> new allocator that would pull pages from a vmarea tied to a
> >> vfs_tmpfile and the file could be opened by the kernel itself during
> >> the ima init. Now if all the measurement list data blobs would be
> >> allocated through this allocator the pages would never be permanently
> >> resident in the kernel, they would only visit the memory for a while
> >> when someone reads the data. If done this way the allocator probably
> >> does not even need a 'free' and the mm code would do all the real work
> >> pushing the data out.
> >>
> >> The benefits would be that no-one would ever have to poll from
> >> userspace (kernel does not depend on the userspace to work) and we
> >> would never OOM because of IMA as long as the filesystem is writable.
> >> Also we would never lose any data as long as the file system is
> >> functioning.
> >>
> >> Thoughts?
> >>
> >>
> >> --
> >> Janne
> >>
> >> On Wed, Aug 26, 2020 at 11:14 AM Janne Karhunen
> >> <janne.karhunen@gmail.com> wrote:
> >>> Hi Raphael,
> >>>
> >>> Sorry I missed the reply. I'm not working on this right now, feel free
> >>> to grab. Please copy me with the results, thank you.
> >>>
> >>>
> >>> --
> >>> Janne
> >>>
> >>> On Tue, Aug 18, 2020 at 12:30 AM Raphael Gianotti
> >>> <raphgi@linux.microsoft.com> wrote:
> >>>>
> >>>> Hi Janne,
> >>>>
> >>>> Subject: Re: [RFC] ima: export the measurement list when needed
> >>>>> Date: Wed, 18 Dec 2019 17:11:22 +0200
> >>>>> From: Janne Karhunen <janne.karhunen@gmail.com>
> >>>>> To: linux-integrity@vger.kernel.org, Mimi Zohar <zohar@linux.ibm.com>
> >>>>> CC: Ken Goldman <kgold@linux.ibm.com>, david.safford@gmail.com,
> >>>>> monty.wiseman@ge.com
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> Have in mind that below is the first trial draft that booted and
> >>>>> seemingly accomplished the task once, it was not really tested at all
> >>>>> yet. I will make a polished and tested version if people like the
> >>>>> concept.
> >>>>>
> >>>>> Note that the code (almost) supports pushing and pulling of the
> >>>>> entries. This variant is a simple pull given that the list size is
> >>>>> above the defined limits. Pushing can be put in place if the recursion
> >>>>> with the list extend_list_mutex is cleared, maybe this could be done
> >>>>> via another patch later on when we have a workqueue for the export
> >>>>> task? The workqueue might be the best context for the export job since
> >>>>> clearing the list is a heavy operation (and it's not entirely correct
> >>>>> here AFAIK, there is no rcu sync before the template free).
> >>>>>
> >>>>>
> >>>>> -- Janne
> >>>>>
> >>>>> On Wed, Dec 18, 2019 at 2:53 PM 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.
> >>>>>>
> >>>>>> 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    | 178 +++++++++++++++++++++++++++++
> >>>>>>   security/integrity/ima/ima_queue.c |   2 +-
> >>>>>>   4 files changed, 192 insertions(+), 5 deletions(-)
> >>>> I've been looking into a solution to this same issue you started some
> >>>> work on. I was wondering if you are still working on it. I was
> >>>> considering taking your initial prototyping on this and extending it
> >>>> into a final solution, but I wanted to reply here first and check if you
> >>>> are currently working on this.
> >>>>

Re: [RFC] ima: export the measurement list when needed

[Raphael Gianotti]

Hi Janne,

Thanks for your response, I didn't reply right away as I hadn't used mm 
and vmarea via vfs_tmpfile before, so I wanted to read some code to 
familiarize myself with it. Correct me if I am misunderstanding the 
approach you mentioned, but in it, we would still lose the logs accross 
kexec/cold boots as we do today, is that correct? It feels like this 
approach would solely solve the issue where we can potentially run out 
of memory for ima logs.

For the original approach, I have a prototype version that I intend to 
send as an RFC soon (I will link you and it's based off of your original 
RFC from late last year).

- Raphael

On 8/26/2020 7:12 AM, Janne Karhunen wrote:
> Hi,
>
> Come to think of it, there could be a MM trap though as I'm not sure
> this has been done before. This new file vmarea would sit in the
> kernel virtual memory area somewhere above the page_offset and the mm
> code might assume that there is no need to look for dirty pages there
> when running the PTE scan. But that shouldn't be more than one line
> patch if that is the only trap..
>
>
> --
> Janne
>
> On Wed, Aug 26, 2020 at 4:40 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
>> Hi,
>>
>> Attached a variant of the patch from that time that only does the
>> element free and relies on the userspace reading the data.
>>
>> The reason why I stopped working on this at the time was that below
>> was sufficient for my needs. However, after a discussion between Mimi
>> and myself (based on a suggestion by Amir Goldstein) we realized that
>> we could do something considerably more elegant through vfs_tmpfile.
>> It's also much more work.
>>
>> The way this could probably work the best is if we would implement a
>> new allocator that would pull pages from a vmarea tied to a
>> vfs_tmpfile and the file could be opened by the kernel itself during
>> the ima init. Now if all the measurement list data blobs would be
>> allocated through this allocator the pages would never be permanently
>> resident in the kernel, they would only visit the memory for a while
>> when someone reads the data. If done this way the allocator probably
>> does not even need a 'free' and the mm code would do all the real work
>> pushing the data out.
>>
>> The benefits would be that no-one would ever have to poll from
>> userspace (kernel does not depend on the userspace to work) and we
>> would never OOM because of IMA as long as the filesystem is writable.
>> Also we would never lose any data as long as the file system is
>> functioning.
>>
>> Thoughts?
>>
>>
>> --
>> Janne
>>
>> On Wed, Aug 26, 2020 at 11:14 AM Janne Karhunen
>> <janne.karhunen@gmail.com> wrote:
>>> Hi Raphael,
>>>
>>> Sorry I missed the reply. I'm not working on this right now, feel free
>>> to grab. Please copy me with the results, thank you.
>>>
>>>
>>> --
>>> Janne
>>>
>>> On Tue, Aug 18, 2020 at 12:30 AM Raphael Gianotti
>>> <raphgi@linux.microsoft.com> wrote:
>>>>
>>>> Hi Janne,
>>>>
>>>> Subject: Re: [RFC] ima: export the measurement list when needed
>>>>> Date: Wed, 18 Dec 2019 17:11:22 +0200
>>>>> From: Janne Karhunen <janne.karhunen@gmail.com>
>>>>> To: linux-integrity@vger.kernel.org, Mimi Zohar <zohar@linux.ibm.com>
>>>>> CC: Ken Goldman <kgold@linux.ibm.com>, david.safford@gmail.com,
>>>>> monty.wiseman@ge.com
>>>>>
>>>>> Hi,
>>>>>
>>>>> Have in mind that below is the first trial draft that booted and
>>>>> seemingly accomplished the task once, it was not really tested at all
>>>>> yet. I will make a polished and tested version if people like the
>>>>> concept.
>>>>>
>>>>> Note that the code (almost) supports pushing and pulling of the
>>>>> entries. This variant is a simple pull given that the list size is
>>>>> above the defined limits. Pushing can be put in place if the recursion
>>>>> with the list extend_list_mutex is cleared, maybe this could be done
>>>>> via another patch later on when we have a workqueue for the export
>>>>> task? The workqueue might be the best context for the export job since
>>>>> clearing the list is a heavy operation (and it's not entirely correct
>>>>> here AFAIK, there is no rcu sync before the template free).
>>>>>
>>>>>
>>>>> -- Janne
>>>>>
>>>>> On Wed, Dec 18, 2019 at 2:53 PM 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.
>>>>>>
>>>>>> 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    | 178 +++++++++++++++++++++++++++++
>>>>>>   security/integrity/ima/ima_queue.c |   2 +-
>>>>>>   4 files changed, 192 insertions(+), 5 deletions(-)
>>>> I've been looking into a solution to this same issue you started some
>>>> work on. I was wondering if you are still working on it. I was
>>>> considering taking your initial prototyping on this and extending it
>>>> into a final solution, but I wanted to reply here first and check if you
>>>> are currently working on this.
>>>>

Re: [RFC] ima: export the measurement list when needed

[Janne Karhunen]

Hi,

Come to think of it, there could be a MM trap though as I'm not sure
this has been done before. This new file vmarea would sit in the
kernel virtual memory area somewhere above the page_offset and the mm
code might assume that there is no need to look for dirty pages there
when running the PTE scan. But that shouldn't be more than one line
patch if that is the only trap..


--
Janne

On Wed, Aug 26, 2020 at 4:40 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> Hi,
>
> Attached a variant of the patch from that time that only does the
> element free and relies on the userspace reading the data.
>
> The reason why I stopped working on this at the time was that below
> was sufficient for my needs. However, after a discussion between Mimi
> and myself (based on a suggestion by Amir Goldstein) we realized that
> we could do something considerably more elegant through vfs_tmpfile.
> It's also much more work.
>
> The way this could probably work the best is if we would implement a
> new allocator that would pull pages from a vmarea tied to a
> vfs_tmpfile and the file could be opened by the kernel itself during
> the ima init. Now if all the measurement list data blobs would be
> allocated through this allocator the pages would never be permanently
> resident in the kernel, they would only visit the memory for a while
> when someone reads the data. If done this way the allocator probably
> does not even need a 'free' and the mm code would do all the real work
> pushing the data out.
>
> The benefits would be that no-one would ever have to poll from
> userspace (kernel does not depend on the userspace to work) and we
> would never OOM because of IMA as long as the filesystem is writable.
> Also we would never lose any data as long as the file system is
> functioning.
>
> Thoughts?
>
>
> --
> Janne
>
> On Wed, Aug 26, 2020 at 11:14 AM Janne Karhunen
> <janne.karhunen@gmail.com> wrote:
> >
> > Hi Raphael,
> >
> > Sorry I missed the reply. I'm not working on this right now, feel free
> > to grab. Please copy me with the results, thank you.
> >
> >
> > --
> > Janne
> >
> > On Tue, Aug 18, 2020 at 12:30 AM Raphael Gianotti
> > <raphgi@linux.microsoft.com> wrote:
> > >
> > >
> > > Hi Janne,
> > >
> > > Subject: Re: [RFC] ima: export the measurement list when needed
> > > > Date: Wed, 18 Dec 2019 17:11:22 +0200
> > > > From: Janne Karhunen <janne.karhunen@gmail.com>
> > > > To: linux-integrity@vger.kernel.org, Mimi Zohar <zohar@linux.ibm.com>
> > > > CC: Ken Goldman <kgold@linux.ibm.com>, david.safford@gmail.com,
> > > > monty.wiseman@ge.com
> > > >
> > > > Hi,
> > > >
> > > > Have in mind that below is the first trial draft that booted and
> > > > seemingly accomplished the task once, it was not really tested at all
> > > > yet. I will make a polished and tested version if people like the
> > > > concept.
> > > >
> > > > Note that the code (almost) supports pushing and pulling of the
> > > > entries. This variant is a simple pull given that the list size is
> > > > above the defined limits. Pushing can be put in place if the recursion
> > > > with the list extend_list_mutex is cleared, maybe this could be done
> > > > via another patch later on when we have a workqueue for the export
> > > > task? The workqueue might be the best context for the export job since
> > > > clearing the list is a heavy operation (and it's not entirely correct
> > > > here AFAIK, there is no rcu sync before the template free).
> > > >
> > > >
> > > > -- Janne
> > > >
> > > > On Wed, Dec 18, 2019 at 2:53 PM 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.
> > > >>
> > > >> 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    | 178 +++++++++++++++++++++++++++++
> > > >>  security/integrity/ima/ima_queue.c |   2 +-
> > > >>  4 files changed, 192 insertions(+), 5 deletions(-)
> > >
> > > I've been looking into a solution to this same issue you started some
> > > work on. I was wondering if you are still working on it. I was
> > > considering taking your initial prototyping on this and extending it
> > > into a final solution, but I wanted to reply here first and check if you
> > > are currently working on this.
> > >

Re: [RFC] ima: export the measurement list when needed

[Janne Karhunen]

[-- Attachment #1: Type: text/plain, Size: 4202 bytes --]

Hi,

Attached a variant of the patch from that time that only does the
element free and relies on the userspace reading the data.

The reason why I stopped working on this at the time was that below
was sufficient for my needs. However, after a discussion between Mimi
and myself (based on a suggestion by Amir Goldstein) we realized that
we could do something considerably more elegant through vfs_tmpfile.
It's also much more work.

The way this could probably work the best is if we would implement a
new allocator that would pull pages from a vmarea tied to a
vfs_tmpfile and the file could be opened by the kernel itself during
the ima init. Now if all the measurement list data blobs would be
allocated through this allocator the pages would never be permanently
resident in the kernel, they would only visit the memory for a while
when someone reads the data. If done this way the allocator probably
does not even need a 'free' and the mm code would do all the real work
pushing the data out.

The benefits would be that no-one would ever have to poll from
userspace (kernel does not depend on the userspace to work) and we
would never OOM because of IMA as long as the filesystem is writable.
Also we would never lose any data as long as the file system is
functioning.

Thoughts?


--
Janne

On Wed, Aug 26, 2020 at 11:14 AM Janne Karhunen
<janne.karhunen@gmail.com> wrote:
>
> Hi Raphael,
>
> Sorry I missed the reply. I'm not working on this right now, feel free
> to grab. Please copy me with the results, thank you.
>
>
> --
> Janne
>
> On Tue, Aug 18, 2020 at 12:30 AM Raphael Gianotti
> <raphgi@linux.microsoft.com> wrote:
> >
> >
> > Hi Janne,
> >
> > Subject: Re: [RFC] ima: export the measurement list when needed
> > > Date: Wed, 18 Dec 2019 17:11:22 +0200
> > > From: Janne Karhunen <janne.karhunen@gmail.com>
> > > To: linux-integrity@vger.kernel.org, Mimi Zohar <zohar@linux.ibm.com>
> > > CC: Ken Goldman <kgold@linux.ibm.com>, david.safford@gmail.com,
> > > monty.wiseman@ge.com
> > >
> > > Hi,
> > >
> > > Have in mind that below is the first trial draft that booted and
> > > seemingly accomplished the task once, it was not really tested at all
> > > yet. I will make a polished and tested version if people like the
> > > concept.
> > >
> > > Note that the code (almost) supports pushing and pulling of the
> > > entries. This variant is a simple pull given that the list size is
> > > above the defined limits. Pushing can be put in place if the recursion
> > > with the list extend_list_mutex is cleared, maybe this could be done
> > > via another patch later on when we have a workqueue for the export
> > > task? The workqueue might be the best context for the export job since
> > > clearing the list is a heavy operation (and it's not entirely correct
> > > here AFAIK, there is no rcu sync before the template free).
> > >
> > >
> > > -- Janne
> > >
> > > On Wed, Dec 18, 2019 at 2:53 PM 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.
> > >>
> > >> 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    | 178 +++++++++++++++++++++++++++++
> > >>  security/integrity/ima/ima_queue.c |   2 +-
> > >>  4 files changed, 192 insertions(+), 5 deletions(-)
> >
> > I've been looking into a solution to this same issue you started some
> > work on. I was wondering if you are still working on it. I was
> > considering taking your initial prototyping on this and extending it
> > into a final solution, but I wanted to reply here first and check if you
> > are currently working on this.
> >

[-- Attachment #2: 0001-ima-allow-the-measurement-list-flush.patch --]
[-- Type: text/x-patch, Size: 4869 bytes --]

From be120a15c8d81655bf112334e3279b19415c2ca2 Mon Sep 17 00:00:00 2001
From: Janne Karhunen <janne.karhunen@gmail.com>
Date: Mon, 16 Dec 2019 12:59:30 +0200
Subject: [PATCH v3] ima: allow the measurement list flush

Some systems can end up carrying lots of entries in the ima
measurement list. Since every entry is using a bit of kernel
memory, allow the sysadmin to free any amount of entries and
handle the list export manually.

Changelog:
v3:
- Depend on the userspace doing the file export, only allow
  the list flush.

Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
 security/integrity/ima/ima.h       |  6 +--
 security/integrity/ima/ima_fs.c    | 65 ++++++++++++++++++++++++++++++
 security/integrity/ima/ima_queue.c |  2 +-
 3 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 64317d95363e..821a0f1d832f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -153,6 +153,7 @@ 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);
@@ -163,10 +164,7 @@ 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 a71e822a6e92..104f828b9444 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -360,6 +360,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_flush_measurements;
 
 enum ima_fs_flags {
 	IMA_FS_BUSY,
@@ -447,6 +448,62 @@ static const struct file_operations ima_measure_policy_ops = {
 	.llseek = generic_file_llseek,
 };
 
+static long ima_free_entries(long ec)
+{
+	struct ima_queue_entry *qe, *e;
+	long c;
+
+	c = atomic_long_read(&ima_htable.len);
+	if (ec > c)
+		return -EINVAL;
+
+	c = 0;
+	mutex_lock(&ima_extend_list_mutex);
+	list_for_each_entry_safe(qe, e, &ima_measurements, later) {
+		if (c++ > ec)
+			break;
+
+		hlist_del_rcu(&qe->hnext);
+		atomic_long_dec(&ima_htable.len);
+
+		list_del_rcu(&qe->later);
+		ima_free_template_entry(qe->entry);
+		kfree(qe);
+	}
+	mutex_unlock(&ima_extend_list_mutex);
+	return c;
+}
+
+static ssize_t ima_flush_write(struct file *filp,
+			       const char __user *buf,
+			       size_t count, loff_t *ppos)
+{
+	char anum[32];
+	long num;
+	int err;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+	if (!count || (count >= 32))
+		return -EINVAL;
+	memset(anum, 0, 32);
+
+	err = copy_from_user(anum, buf, count);
+	if (err)
+		return err;
+	if (anum[count-1] == '\n')
+		anum[count-1] = 0;
+	err = kstrtol(anum, 0, &num);
+	if (err)
+		return err;
+
+	return ima_free_entries(num);
+}
+
+static const struct file_operations ima_flush_ops = {
+	.write = ima_flush_write,
+};
+
 int __init ima_fs_init(void)
 {
 	ima_dir = securityfs_create_dir("ima", integrity_dir);
@@ -491,6 +548,12 @@ int __init ima_fs_init(void)
 	if (IS_ERR(ima_policy))
 		goto out;
 
+	ima_flush_measurements =
+	    securityfs_create_file("flush_measurements", 0200, ima_dir,
+				   NULL, &ima_flush_ops);
+	if (IS_ERR(ima_flush_measurements))
+		goto out;
+
 	return 0;
 out:
 	securityfs_remove(violations);
@@ -500,5 +563,7 @@ int __init ima_fs_init(void)
 	securityfs_remove(ima_symlink);
 	securityfs_remove(ima_dir);
 	securityfs_remove(ima_policy);
+	securityfs_remove(ima_flush_measurements);
+
 	return -1;
 }
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 8753212ddb18..4eee0bbf2647 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -42,7 +42,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,
-- 
2.17.1

Re: [RFC] ima: export the measurement list when needed

[Janne Karhunen]

Hi Raphael,

Sorry I missed the reply. I'm not working on this right now, feel free
to grab. Please copy me with the results, thank you.


--
Janne

On Tue, Aug 18, 2020 at 12:30 AM Raphael Gianotti
<raphgi@linux.microsoft.com> wrote:
>
>
> Hi Janne,
>
> Subject: Re: [RFC] ima: export the measurement list when needed
> > Date: Wed, 18 Dec 2019 17:11:22 +0200
> > From: Janne Karhunen <janne.karhunen@gmail.com>
> > To: linux-integrity@vger.kernel.org, Mimi Zohar <zohar@linux.ibm.com>
> > CC: Ken Goldman <kgold@linux.ibm.com>, david.safford@gmail.com,
> > monty.wiseman@ge.com
> >
> > Hi,
> >
> > Have in mind that below is the first trial draft that booted and
> > seemingly accomplished the task once, it was not really tested at all
> > yet. I will make a polished and tested version if people like the
> > concept.
> >
> > Note that the code (almost) supports pushing and pulling of the
> > entries. This variant is a simple pull given that the list size is
> > above the defined limits. Pushing can be put in place if the recursion
> > with the list extend_list_mutex is cleared, maybe this could be done
> > via another patch later on when we have a workqueue for the export
> > task? The workqueue might be the best context for the export job since
> > clearing the list is a heavy operation (and it's not entirely correct
> > here AFAIK, there is no rcu sync before the template free).
> >
> >
> > -- Janne
> >
> > On Wed, Dec 18, 2019 at 2:53 PM 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.
> >>
> >> 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    | 178 +++++++++++++++++++++++++++++
> >>  security/integrity/ima/ima_queue.c |   2 +-
> >>  4 files changed, 192 insertions(+), 5 deletions(-)
>
> I've been looking into a solution to this same issue you started some
> work on. I was wondering if you are still working on it. I was
> considering taking your initial prototyping on this and extending it
> into a final solution, but I wanted to reply here first and check if you
> are currently working on this.
>

Re: [RFC] ima: export the measurement list when needed

[Raphael Gianotti]

Hi Janne,

Subject: Re: [RFC] ima: export the measurement list when needed
> Date: Wed, 18 Dec 2019 17:11:22 +0200
> From: Janne Karhunen <janne.karhunen@gmail.com>
> To: linux-integrity@vger.kernel.org, Mimi Zohar <zohar@linux.ibm.com>
> CC: Ken Goldman <kgold@linux.ibm.com>, david.safford@gmail.com, 
> monty.wiseman@ge.com
>
> Hi,
>
> Have in mind that below is the first trial draft that booted and
> seemingly accomplished the task once, it was not really tested at all
> yet. I will make a polished and tested version if people like the
> concept.
>
> Note that the code (almost) supports pushing and pulling of the
> entries. This variant is a simple pull given that the list size is
> above the defined limits. Pushing can be put in place if the recursion
> with the list extend_list_mutex is cleared, maybe this could be done
> via another patch later on when we have a workqueue for the export
> task? The workqueue might be the best context for the export job since
> clearing the list is a heavy operation (and it's not entirely correct
> here AFAIK, there is no rcu sync before the template free).
>
>
> -- Janne
>
> On Wed, Dec 18, 2019 at 2:53 PM 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.
>>
>> 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    | 178 +++++++++++++++++++++++++++++
>>  security/integrity/ima/ima_queue.c |   2 +-
>>  4 files changed, 192 insertions(+), 5 deletions(-)

I've been looking into a solution to this same issue you started some 
work on. I was wondering if you are still working on it. I was 
considering taking your initial prototyping on this and extending it 
into a final solution, but I wanted to reply here first and check if you 
are currently working on this.