Messages in this thread | | | Date | Thu, 9 Dec 2021 10:09:33 -0500 | Subject | Re: [PATCH v5 04/16] ima: Move delayed work queue and variables into ima_namespace | From | Stefan Berger <> |
| |
On 12/9/21 08:11, Christian Brauner wrote: > 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@linux.ibm.com> >> --- >> 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.
My changes aren't correct and I have to roll this back. The correct solution would be to cancel the delayed work when deleting the ima_namespace [cancel_delayed_work_sync()]. I hadn't implemented that since currently there's not delayed work in non-init_ima_ns running. Any release of a last reference, as implemented by the modifications to this patch, could also delete the delayed_work structure and this would likely crash the system shortly after (although the delayed freeing behavior of the user namespace may save us from this).
I apologize for the confusion.
> > 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().
I will do that, yes.
> 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.
That won't work due to the work_queue getting freed. I will revert the modifications to this patch.
> > 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.)
True. I will remove it.
> > [1]: https://lore.kernel.org/lkml/60fa585b-984e-fa13-e76f-56083a726259@linux.ibm.com > > Here's a sketch neither compile nor runtime tested and without the > refcount issues I pointed out above fixed: > > From 130e8d3faaad42820040587eff8695027fcf062a Mon Sep 17 00:00:00 2001 > From: Christian Brauner <christian.brauner@ubuntu.com> > Date: Thu, 9 Dec 2021 13:15:49 +0100 > Subject: [PATCH] !!!! HERE BE DRAGONS - UNFIXED REFCOUNT ISSUES FROM PREVIOUS > PATCH AND ALL UNTESTED !!!! > > ima: get rid of user_ns member in struct ima_namespace > --- > include/linux/ima.h | 40 +++++++++--------------- > security/integrity/ima/ima_fs.c | 2 +- > security/integrity/ima/ima_init_ima_ns.c | 3 +- > security/integrity/ima/ima_main.c | 2 +- > security/integrity/ima/ima_ns.c | 9 ++---- > security/integrity/ima/ima_queue_keys.c | 22 +++++++++---- > 6 files changed, 36 insertions(+), 42 deletions(-) > > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 32bf98092143..73cdfbf3f9d4 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -13,6 +13,7 @@ > #include <linux/kexec.h> > #include <linux/user_namespace.h> > #include <crypto/hash_info.h> > +#include <linux/refcount.h> > struct linux_binprm; > > #ifdef CONFIG_IMA > @@ -241,8 +242,6 @@ enum { > }; > > struct ima_namespace { > - struct kref kref; > - struct user_namespace *user_ns; > struct rb_root ns_status_tree; > rwlock_t ns_status_lock; > struct kmem_cache *ns_status_cache; > @@ -264,6 +263,7 @@ struct ima_namespace { > * for measurement should be freed. This worker is used > * for handling this scenario. > */ > + refcount_t ima_keys_delayed_ref; > struct delayed_work ima_keys_delayed_work; > long ima_key_queue_timeout; > bool timer_expired; > @@ -295,24 +295,12 @@ extern struct list_head ima_default_rules; > > #ifdef CONFIG_IMA_NS > > -void free_ima_ns(struct kref *kref); > - > -static inline struct ima_namespace *get_ima_ns(struct ima_namespace *ns) > -{ > - if (ns) > - kref_get(&ns->kref); > - > - return ns; > -} > +void free_ima_ns(struct ima_namespace *ns); > +void __put_delayed_ima_ns(struct ima_namespace *ns); > > static inline void put_ima_ns(struct user_namespace *user_ns) > { > - struct ima_namespace *ns = user_ns->ima_ns; > - > - if (ns) { > - pr_debug("DEREF ima_ns: 0x%p ctr: %d\n", ns, kref_read(&ns->kref)); > - kref_put(&ns->kref, free_ima_ns); > - } > + __put_delayed_ima_ns(user_ns->ima_ns); > } > > int create_ima_ns(struct user_namespace *user_ns); > @@ -322,21 +310,20 @@ static inline struct ima_namespace *get_current_ns(void) > return current_user_ns()->ima_ns; > } > > -#else > - > -static inline struct ima_namespace *get_ima_ns(struct ima_namespace *ns) > +static inline struct user_namespace *ima_user_ns(const struct ima_namespace *ima_ns) > { > - return ns; > + struct user_namespace *user_ns; > + user_ns = current_user_ns(); > + WARN_ON(user_ns->ima_ns != ima_ns); > + return user_ns; > } > > -static inline void put_ima_ns(struct user_namespace *user_ns) > -{ > -} > +#else > > static inline int create_ima_ns(struct user_namespace *user_ns) > { > #if CONFIG_IMA > - user_ns->ima_ns = get_ima_ns(&init_ima_ns); > + user_ns->ima_ns = &init_ima_ns; > #endif > return 0; > } > @@ -346,6 +333,9 @@ static inline struct ima_namespace *get_current_ns(void) > return &init_ima_ns; > } > > +static inline void put_ima_ns(struct user_namespace *user_ns) > +{ > +} > #endif /* CONFIG_IMA_NS */ > > #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING) > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c > index 778983fd9a73..583462b29cb5 100644 > --- a/security/integrity/ima/ima_fs.c > +++ b/security/integrity/ima/ima_fs.c > @@ -386,7 +386,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp) > #else > if ((filp->f_flags & O_ACCMODE) != O_RDONLY) > return -EACCES; > - if (!mac_admin_ns_capable(ns->user_ns)) > + if (!mac_admin_ns_capable(ima_user_ns(ns))) > return -EPERM; > return seq_open(filp, &ima_policy_seqops); > #endif > diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c > index 162c94e06d13..6ae6df037f03 100644 > --- a/security/integrity/ima/ima_init_ima_ns.c > +++ b/security/integrity/ima/ima_init_ima_ns.c > @@ -62,12 +62,11 @@ int __init ima_ns_init(void) > } > > struct ima_namespace init_ima_ns = { > - .kref = KREF_INIT(1), > - .user_ns = &init_user_ns, > #ifdef CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS > .ima_process_keys = false, > .ima_keys_lock = __MUTEX_INITIALIZER(init_ima_ns.ima_keys_lock), > .ima_keys = LIST_HEAD_INIT(init_ima_ns.ima_keys), > #endif > + .ima_keys_delayed_ref = REFCOUNT_INIT(1), > }; > EXPORT_SYMBOL(init_ima_ns); > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 70fa26b7bd3f..6ebc57cd91d3 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -410,7 +410,7 @@ static int process_measurement(struct ima_namespace *ns, > u32 secid, char *buf, loff_t size, int mask, > enum ima_hooks func) > { > - struct user_namespace *user_ns = ns->user_ns; > + struct user_namespace *user_ns = ima_user_ns(ns); > int ret = 0; > > while (user_ns) { > diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c > index 6a0632806cdb..f96286ad0da8 100644 > --- a/security/integrity/ima/ima_ns.c > +++ b/security/integrity/ima/ima_ns.c > @@ -31,9 +31,6 @@ int create_ima_ns(struct user_namespace *user_ns) > return -ENOMEM; > pr_debug("NEW ima_ns: 0x%p\n", ns); > > - kref_init(&ns->kref); > - ns->user_ns = user_ns; > - > err = ima_init_namespace(ns); > if (err) > goto fail_free; > @@ -44,6 +41,7 @@ int create_ima_ns(struct user_namespace *user_ns) > INIT_LIST_HEAD(&ns->ima_keys); > #endif > > + refcount_set(&ns->ima_keys_delayed_ref, 1); > user_ns->ima_ns = ns; > > return 0; > @@ -63,11 +61,8 @@ static void destroy_ima_ns(struct ima_namespace *ns) > kmem_cache_free(imans_cachep, ns); > } > > -void free_ima_ns(struct kref *kref) > +void free_ima_ns(struct ima_namespace *ns) > { > - struct ima_namespace *ns; > - > - ns = container_of(kref, struct ima_namespace, kref); > if (WARN_ON(ns == &init_ima_ns)) > return; > > diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c > index a6eb802e5ae4..d7c43e592e2c 100644 > --- a/security/integrity/ima/ima_queue_keys.c > +++ b/security/integrity/ima/ima_queue_keys.c > @@ -14,6 +14,19 @@ > #include <keys/asymmetric-type.h> > #include "ima.h" > > +static inline void __get_delayed_ima_ns(struct ima_namespace *ima_ns) > +{ > + refcount_inc(&ima_ns->ima_keys_delayed_ref); > +} > + > +void __put_delayed_ima_ns(struct ima_namespace *ima_ns) > +{ > + if (ima_ns && refcount_dec_and_test(&ima_ns->ima_keys_delayed_ref)) { > + pr_debug("DEREF ima_ns: 0x%p ctr: %d\n", ima_ns, > + refcount_read(&ima_ns->ima_keys_delayed_ref)); > + free_ima_ns(ima_ns); > + } > +} > > /* > * This worker function frees keys that may still be > @@ -26,8 +39,7 @@ void ima_keys_handler(struct work_struct *work) > ns = container_of(work, struct ima_namespace, ima_keys_delayed_work.work); > ns->timer_expired = true; > ima_process_queued_keys(ns); > - > - put_user_ns(ns->user_ns); > + __put_delayed_ima_ns(ns); > } > > /* > @@ -36,9 +48,7 @@ void ima_keys_handler(struct work_struct *work) > */ > void ima_init_key_queue(struct ima_namespace *ns) > { > - /* keep IMA namespace until delayed work is done */ > - get_user_ns(ns->user_ns); > - > + __get_delayed_ima_ns(ns); > schedule_delayed_work(&ns->ima_keys_delayed_work, > msecs_to_jiffies(ns->ima_key_queue_timeout)); > } > @@ -145,7 +155,7 @@ void ima_process_queued_keys(struct ima_namespace *ns) > if (!ns->timer_expired) { > if (cancel_delayed_work_sync(&ns->ima_keys_delayed_work)) > /* undo reference from ima_init_key_queue */ > - put_user_ns(ns->user_ns); > + __put_delayed_ima_ns(ns); > } > > list_for_each_entry_safe(entry, tmp, &ns->ima_keys, list) {
| |