Re: [PATCH v5 04/16] ima: Move delayed work queue and variables into ima_namespace

[Christian Brauner]

On Wed, Dec 08, 2021 at 05:18:06PM -0500, Stefan Berger wrote:
> Move the delayed work queue and associated variables to the
> ima_namespace and initialize them.
> 
> Since keys queued up for measurement currently are only relevant in the
> init_ima_ns, call ima_init_key_queue() only when the init_ima_ns is
> initialized.
> 
> Protect the ima_namespace when scheduling the delayed work by taking an
> additional reference to its user namespace. Put the reference when either
> the delayed work has completed or when it was cancelled but hadn't run.
> 
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> ---
>  include/linux/ima.h                      | 11 +++++++
>  security/integrity/ima/ima.h             | 12 ++++---
>  security/integrity/ima/ima_fs.c          |  4 ++-
>  security/integrity/ima/ima_init.c        |  2 --
>  security/integrity/ima/ima_init_ima_ns.c |  8 +++++
>  security/integrity/ima/ima_policy.c      |  4 +--
>  security/integrity/ima/ima_queue_keys.c  | 42 +++++++++++++-----------
>  7 files changed, 53 insertions(+), 30 deletions(-)
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 9f6de36240b0..529defe4d272 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -217,6 +217,17 @@ struct ima_namespace {
>  	struct rb_root ns_status_tree;
>  	rwlock_t ns_status_lock;
>  	struct kmem_cache *ns_status_cache;
> +
> +#ifdef CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS
> +	/*
> +	 * If custom IMA policy is not loaded then keys queued up
> +	 * for measurement should be freed. This worker is used
> +	 * for handling this scenario.
> +	 */
> +	struct delayed_work ima_keys_delayed_work;
> +	long ima_key_queue_timeout;
> +	bool timer_expired;
> +#endif
>  };
>  
>  extern struct ima_namespace init_ima_ns;
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index dd06e16c4e1c..9edab9050dc7 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -77,6 +77,8 @@ struct ima_field_data {
>  	u32 len;
>  };
>  
> +struct ima_namespace;
> +
>  /* IMA template field definition */
>  struct ima_template_field {
>  	const char field_id[IMA_TEMPLATE_FIELD_ID_MAX_LEN];
> @@ -247,18 +249,18 @@ struct ima_key_entry {
>  	size_t payload_len;
>  	char *keyring_name;
>  };
> -void ima_init_key_queue(void);
> +void ima_init_key_queue(struct ima_namespace *ns);
>  bool ima_should_queue_key(void);
>  bool ima_queue_key(struct key *keyring, const void *payload,
>  		   size_t payload_len);
> -void ima_process_queued_keys(void);
> +void ima_process_queued_keys(struct ima_namespace *ns);
> +void ima_keys_handler(struct work_struct *work);
>  #else
> -static inline void ima_init_key_queue(void) {}
>  static inline bool ima_should_queue_key(void) { return false; }
>  static inline bool ima_queue_key(struct key *keyring,
>  				 const void *payload,
>  				 size_t payload_len) { return false; }
> -static inline void ima_process_queued_keys(void) {}
> +static inline void ima_process_queued_keys(struct ima_namespace *ns) {}
>  #endif /* CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS */
>  
>  /* LIM API function definitions */
> @@ -300,7 +302,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>  		     struct ima_template_desc **template_desc,
>  		     const char *func_data, unsigned int *allowed_algos);
>  void ima_init_policy(void);
> -void ima_update_policy(void);
> +void ima_update_policy(struct ima_namespace *ns);
>  void ima_update_policy_flags(void);
>  ssize_t ima_parse_add_rule(char *);
>  void ima_delete_rules(void);
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 3d8e9d5db5aa..5cff3d6c3dc7 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -21,6 +21,7 @@
>  #include <linux/rcupdate.h>
>  #include <linux/parser.h>
>  #include <linux/vmalloc.h>
> +#include <linux/ima.h>
>  
>  #include "ima.h"
>  
> @@ -410,6 +411,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
>  static int ima_release_policy(struct inode *inode, struct file *file)
>  {
>  	const char *cause = valid_policy ? "completed" : "failed";
> +	struct ima_namespace *ns = get_current_ns();
>  
>  	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
>  		return seq_release(inode, file);
> @@ -430,7 +432,7 @@ static int ima_release_policy(struct inode *inode, struct file *file)
>  		return 0;
>  	}
>  
> -	ima_update_policy();
> +	ima_update_policy(ns);
>  #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
>  	securityfs_remove(ima_policy);
>  	ima_policy = NULL;
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index f6ae4557a0da..24848373a061 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -155,8 +155,6 @@ int __init ima_init(void)
>  	if (rc != 0)
>  		return rc;
>  
> -	ima_init_key_queue();
> -
>  	ima_measure_critical_data("kernel_info", "kernel_version",
>  				  UTS_RELEASE, strlen(UTS_RELEASE), false,
>  				  NULL, 0);
> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
> index 64777377664b..75ef17d52b5b 100644
> --- a/security/integrity/ima/ima_init_ima_ns.c
> +++ b/security/integrity/ima/ima_init_ima_ns.c
> @@ -26,6 +26,14 @@ int ima_init_namespace(struct ima_namespace *ns)
>  	if (!ns->ns_status_cache)
>  		return -ENOMEM;
>  
> +#ifdef CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS
> +	INIT_DELAYED_WORK(&ns->ima_keys_delayed_work, ima_keys_handler);
> +	ns->ima_key_queue_timeout = 300000;
> +	ns->timer_expired = false;
> +	if (ns == &init_ima_ns)
> +		ima_init_key_queue(ns);

The refcounting seems wrong?
ima_init_key_queue() only takes a reference for init_ima_ns and
consequently on init_user_ns (which is a bit pointless since it can't go
away and so can't init_ima_ns).

In contrast non-init_ima_ns will not take a reference on their user_ns.
But ima_keys_handler() always puts the refcount for user_ns for both
non-init_ima_ns and init_ima_ns alike.

Maybe I'm misreading this.

In your earlier mail in [1] you said:

> > The only problem that I see where we are accessing the IMA namespace outside a
> > process context is in 4/16 'ima: Move delayed work queue and variables into
> > ima_namespace' where a delayed work queue is used. I fixed this now by getting

So we seem to know that ima always accesses ima_ns from
current_user_ns() and only in the workqueue case will it delay key
processing for a specific ima namespace without walking a userns
hierarchy.

If that's the case we should remove the user_ns member from ima_ns and
enforce that ima_ns is always accessed from current_user_ns().

Since the workqueue case luckily doesn't need access to user_ns anywhere
we can add a workqueue specific refcount that only keeps it alive for
the workqueue case. We just need to enforce that when the refcount is
bumped for the workqeue it must be done from process context so we're
guaranteed that when we bump the reference the user_ns and consequently
the ima_ns is still alive.

This should solve your lifetime issues (once you fixed the problem I
pointed out above).

(Btw, the kref member was unused before my patch. It didn't really do
any lifetime management for ima_ns afaict.)

[1]: https://lore.kernel.org/lkml/60fa585b-984e-fa13-e76f-56083a726259@xxxxxxxxxxxxx

Here's a sketch neither compile nor runtime tested and without the
refcount issues I pointed out above fixed: