From patchwork Sat Nov 27 16:45:49 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: James Bottomley X-Patchwork-Id: 12642437 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A8C15C433F5 for ; Sat, 27 Nov 2021 16:50:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1355566AbhK0Qxu (ORCPT ); Sat, 27 Nov 2021 11:53:50 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38974 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237131AbhK0Qvu (ORCPT ); Sat, 27 Nov 2021 11:51:50 -0500 Received: from bedivere.hansenpartnership.com (bedivere.hansenpartnership.com [IPv6:2607:fcd0:100:8a00::2]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A512FC061748 for ; Sat, 27 Nov 2021 08:48:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=hansenpartnership.com; s=20151216; t=1638031715; bh=zHhAbPG3Otnbb5Z5gpk3LKyj3V2/qq/UA1JZushr+i8=; h=From:To:Subject:Date:Message-Id:In-Reply-To:References:From; b=VZNA7vqRMc26BffHLPKaNJAzpuyFQrTkhNEzf7fzQ1gDn2QjDecdIiuxv0cCAnPEr VXe7UCCM3jiTJwc54cNOAdHwpr0r1mOfRvnoEGlh7e9yODL++mCOFjt/RN4zIBLnkw GgKEaqLNSvPSLZ/ELUKrHfZ88/EuGm3RiiSpbdv4= Received: from localhost (localhost [127.0.0.1]) by bedivere.hansenpartnership.com (Postfix) with ESMTP id 7B2BE1280A6F; Sat, 27 Nov 2021 11:48:35 -0500 (EST) Received: from bedivere.hansenpartnership.com ([127.0.0.1]) by localhost (bedivere.hansenpartnership.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zYbxipdlyS1r; Sat, 27 Nov 2021 11:48:35 -0500 (EST) Received: from rainbow.int.hansenpartnership.com (unknown [153.66.140.204]) by bedivere.hansenpartnership.com (Postfix) with ESMTP id 201071280693; Sat, 27 Nov 2021 11:48:34 -0500 (EST) From: James Bottomley To: linux-integrity@vger.kernel.org Cc: containers@lists.linux.dev, Mimi Zohar , Dmitry Kasatkin , Stefan Berger , "Eric W . Biederman" , krzysztof.struczynski@huawei.com, Roberto Sassu , "Serge E . Hallyn" , Michael Peters , Luke Hinds , Lily Sturmann , Patrick Uiterwijk , Christian Brauner Subject: [RFC 3/3] ima: make the integrity inode cache per namespace Date: Sat, 27 Nov 2021 16:45:49 +0000 Message-Id: <20211127164549.2571457-4-James.Bottomley@HansenPartnership.com> X-Mailer: git-send-email 2.33.0 In-Reply-To: <20211127164549.2571457-1-James.Bottomley@HansenPartnership.com> References: <20211127164549.2571457-1-James.Bottomley@HansenPartnership.com> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-integrity@vger.kernel.org Currently we get one entry in the IMA log per unique file event. So, if you have a measurement policy and it measures a particular binary it will not get measured again if it is subsequently executed. For Namespaced IMA, the correct behaviour seems to be to log once per inode per namespace (so every unique execution in a namespace gets a separate log entry). Since logging once per inode per namespace is different from current behaviour, it is only activated if the namespace appears in the log template (so there's no behaviour change for any of the original templates). Expand the iint cache to have a list of namespaces, per iint entry, the inode has been seen in by moving the action flags and the measured_pcrs into a per-namespace structure. The lifetime of these additional list entries is tied to the lifetime of the iint entry and the namespace, so if either is deleted, the new entry is. Signed-off-by: James Bottomley --- include/linux/ima.h | 13 ++- include/linux/user_namespace.h | 5 + kernel/user.c | 1 + kernel/user_namespace.c | 6 ++ security/integrity/iint.c | 4 +- security/integrity/ima/Makefile | 2 +- security/integrity/ima/ima.h | 21 +++- security/integrity/ima/ima_api.c | 7 +- security/integrity/ima/ima_main.c | 21 ++-- security/integrity/ima/ima_ns.c | 115 ++++++++++++++++++++++ security/integrity/ima/ima_policy.c | 2 +- security/integrity/ima/ima_template_lib.c | 2 + security/integrity/integrity.h | 11 ++- 13 files changed, 192 insertions(+), 18 deletions(-) create mode 100644 security/integrity/ima/ima_ns.c diff --git a/include/linux/ima.h b/include/linux/ima.h index 09b14b73889e..dbbc0257d065 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -40,7 +40,8 @@ extern int ima_measure_critical_data(const char *event_label, const char *event_name, const void *buf, size_t buf_len, bool hash, u8 *digest, size_t digest_len); - +extern void ima_init_user_ns(struct user_namespace *ns); +extern void ima_free_user_ns(struct user_namespace *ns); #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM extern void ima_appraise_parse_cmdline(void); #else @@ -154,6 +155,16 @@ static inline int ima_measure_critical_data(const char *event_label, return -ENOENT; } +static inline void ima_init_user_ns(struct user_namespace *ns) +{ + return; +} + +static inline void ima_free_user_ns(struct user_namespace *ns) +{ + return; +} + #endif /* CONFIG_IMA */ #ifndef CONFIG_IMA_KEXEC diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h index d155788abdc1..52968764b195 100644 --- a/include/linux/user_namespace.h +++ b/include/linux/user_namespace.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -101,6 +102,10 @@ struct user_namespace { struct ucounts *ucounts; long ucount_max[UCOUNT_COUNTS]; uuid_t uuid; +#ifdef CONFIG_IMA + struct list_head ima_inode_list; + rwlock_t ima_inode_list_lock; +#endif } __randomize_layout; struct ucounts { diff --git a/kernel/user.c b/kernel/user.c index bf9ae1d0b670..670d3c41c817 100644 --- a/kernel/user.c +++ b/kernel/user.c @@ -68,6 +68,7 @@ struct user_namespace init_user_ns = { .keyring_sem = __RWSEM_INITIALIZER(init_user_ns.keyring_sem), #endif /* .uuid is initialized in user_namespaces_init() */ + /* all IMA fields are initialized by ima_init_user_ns() */ }; EXPORT_SYMBOL_GPL(init_user_ns); diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c index 8ce57c16ddd3..8afa5dc0b992 100644 --- a/kernel/user_namespace.c +++ b/kernel/user_namespace.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-only #include +#include #include #include #include @@ -144,6 +145,9 @@ int create_user_ns(struct cred *new) uuid_gen(&ns->uuid); set_cred_user_ns(new, ns); + + ima_init_user_ns(ns); + return 0; fail_keyring: #ifdef CONFIG_PERSISTENT_KEYRINGS @@ -200,6 +204,7 @@ static void free_user_ns(struct work_struct *work) } retire_userns_sysctls(ns); key_free_user_ns(ns); + ima_free_user_ns(ns); ns_free_inum(&ns->ns); kmem_cache_free(user_ns_cachep, ns); dec_user_namespaces(ucounts); @@ -1389,6 +1394,7 @@ static __init int user_namespaces_init(void) { user_ns_cachep = KMEM_CACHE(user_namespace, SLAB_PANIC | SLAB_ACCOUNT); uuid_gen(&init_user_ns.uuid); + ima_init_user_ns(&init_user_ns); return 0; } subsys_initcall(user_namespaces_init); diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 8638976f7990..f714532feb7d 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -71,6 +71,7 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode) static void iint_free(struct integrity_iint_cache *iint) { kfree(iint->ima_hash); + ima_ns_iint_list_free(iint); iint->ima_hash = NULL; iint->version = 0; iint->flags = 0UL; @@ -81,7 +82,7 @@ static void iint_free(struct integrity_iint_cache *iint) iint->ima_read_status = INTEGRITY_UNKNOWN; iint->ima_creds_status = INTEGRITY_UNKNOWN; iint->evm_status = INTEGRITY_UNKNOWN; - iint->measured_pcrs = 0; + INIT_LIST_HEAD(&iint->ns_list); kmem_cache_free(iint_cache, iint); } @@ -170,6 +171,7 @@ static void init_once(void *foo) iint->ima_creds_status = INTEGRITY_UNKNOWN; iint->evm_status = INTEGRITY_UNKNOWN; mutex_init(&iint->mutex); + INIT_LIST_HEAD(&iint->ns_list); } static int __init integrity_iintcache_init(void) diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile index 2499f2485c04..1741aa5d97bc 100644 --- a/security/integrity/ima/Makefile +++ b/security/integrity/ima/Makefile @@ -7,7 +7,7 @@ obj-$(CONFIG_IMA) += ima.o ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \ - ima_policy.o ima_template.o ima_template_lib.o + ima_policy.o ima_template.o ima_template_lib.o ima_ns.o ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index be965a8715e4..047256be7195 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -119,6 +119,16 @@ struct ima_kexec_hdr { u64 count; }; +/* IMA cache of per-user-namespace flags */ +struct ima_ns_cache { + struct user_namespace *ns; + struct integrity_iint_cache *iint; + struct list_head ns_list; + struct list_head iint_list; + unsigned long measured_pcrs; + unsigned long flags; +}; + extern const int read_idmap[]; #ifdef CONFIG_HAVE_IMA_KEXEC @@ -263,7 +273,8 @@ int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func); int ima_collect_measurement(struct integrity_iint_cache *iint, struct file *file, void *buf, loff_t size, enum hash_algo algo, struct modsig *modsig); -void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file, +void ima_store_measurement(struct integrity_iint_cache *iint, + struct ima_ns_cache *nsc, struct file *file, const unsigned char *filename, struct evm_ima_xattr_data *xattr_value, int xattr_len, const struct modsig *modsig, int pcr, @@ -301,6 +312,14 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos); void ima_policy_stop(struct seq_file *m, void *v); int ima_policy_show(struct seq_file *m, void *v); +/* IMA Namespace related functions */ +extern bool ima_ns_in_template; +struct ima_ns_cache *ima_ns_cache_get(struct integrity_iint_cache *iint, + struct user_namespace *ns); +void ima_ns_cache_clear(struct integrity_iint_cache *iint); +void ima_init_ns(void); + + /* Appraise integrity measurements */ #define IMA_APPRAISE_ENFORCE 0x01 #define IMA_APPRAISE_FIX 0x02 diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index a64fb0130b01..d613ea1ee378 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -298,6 +298,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, * Must be called with iint->mutex held. */ void ima_store_measurement(struct integrity_iint_cache *iint, + struct ima_ns_cache *nsc, struct file *file, const unsigned char *filename, struct evm_ima_xattr_data *xattr_value, int xattr_len, const struct modsig *modsig, int pcr, @@ -322,7 +323,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, * appraisal, but a file measurement from earlier might already exist in * the measurement list. */ - if (iint->measured_pcrs & (0x1 << pcr) && !modsig) + if (nsc->measured_pcrs & (0x1 << pcr) && !modsig) return; result = ima_alloc_init_template(&event_data, &entry, template_desc); @@ -334,8 +335,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint, result = ima_store_template(entry, violation, inode, filename, pcr); if ((!result || result == -EEXIST) && !(file->f_flags & O_DIRECT)) { - iint->flags |= IMA_MEASURED; - iint->measured_pcrs |= (0x1 << pcr); + nsc->flags |= IMA_MEASURED; + nsc->measured_pcrs |= (0x1 << pcr); } if (result < 0) ima_free_template_entry(entry); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 465865412100..049710203fac 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -168,8 +168,8 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint, if (!IS_I_VERSION(inode) || !inode_eq_iversion(inode, iint->version) || (iint->flags & IMA_NEW_FILE)) { - iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE); - iint->measured_pcrs = 0; + iint->flags &= ~IMA_NEW_FILE; + ima_ns_cache_clear(iint); if (update) ima_update_xattr(iint, file); } @@ -204,6 +204,7 @@ static int process_measurement(struct file *file, const struct cred *cred, { struct inode *inode = file_inode(file); struct integrity_iint_cache *iint = NULL; + struct ima_ns_cache *nsc = NULL; struct ima_template_desc *template_desc = NULL; char *pathbuf = NULL; char filename[NAME_MAX]; @@ -274,20 +275,20 @@ static int process_measurement(struct file *file, const struct cred *cred, ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) && !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) && !(action & IMA_FAIL_UNVERIFIABLE_SIGS))) { - iint->flags &= ~IMA_DONE_MASK; - iint->measured_pcrs = 0; + ima_ns_cache_clear(iint); } + nsc = ima_ns_cache_get(iint, current_user_ns()); /* Determine if already appraised/measured based on bitmask * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED, * IMA_AUDIT, IMA_AUDITED) */ - iint->flags |= action; + nsc->flags |= action; action &= IMA_DO_MASK; - action &= ~((iint->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1); + action &= ~((nsc->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1); /* If target pcr is already measured, unset IMA_MEASURE action */ - if ((action & IMA_MEASURE) && (iint->measured_pcrs & (0x1 << pcr))) + if ((action & IMA_MEASURE) && (nsc->measured_pcrs & (0x1 << pcr))) action ^= IMA_MEASURE; /* HASH sets the digital signature and update flags, nothing else */ @@ -297,7 +298,7 @@ static int process_measurement(struct file *file, const struct cred *cred, if ((xattr_value && xattr_len > 2) && (xattr_value->type == EVM_IMA_XATTR_DIGSIG)) set_bit(IMA_DIGSIG, &iint->atomic_flags); - iint->flags |= IMA_HASHED; + nsc->flags |= IMA_HASHED; action ^= IMA_HASH; set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); } @@ -342,7 +343,7 @@ static int process_measurement(struct file *file, const struct cred *cred, pathname = ima_d_path(&file->f_path, &pathbuf, filename); if (action & IMA_MEASURE) - ima_store_measurement(iint, file, pathname, + ima_store_measurement(iint, nsc, file, pathname, xattr_value, xattr_len, modsig, pcr, template_desc); if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) { @@ -1047,6 +1048,8 @@ static int __init init_ima(void) if (error) return error; + ima_init_ns(); + error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier); if (error) pr_warn("Couldn't register LSM notifier, error %d\n", error); diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c new file mode 100644 index 000000000000..7134ba7f3544 --- /dev/null +++ b/security/integrity/ima/ima_ns.c @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: GPL-2.0-only + +/* + * IMA Namespace Support routines + */ + +#include +#include + +#include "ima.h" + +static struct kmem_cache *ns_cache __read_mostly; +bool ima_ns_in_template __read_mostly; + +void __init ima_init_ns(void) +{ + ns_cache = KMEM_CACHE(ima_ns_cache, SLAB_PANIC); +} + +void ima_init_user_ns(struct user_namespace *ns) +{ + INIT_LIST_HEAD(&ns->ima_inode_list); +} + +void ima_free_user_ns(struct user_namespace *ns) +{ + struct ima_ns_cache *entry; + + /* no refs to ns left, so no need to lock */ + while (!list_empty(&ns->ima_inode_list)) { + entry = list_entry(ns->ima_inode_list.next, struct ima_ns_cache, + ns_list); + + /* iint cache entry is still active to lock to delete */ + write_lock(&entry->iint->ns_list_lock); + list_del(&entry->iint_list); + write_unlock(&entry->iint->ns_list_lock); + + list_del(&entry->ns_list); + kmem_cache_free(ns_cache, entry); + } +} + +struct ima_ns_cache *ima_ns_cache_get(struct integrity_iint_cache *iint, + struct user_namespace *ns) +{ + struct ima_ns_cache *entry = NULL; + + if (!ima_ns_in_template) + /* + * if we're not logging the namespace, don't separate the + * iint cache per namespace. This preserves original + * behaviour for the non-ns case. + */ + ns = &init_user_ns; + + read_lock(&iint->ns_list_lock); + list_for_each_entry(entry, &iint->ns_list, ns_list) + if (entry->ns == ns) + break; + read_unlock(&iint->ns_list_lock); + + if (entry && entry->ns == ns) + return entry; + + entry = kmem_cache_zalloc(ns_cache, GFP_NOFS); + if (!entry) + return NULL; + + /* no refs taken: entry is freed on either ns delete or iint delete */ + entry->ns = ns; + entry->iint = iint; + + write_lock(&iint->ns_list_lock); + list_add_tail(&entry->iint_list, &iint->ns_list); + write_unlock(&iint->ns_list_lock); + + write_lock(&ns->ima_inode_list_lock); + list_add_tail(&entry->ns_list, &ns->ima_inode_list); + write_unlock(&ns->ima_inode_list_lock); + + return entry; +} + +/* clear the flags and measured PCR for every entry in the iint */ +void ima_ns_cache_clear(struct integrity_iint_cache *iint) +{ + struct ima_ns_cache *entry; + + read_lock(&iint->ns_list_lock); + list_for_each_entry(entry, &iint->ns_list, ns_list) { + entry->flags = 0; + entry->measured_pcrs = 0; + } + read_unlock(&iint->ns_list_lock); +} + +void ima_ns_iint_list_free(struct integrity_iint_cache *iint) +{ + struct ima_ns_cache *entry; + + /* iint locking unnecessary; no-one should have acces to the list */ + while (!list_empty(&iint->ns_list)) { + entry = list_entry(iint->ns_list.next, struct ima_ns_cache, + iint_list); + + /* namespace is still active to lock to delete */ + write_lock(&entry->ns->ima_inode_list_lock); + list_del(&entry->ns_list); + write_unlock(&entry->ns->ima_inode_list_lock); + + list_del(&entry->iint_list); + kmem_cache_free(ns_cache, entry); + } +} diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 320ca80aacab..9434a1064da6 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -50,7 +50,7 @@ #define DONT_HASH 0x0200 #define INVALID_PCR(a) (((a) < 0) || \ - (a) >= (sizeof_field(struct integrity_iint_cache, measured_pcrs) * 8)) + (a) >= (sizeof_field(struct ima_ns_cache, measured_pcrs) * 8)) int ima_policy_flag; static int temp_ima_appraise; diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index ebd54c1b5206..d1a381add127 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -703,6 +703,8 @@ int ima_eventinodexattrvalues_init(struct ima_event_data *event_data, int ima_ns_init(struct ima_event_data *event_data, struct ima_field_data *field_data) { + if (unlikely(!ima_ns_in_template)) + ima_ns_in_template = true; return ima_write_template_field_data(¤t_user_ns()->uuid, UUID_SIZE, DATA_FMT_UUID, field_data); diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 547425c20e11..8755635da3ff 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -129,7 +129,6 @@ struct integrity_iint_cache { struct inode *inode; /* back pointer to inode in question */ u64 version; /* track inode changes */ unsigned long flags; - unsigned long measured_pcrs; unsigned long atomic_flags; enum integrity_status ima_file_status:4; enum integrity_status ima_mmap_status:4; @@ -138,6 +137,8 @@ struct integrity_iint_cache { enum integrity_status ima_creds_status:4; enum integrity_status evm_status:4; struct ima_digest_data *ima_hash; + struct list_head ns_list; /* list of namespaces inode seen in */ + rwlock_t ns_list_lock; }; /* rbtree tree calls to lookup, insert, delete @@ -225,6 +226,14 @@ static inline void ima_load_x509(void) } #endif +#ifdef CONFIG_IMA +void ima_ns_iint_list_free(struct integrity_iint_cache *iint); +#else +void ima_ns_iint_list_free(struct integrity_iint_cache *iint) +{ +} +#endif + #ifdef CONFIG_EVM_LOAD_X509 void __init evm_load_x509(void); #else