The goal of this series of patches is to start with the namespacing of
IMA and support auditing within an IMA namespace (IMA-ns) as the first
step.
In this series the IMA namespace is piggy backing on the user namespace
and therefore an IMA namespace gets created when a user namespace is
created. The advantage of this is that the user namespace can provide
the keys infrastructure that IMA appraisal support will need later on.
We chose the goal of supporting auditing within an IMA namespace since it
requires the least changes to IMA. Following this series, auditing within
an IMA namespace can be activated by a user running the following lines
that rely on a statically linked busybox to be installed on the host for
execution within the minimal container environment:
mkdir -p rootfs/{bin,mnt,proc}
cp /sbin/busybox rootfs/bin
PATH=/bin unshare --user --map-root-user --mount-proc --pid --fork \
--root rootfs busybox sh -c \
"busybox mount -t securityfs /mnt /mnt; \
busybox echo 'audit func=BPRM_CHECK mask=MAY_EXEC' > /mnt/ima/policy; \
busybox cat /mnt/ima/policy"
Following the audit log on the host the last line cat'ing the IMA policy
inside the namespace would have been audited. Unfortunately the auditing
line is not distinguishable from one stemming from actions on the host.
The hope here is that Richard Brigg's container id support for auditing
would help resolve the problem.
The following lines added to a suitable IMA policy on the host would
cause the execution of the commands inside the container (by uid 1000)
to be measured and audited as well on the host, thus leading to two
auditing messages for the 'busybox cat' above and log entries in IMA's
system log.
echo -e "measure func=BPRM_CHECK mask=MAY_EXEC uid=1000\n" \
"audit func=BPRM_CHECK mask=MAY_EXEC uid=1000\n" \
> /sys/kernel/security/ima/policy
The goal of supporting measurement and auditing by the host, of actions
occurring within IMA namespaces, is that users, particularly root,
should not be able to evade the host's IMA policy just by spawning
new IMA namespaces, running programs there, and discarding the namespaces
again. This is achieved through 'hierarchical processing' of file
accesses that are evaluated against the policy of the namespace where
the action occurred and against all namespaces' and their policies leading
back to the root IMA namespace (init_ima_ns).
The patch series adds support for a virtualized SecurityFS with a few
new API calls that are used by IMA namespacing so that only the data
relevant to the IMA namespace are shown. The files and directories of
other security subsystems (TPM, evm, Tomoyo, safesetid) are not showing
up when secruityfs is mounted inside a user namespace.
Much of the code leading up to the virtualization of SecurityFS deals
with moving IMA's variables from various files into the IMA namespace
structure called 'ima_namespace'. When it comes to determining the
current IMA namespace I took the approach to get the current IMA
namespace (get_current_ns()) on the top level and pass the pointer all
the way down to those functions that now need access to the ima_namespace
to get to their variables. This later on comes in handy once hierarchical
processing is implemented in this series where we walk the list of
namespaces backwards and again need to pass the pointer into functions.
A side-effect of IMA being a child of the user namespace becomes apparent
when virtualizing SecurityFS which has the effect that the filesystem code
needs to take an additional reference to the user namespace (for the keyed
filesystem), which in turn leads to the problem that the one additional
reference doesn't allow the user namespace to be deleted. The work-around
for this is to introduce an early teardown reference counter and related
IMA function that gets invoked when the user namespace reference counter
has reached a certain value, '1'. Freeing the filesystem then finally also
leads to the deletion of the user namespace.
This patch also introduces CAP_INTEGRITY_ADMIN as a subset of
CAP_SYS_ADMIN's capabilities that allows access to the IMA policy via
reduced capabilities. We would again later on use this capability to allow
users to set file extended attributes for IMA appraisal support.
The basis for this series of patches is Linux v5.15.
My tree with these patches is here:
https://github.com/stefanberger/linux-ima-namespaces/tree/v5.15%2Bimans.20211119.v4
Regards,
Stefan
v2:
- Folllwed Christian's suggestion to virtualize securitytfs; no more securityfs_ns
- Followed James's advice for late 'population' of securityfs for IMA namespaces
- Squashed 2 patches dealing with capabilities
- Added missing 'depends on USER_NS' to Kconfig
- Added missing 'static' to several functions
Denis Semakin (1):
capabilities: Introduce CAP_INTEGRITY_ADMIN
Mehmet Kayaalp (2):
ima: Define ns_status for storing namespaced iint data
ima: Namespace audit status flags
Stefan Berger (16):
ima: Add IMA namespace support
ima: Move delayed work queue and variables into ima_namespace
ima: Move IMA's keys queue related variables into ima_namespace
ima: Move policy related variables into ima_namespace
ima: Move ima_htable into ima_namespace
ima: Move measurement list related variables into ima_namespace
ima: Only accept AUDIT rules for IMA non-init_ima_ns namespaces for
now
ima: Implement hierarchical processing of file accesses
securityfs: Prefix global variables with securityfs_
securityfs: Pass static variables as parameters from top level
functions
securityfs: Extend securityfs with namespacing support
ima: Move some IMA policy and filesystem related variables into
ima_namespace
ima: Use integrity_admin_ns_capable() to check corresponding
capability
userns: Introduce a refcount variable for calling early teardown
function
ima/userns: Define early teardown function for IMA namespace
ima: Setup securityfs for IMA namespace
include/linux/capability.h | 6 +
include/linux/ima.h | 129 ++++++++++
include/linux/security.h | 15 ++
include/linux/user_namespace.h | 21 +-
include/uapi/linux/capability.h | 7 +-
init/Kconfig | 13 +
kernel/user.c | 9 +-
kernel/user_namespace.c | 16 ++
security/inode.c | 239 ++++++++++++++++---
security/integrity/ima/Makefile | 4 +-
security/integrity/ima/ima.h | 147 ++++++++----
security/integrity/ima/ima_api.c | 33 ++-
security/integrity/ima/ima_appraise.c | 26 +-
security/integrity/ima/ima_asymmetric_keys.c | 8 +-
security/integrity/ima/ima_fs.c | 215 ++++++++++++++---
security/integrity/ima/ima_init.c | 14 +-
security/integrity/ima/ima_init_ima_ns.c | 77 ++++++
security/integrity/ima/ima_main.c | 128 ++++++----
security/integrity/ima/ima_ns.c | 119 +++++++++
security/integrity/ima/ima_ns_status.c | 132 ++++++++++
security/integrity/ima/ima_policy.c | 142 ++++++-----
security/integrity/ima/ima_queue.c | 75 +++---
security/integrity/ima/ima_queue_keys.c | 73 +++---
security/integrity/ima/ima_template.c | 4 +-
security/selinux/include/classmap.h | 4 +-
25 files changed, 1324 insertions(+), 332 deletions(-)
create mode 100644 security/integrity/ima/ima_init_ima_ns.c
create mode 100644 security/integrity/ima/ima_ns.c
create mode 100644 security/integrity/ima/ima_ns_status.c
--
2.31.1
From: Mehmet Kayaalp <[email protected]>
This patch adds an rbtree to the IMA namespace structure that stores a
namespaced version of iint->flags in ns_status struct. Similar to the
integrity_iint_cache, both the iint ns_struct are looked up using the
inode pointer value. The lookup, allocate, and insertion code is also
similar, except ns_struct is not free'd when the inode is free'd.
Instead, the lookup verifies the i_ino and i_generation fields are also a
match.
Signed-off-by: Mehmet Kayaalp <[email protected]>
Signed-off-by: Stefan Berger <[email protected]>
Changelog:
v2:
* fixed tree traversal in __ima_ns_status_find()
---
include/linux/ima.h | 3 +
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 24 +++++
security/integrity/ima/ima_init_ima_ns.c | 9 ++
security/integrity/ima/ima_ns.c | 1 +
security/integrity/ima/ima_ns_status.c | 132 +++++++++++++++++++++++
6 files changed, 170 insertions(+)
create mode 100644 security/integrity/ima/ima_ns_status.c
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 86d126b9ff2f..cc0e8c509fa2 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -214,6 +214,9 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
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;
};
extern struct ima_namespace init_ima_ns;
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index b86a35fbed60..78c84214e109 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -10,6 +10,7 @@ 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_init_ima_ns.o
ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
+ima-$(CONFIG_IMA_NS) += ima_ns.o ima_ns_status.o
ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
ima-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 2f8adf383054..28896d256e36 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -133,6 +133,14 @@ static inline void ima_load_kexec_buffer(void) {}
*/
extern bool ima_canonical_fmt;
+struct ns_status {
+ struct rb_node rb_node;
+ struct inode *inode;
+ ino_t i_ino;
+ u32 i_generation;
+ unsigned long flags;
+};
+
/* Internal IMA function definitions */
int ima_init(void);
int ima_fs_init(void);
@@ -422,6 +430,22 @@ int ima_ns_init(void);
struct ima_namespace;
int ima_init_namespace(struct ima_namespace *ns);
+#ifdef CONFIG_IMA_NS
+struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+ struct inode *inode);
+
+void free_ns_status_cache(struct ima_namespace *ns);
+
+#else
+
+static inline struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+ struct inode *inode)
+{
+ return NULL;
+}
+
+#endif /* CONFIG_IMA_NS */
+
/* LSM based policy rules require audit */
#ifdef CONFIG_IMA_LSM_RULES
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index 12723d77fe17..1a44963e8ba9 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -14,9 +14,18 @@
#include <linux/user_namespace.h>
#include <linux/ima.h>
#include <linux/proc_ns.h>
+#include <linux/slab.h>
+
+#include "ima.h"
int ima_init_namespace(struct ima_namespace *ns)
{
+ ns->ns_status_tree = RB_ROOT;
+ rwlock_init(&ns->ns_status_lock);
+ ns->ns_status_cache = KMEM_CACHE(ns_status, SLAB_PANIC);
+ if (!ns->ns_status_cache)
+ return -ENOMEM;
+
return 0;
}
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 9a782c08c34e..566e59524958 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -61,6 +61,7 @@ struct ima_namespace *copy_ima_ns(struct ima_namespace *old_ns,
static void destroy_ima_ns(struct ima_namespace *ns)
{
pr_debug("DESTROY ima_ns: 0x%p\n", ns);
+ free_ns_status_cache(ns);
kmem_cache_free(imans_cachep, ns);
}
diff --git a/security/integrity/ima/ima_ns_status.c b/security/integrity/ima/ima_ns_status.c
new file mode 100644
index 000000000000..807dfaecdb5e
--- /dev/null
+++ b/security/integrity/ima/ima_ns_status.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2016-2021 IBM Corporation
+ * Author:
+ * Yuqiong Sun <[email protected]>
+ * Stefan Berger <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#include <linux/user_namespace.h>
+#include <linux/proc_ns.h>
+#include <linux/ima.h>
+
+#include "ima.h"
+
+void free_ns_status_cache(struct ima_namespace *ns)
+{
+ struct ns_status *status, *next;
+
+ write_lock(&ns->ns_status_lock);
+ rbtree_postorder_for_each_entry_safe(status, next,
+ &ns->ns_status_tree, rb_node)
+ kmem_cache_free(ns->ns_status_cache, status);
+ ns->ns_status_tree = RB_ROOT;
+ write_unlock(&ns->ns_status_lock);
+ kmem_cache_destroy(ns->ns_status_cache);
+}
+
+/*
+ * __ima_ns_status_find - return the ns_status associated with an inode
+ */
+static struct ns_status *__ima_ns_status_find(struct ima_namespace *ns,
+ struct inode *inode)
+{
+ struct ns_status *status;
+ struct rb_node *n = ns->ns_status_tree.rb_node;
+
+ while (n) {
+ status = rb_entry(n, struct ns_status, rb_node);
+
+ if (inode < status->inode)
+ n = n->rb_left;
+ else if (inode > status->inode)
+ n = n->rb_right;
+ else
+ break;
+ }
+ if (!n)
+ return NULL;
+
+ return status;
+}
+
+/*
+ * ima_ns_status_find - return the ns_status associated with an inode
+ */
+static struct ns_status *ima_ns_status_find(struct ima_namespace *ns,
+ struct inode *inode)
+{
+ struct ns_status *status;
+
+ read_lock(&ns->ns_status_lock);
+ status = __ima_ns_status_find(ns, inode);
+ read_unlock(&ns->ns_status_lock);
+
+ return status;
+}
+
+static void insert_ns_status(struct ima_namespace *ns, struct inode *inode,
+ struct ns_status *status)
+{
+ struct rb_node **p;
+ struct rb_node *node, *parent = NULL;
+ struct ns_status *test_status;
+
+ p = &ns->ns_status_tree.rb_node;
+ while (*p) {
+ parent = *p;
+ test_status = rb_entry(parent, struct ns_status, rb_node);
+ if (inode < test_status->inode)
+ p = &(*p)->rb_left;
+ else
+ p = &(*p)->rb_right;
+ }
+ node = &status->rb_node;
+ rb_link_node(node, parent, p);
+ rb_insert_color(node, &ns->ns_status_tree);
+}
+
+struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+ struct inode *inode)
+{
+ struct ns_status *status;
+ int skip_insert = 0;
+
+ status = ima_ns_status_find(ns, inode);
+ if (status) {
+ /*
+ * Unlike integrity_iint_cache we are not free'ing the
+ * ns_status data when the inode is free'd. So, in addition to
+ * checking the inode pointer, we need to make sure the
+ * (i_generation, i_ino) pair matches as well.
+ */
+ if (inode->i_ino == status->i_ino &&
+ inode->i_generation == status->i_generation)
+ return status;
+
+ /* Same inode number is reused, overwrite the ns_status */
+ skip_insert = 1;
+ } else {
+ status = kmem_cache_alloc(ns->ns_status_cache, GFP_NOFS);
+ if (!status)
+ return ERR_PTR(-ENOMEM);
+ }
+
+ write_lock(&ns->ns_status_lock);
+
+ if (!skip_insert)
+ insert_ns_status(ns, inode, status);
+
+ status->inode = inode;
+ status->i_ino = inode->i_ino;
+ status->i_generation = inode->i_generation;
+ status->flags = 0UL;
+
+ write_unlock(&ns->ns_status_lock);
+
+ return status;
+}
--
2.31.1
Move variables related to the IMA policy into the ima_namespace. This way
the IMA policy of an IMA namespace can be set and displayed using a
front-end like SecurityFS.
Implement ima_free_policy_rules() that frees the policy rules on
ima_namespace deletion.
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 10 ++
security/integrity/ima/ima.h | 35 ++---
security/integrity/ima/ima_api.c | 8 +-
security/integrity/ima/ima_appraise.c | 26 ++--
security/integrity/ima/ima_asymmetric_keys.c | 3 +-
security/integrity/ima/ima_fs.c | 11 +-
security/integrity/ima/ima_init.c | 2 +-
security/integrity/ima/ima_init_ima_ns.c | 6 +
security/integrity/ima/ima_main.c | 68 +++++-----
security/integrity/ima/ima_ns.c | 1 +
security/integrity/ima/ima_policy.c | 128 ++++++++++---------
security/integrity/ima/ima_queue_keys.c | 2 +-
12 files changed, 176 insertions(+), 124 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 977df9155cde..e13e63a539d8 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -239,9 +239,19 @@ struct ima_namespace {
long ima_key_queue_timeout;
bool timer_expired;
#endif
+
+ struct list_head ima_default_rules;
+ /* ns's policy rules */
+ struct list_head ima_policy_rules;
+ struct list_head ima_temp_rules;
+ /* Pointer to ns's current policy */
+ struct list_head __rcu *ima_rules;
+ /* current content of the policy */
+ int ima_policy_flag;
};
extern struct ima_namespace init_ima_ns;
+extern struct list_head ima_default_rules;
#ifdef CONFIG_IMA_NS
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 97eb03376855..e295141f2478 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -43,9 +43,6 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
#define NR_BANKS(chip) ((chip != NULL) ? chip->nr_allocated_banks : 0)
-/* current content of the policy */
-extern int ima_policy_flag;
-
/* bitset of digests algorithms allowed in the setxattr hook */
extern atomic_t ima_setxattr_allowed_hash_algorithms;
@@ -265,7 +262,8 @@ static inline void ima_process_queued_keys(struct ima_namespace *ns) {}
#endif /* CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS */
/* LIM API function definitions */
-int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
+int ima_get_action(struct ima_namespace *ns,
+ struct user_namespace *mnt_userns, struct inode *inode,
const struct cred *cred, u32 secid, int mask,
enum ima_hooks func, int *pcr,
struct ima_template_desc **template_desc,
@@ -279,7 +277,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
struct ima_template_desc *template_desc);
-int process_buffer_measurement(struct user_namespace *mnt_userns,
+int process_buffer_measurement(struct ima_namespace *ns,
+ struct user_namespace *mnt_userns,
struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
int pcr, const char *func_data,
@@ -297,17 +296,19 @@ void ima_free_template_entry(struct ima_template_entry *entry);
const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
/* IMA policy related functions */
-int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
+int ima_match_policy(struct ima_namespace *ns,
+ struct user_namespace *mnt_userns, struct inode *inode,
const struct cred *cred, u32 secid, enum ima_hooks func,
int mask, int flags, int *pcr,
struct ima_template_desc **template_desc,
const char *func_data, unsigned int *allowed_algos);
-void ima_init_policy(void);
+void ima_init_policy(struct ima_namespace *ns);
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);
-int ima_check_policy(void);
+void ima_update_policy_flags(struct ima_namespace *ns);
+ssize_t ima_parse_add_rule(struct ima_namespace *ns, char *rule);
+void ima_delete_rules(struct ima_namespace *ns);
+int ima_check_policy(struct ima_namespace *ns);
+void ima_free_policy_rules(struct ima_namespace *ns);
void *ima_policy_start(struct seq_file *m, loff_t *pos);
void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
void ima_policy_stop(struct seq_file *m, void *v);
@@ -323,14 +324,16 @@ int ima_policy_show(struct seq_file *m, void *v);
#define IMA_APPRAISE_KEXEC 0x40
#ifdef CONFIG_IMA_APPRAISE
-int ima_check_blacklist(struct integrity_iint_cache *iint,
+int ima_check_blacklist(struct ima_namespace *ns,
+ struct integrity_iint_cache *iint,
const struct modsig *modsig, int pcr);
int ima_appraise_measurement(enum ima_hooks func,
struct integrity_iint_cache *iint,
struct file *file, const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig);
-int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode,
+int ima_must_appraise(struct ima_namespace *ns,
+ struct user_namespace *mnt_userns, struct inode *inode,
int mask, enum ima_hooks func);
void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
@@ -341,7 +344,8 @@ int ima_read_xattr(struct dentry *dentry,
struct evm_ima_xattr_data **xattr_value);
#else
-static inline int ima_check_blacklist(struct integrity_iint_cache *iint,
+static inline int ima_check_blacklist(struct ima_namespace *ns,
+ struct integrity_iint_cache *iint,
const struct modsig *modsig, int pcr)
{
return 0;
@@ -358,7 +362,8 @@ static inline int ima_appraise_measurement(enum ima_hooks func,
return INTEGRITY_UNKNOWN;
}
-static inline int ima_must_appraise(struct user_namespace *mnt_userns,
+static inline int ima_must_appraise(struct ima_namespace *ns,
+ struct user_namespace *mnt_userns,
struct inode *inode, int mask,
enum ima_hooks func)
{
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 8f7bab17b784..808aec56dbb6 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -14,6 +14,7 @@
#include <linux/xattr.h>
#include <linux/evm.h>
#include <linux/iversion.h>
+#include <linux/ima.h>
#include "ima.h"
@@ -185,7 +186,8 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
* Returns IMA_MEASURE, IMA_APPRAISE mask.
*
*/
-int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
+int ima_get_action(struct ima_namespace *ns,
+ struct user_namespace *mnt_userns, struct inode *inode,
const struct cred *cred, u32 secid, int mask,
enum ima_hooks func, int *pcr,
struct ima_template_desc **template_desc,
@@ -193,9 +195,9 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
{
int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
- flags &= ima_policy_flag;
+ flags &= ns->ima_policy_flag;
- return ima_match_policy(mnt_userns, inode, cred, secid, func, mask,
+ return ima_match_policy(ns, mnt_userns, inode, cred, secid, func, mask,
flags, pcr, template_desc, func_data,
allowed_algos);
}
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index dbba51583e7c..b0c1992d8c4b 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -68,7 +68,8 @@ bool is_ima_appraise_enabled(void)
*
* Return 1 to appraise or hash
*/
-int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode,
+int ima_must_appraise(struct ima_namespace *ns,
+ struct user_namespace *mnt_userns, struct inode *inode,
int mask, enum ima_hooks func)
{
u32 secid;
@@ -77,7 +78,7 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode,
return 0;
security_task_getsecid_subj(current, &secid);
- return ima_match_policy(mnt_userns, inode, current_cred(), secid,
+ return ima_match_policy(ns, mnt_userns, inode, current_cred(), secid,
func, mask, IMA_APPRAISE | IMA_HASH, NULL,
NULL, NULL, NULL);
}
@@ -341,7 +342,8 @@ static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
*
* Returns -EPERM if the hash is blacklisted.
*/
-int ima_check_blacklist(struct integrity_iint_cache *iint,
+int ima_check_blacklist(struct ima_namespace *ns,
+ struct integrity_iint_cache *iint,
const struct modsig *modsig, int pcr)
{
enum hash_algo hash_algo;
@@ -357,7 +359,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
rc = is_binary_blacklisted(digest, digestsize);
if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
- process_buffer_measurement(&init_user_ns, NULL, digest, digestsize,
+ process_buffer_measurement(ns, &init_user_ns, NULL, digest, digestsize,
"blacklisted-hash", NONE,
pcr, NULL, false, NULL, 0);
}
@@ -527,14 +529,15 @@ void ima_inode_post_setattr(struct user_namespace *mnt_userns,
struct dentry *dentry)
{
struct inode *inode = d_backing_inode(dentry);
+ struct ima_namespace *ns = get_current_ns();
struct integrity_iint_cache *iint;
int action;
- if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)
+ if (!(ns->ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)
|| !(inode->i_opflags & IOP_XATTR))
return;
- action = ima_must_appraise(mnt_userns, inode, MAY_ACCESS, POST_SETATTR);
+ action = ima_must_appraise(ns, mnt_userns, inode, MAY_ACCESS, POST_SETATTR);
iint = integrity_iint_find(inode);
if (iint) {
set_bit(IMA_CHANGE_ATTR, &iint->atomic_flags);
@@ -559,11 +562,12 @@ static int ima_protect_xattr(struct dentry *dentry, const char *xattr_name,
return 0;
}
-static void ima_reset_appraise_flags(struct inode *inode, int digsig)
+static void ima_reset_appraise_flags(struct ima_namespace *ns,
+ struct inode *inode, int digsig)
{
struct integrity_iint_cache *iint;
- if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode))
+ if (!(ns->ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode))
return;
iint = integrity_iint_find(inode);
@@ -641,6 +645,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
const void *xattr_value, size_t xattr_value_len)
{
const struct evm_ima_xattr_data *xvalue = xattr_value;
+ struct ima_namespace *ns = get_current_ns();
int digsig = 0;
int result;
@@ -658,18 +663,19 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
if (result)
return result;
- ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
+ ima_reset_appraise_flags(ns, d_backing_inode(dentry), digsig);
}
return result;
}
int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
{
+ struct ima_namespace *ns = get_current_ns();
int result;
result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
if (result == 1 || evm_revalidate_status(xattr_name)) {
- ima_reset_appraise_flags(d_backing_inode(dentry), 0);
+ ima_reset_appraise_flags(ns, d_backing_inode(dentry), 0);
if (result == 1)
result = 0;
}
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index b20e82eda8f4..b5fe4ed62fec 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -61,7 +61,8 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
* if the IMA policy is configured to measure a key linked
* to the given keyring.
*/
- process_buffer_measurement(&init_user_ns, NULL, payload, payload_len,
+ process_buffer_measurement(get_current_ns(), &init_user_ns, NULL,
+ payload, payload_len,
keyring->description, KEY_CHECK, 0,
keyring->description, false, NULL, 0);
}
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index b89cd69df0de..fc0413c8c358 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -297,7 +297,7 @@ static ssize_t ima_read_policy(char *path)
datap = data;
while (size > 0 && (p = strsep(&datap, "\n"))) {
pr_debug("rule: %s\n", p);
- rc = ima_parse_add_rule(p);
+ rc = ima_parse_add_rule(get_current_ns(), p);
if (rc < 0)
break;
size -= rc;
@@ -345,7 +345,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
1, 0);
result = -EACCES;
} else {
- result = ima_parse_add_rule(data);
+ result = ima_parse_add_rule(get_current_ns(), data);
}
mutex_unlock(&ima_write_mutex);
out_free:
@@ -411,11 +411,12 @@ 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);
- if (valid_policy && ima_check_policy() < 0) {
+ if (valid_policy && ima_check_policy(ns) < 0) {
cause = "failed";
valid_policy = 0;
}
@@ -425,13 +426,13 @@ static int ima_release_policy(struct inode *inode, struct file *file)
"policy_update", cause, !valid_policy, 0);
if (!valid_policy) {
- ima_delete_rules();
+ ima_delete_rules(ns);
valid_policy = 1;
clear_bit(IMA_FS_BUSY, &ima_fs_flags);
return 0;
}
- ima_update_policy(get_current_ns());
+ 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 24848373a061..2ec9a22bbddf 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -149,7 +149,7 @@ int __init ima_init(void)
if (rc != 0)
return rc;
- ima_init_policy();
+ ima_init_policy(&init_ima_ns);
rc = ima_fs_init();
if (rc != 0)
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index 7b66fe598789..2d644791a795 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -34,6 +34,12 @@ int ima_init_namespace(struct ima_namespace *ns)
ima_init_key_queue(ns);
#endif
+ INIT_LIST_HEAD(&ns->ima_default_rules);
+ INIT_LIST_HEAD(&ns->ima_policy_rules);
+ INIT_LIST_HEAD(&ns->ima_temp_rules);
+ ns->ima_rules = (struct list_head __rcu *)(&ns->ima_default_rules);
+ ns->ima_policy_flag = 0;
+
return 0;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 4df60dbb56f7..9cf1fd7c70bf 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -188,7 +188,7 @@ void ima_file_free(struct file *file)
struct inode *inode = file_inode(file);
struct integrity_iint_cache *iint;
- if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+ if (!get_current_ns()->ima_policy_flag || !S_ISREG(inode->i_mode))
return;
iint = integrity_iint_find(inode);
@@ -198,7 +198,8 @@ void ima_file_free(struct file *file)
ima_check_last_writer(iint, inode, file);
}
-static int process_measurement(struct file *file, const struct cred *cred,
+static int process_measurement(struct ima_namespace *ns,
+ struct file *file, const struct cred *cred,
u32 secid, char *buf, loff_t size, int mask,
enum ima_hooks func)
{
@@ -219,18 +220,18 @@ static int process_measurement(struct file *file, const struct cred *cred,
unsigned int allowed_algos = 0;
unsigned long flags;
- if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+ if (!ns->ima_policy_flag || !S_ISREG(inode->i_mode))
return 0;
/* Return an IMA_MEASURE, IMA_APPRAISE, IMA_AUDIT action
* bitmask based on the appraise/audit/measurement policy.
* Included is the appraise submask.
*/
- action = ima_get_action(file_mnt_user_ns(file), inode, cred, secid,
+ action = ima_get_action(ns, file_mnt_user_ns(file), inode, cred, secid,
mask, func, &pcr, &template_desc, NULL,
&allowed_algos);
violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
- (ima_policy_flag & IMA_MEASURE));
+ (ns->ima_policy_flag & IMA_MEASURE));
if (!action && !violation_check)
return 0;
@@ -248,7 +249,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
rc = -ENOMEM;
if (!rc && (action & IMA_NS_STATUS_ACTIONS)) {
- status = ima_get_ns_status(get_current_ns(), inode);
+ status = ima_get_ns_status(ns, inode);
if (IS_ERR(status))
rc = PTR_ERR(status);
}
@@ -356,7 +357,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
xattr_value, xattr_len, modsig, pcr,
template_desc);
if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
- rc = ima_check_blacklist(iint, modsig, pcr);
+ rc = ima_check_blacklist(ns, iint, modsig, pcr);
if (rc != -EPERM) {
inode_lock(inode);
rc = ima_appraise_measurement(func, iint, file,
@@ -419,7 +420,8 @@ int ima_file_mmap(struct file *file, unsigned long prot)
if (file && (prot & PROT_EXEC)) {
security_task_getsecid_subj(current, &secid);
- return process_measurement(file, current_cred(), secid, NULL,
+ return process_measurement(get_current_ns(),
+ file, current_cred(), secid, NULL,
0, MAY_EXEC, MMAP_CHECK);
}
@@ -440,6 +442,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
*/
int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
{
+ struct ima_namespace *ns = get_current_ns();
struct ima_template_desc *template = NULL;
struct file *file = vma->vm_file;
char filename[NAME_MAX];
@@ -452,13 +455,13 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
int pcr;
/* Is mprotect making an mmap'ed file executable? */
- if (!(ima_policy_flag & IMA_APPRAISE) || !vma->vm_file ||
+ if (!(ns->ima_policy_flag & IMA_APPRAISE) || !vma->vm_file ||
!(prot & PROT_EXEC) || (vma->vm_flags & VM_EXEC))
return 0;
security_task_getsecid_subj(current, &secid);
inode = file_inode(vma->vm_file);
- action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode,
+ action = ima_get_action(ns, file_mnt_user_ns(vma->vm_file), inode,
current_cred(), secid, MAY_EXEC, MMAP_CHECK,
&pcr, &template, NULL, NULL);
@@ -498,13 +501,13 @@ int ima_bprm_check(struct linux_binprm *bprm)
u32 secid;
security_task_getsecid_subj(current, &secid);
- ret = process_measurement(bprm->file, current_cred(), secid, NULL, 0,
+ ret = process_measurement(get_current_ns(), bprm->file, current_cred(), secid, NULL, 0,
MAY_EXEC, BPRM_CHECK);
if (ret)
return ret;
security_cred_getsecid(bprm->cred, &secid);
- return process_measurement(bprm->file, bprm->cred, secid, NULL, 0,
+ return process_measurement(get_current_ns(), bprm->file, bprm->cred, secid, NULL, 0,
MAY_EXEC, CREDS_CHECK);
}
@@ -523,18 +526,19 @@ int ima_file_check(struct file *file, int mask)
u32 secid;
security_task_getsecid_subj(current, &secid);
- return process_measurement(file, current_cred(), secid, NULL, 0,
+ return process_measurement(get_current_ns(), file, current_cred(), secid, NULL, 0,
mask & (MAY_READ | MAY_WRITE | MAY_EXEC |
MAY_APPEND), FILE_CHECK);
}
EXPORT_SYMBOL_GPL(ima_file_check);
-static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
+static int __ima_inode_hash(struct ima_namespace *ns,
+ struct inode *inode, char *buf, size_t buf_size)
{
struct integrity_iint_cache *iint;
int hash_algo;
- if (!ima_policy_flag)
+ if (!ns->ima_policy_flag)
return -EOPNOTSUPP;
iint = integrity_iint_find(inode);
@@ -587,7 +591,7 @@ int ima_file_hash(struct file *file, char *buf, size_t buf_size)
if (!file)
return -EINVAL;
- return __ima_inode_hash(file_inode(file), buf, buf_size);
+ return __ima_inode_hash(get_current_ns(), file_inode(file), buf, buf_size);
}
EXPORT_SYMBOL_GPL(ima_file_hash);
@@ -614,7 +618,7 @@ int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
if (!inode)
return -EINVAL;
- return __ima_inode_hash(inode, buf, buf_size);
+ return __ima_inode_hash(get_current_ns(), inode, buf, buf_size);
}
EXPORT_SYMBOL_GPL(ima_inode_hash);
@@ -630,13 +634,14 @@ EXPORT_SYMBOL_GPL(ima_inode_hash);
void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
struct inode *inode)
{
+ struct ima_namespace *ns = get_current_ns();
struct integrity_iint_cache *iint;
int must_appraise;
- if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+ if (!ns->ima_policy_flag || !S_ISREG(inode->i_mode))
return;
- must_appraise = ima_must_appraise(mnt_userns, inode, MAY_ACCESS,
+ must_appraise = ima_must_appraise(ns, mnt_userns, inode, MAY_ACCESS,
FILE_CHECK);
if (!must_appraise)
return;
@@ -662,14 +667,15 @@ void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
void ima_post_path_mknod(struct user_namespace *mnt_userns,
struct dentry *dentry)
{
+ struct ima_namespace *ns = get_current_ns();
struct integrity_iint_cache *iint;
struct inode *inode = dentry->d_inode;
int must_appraise;
- if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+ if (!ns->ima_policy_flag || !S_ISREG(inode->i_mode))
return;
- must_appraise = ima_must_appraise(mnt_userns, inode, MAY_ACCESS,
+ must_appraise = ima_must_appraise(ns, mnt_userns, inode, MAY_ACCESS,
FILE_CHECK);
if (!must_appraise)
return;
@@ -720,7 +726,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id,
/* Read entire file for all partial reads. */
func = read_idmap[read_id] ?: FILE_CHECK;
security_task_getsecid_subj(current, &secid);
- return process_measurement(file, current_cred(), secid, NULL,
+ return process_measurement(get_current_ns(), file, current_cred(), secid, NULL,
0, MAY_READ, func);
}
@@ -763,7 +769,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
func = read_idmap[read_id] ?: FILE_CHECK;
security_task_getsecid_subj(current, &secid);
- return process_measurement(file, current_cred(), secid, buf, size,
+ return process_measurement(get_current_ns(), file, current_cred(), secid, buf, size,
MAY_READ, func);
}
@@ -869,7 +875,8 @@ int ima_post_load_data(char *buf, loff_t size,
* has been written to the passed location but not added to a measurement entry,
* a negative value otherwise.
*/
-int process_buffer_measurement(struct user_namespace *mnt_userns,
+int process_buffer_measurement(struct ima_namespace *ns,
+ struct user_namespace *mnt_userns,
struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
int pcr, const char *func_data,
@@ -897,7 +904,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
if (digest && digest_len < digest_hash_len)
return -EINVAL;
- if (!ima_policy_flag && !digest)
+ if (!ns->ima_policy_flag && !digest)
return -ENOENT;
template = ima_template_desc_buf();
@@ -916,7 +923,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
*/
if (func) {
security_task_getsecid_subj(current, &secid);
- action = ima_get_action(mnt_userns, inode, current_cred(),
+ action = ima_get_action(ns, mnt_userns, inode, current_cred(),
secid, 0, func, &pcr, &template,
func_data, NULL);
if (!(action & IMA_MEASURE) && !digest)
@@ -953,7 +960,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
if (digest)
memcpy(digest, iint.ima_hash->digest, digest_hash_len);
- if (!ima_policy_flag || (func && !(action & IMA_MEASURE)))
+ if (!ns->ima_policy_flag || (func && !(action & IMA_MEASURE)))
return 1;
ret = ima_alloc_init_template(&event_data, &entry, template);
@@ -996,7 +1003,8 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
if (!f.file)
return;
- process_buffer_measurement(file_mnt_user_ns(f.file), file_inode(f.file),
+ process_buffer_measurement(get_current_ns(),
+ file_mnt_user_ns(f.file), file_inode(f.file),
buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
NULL, false, NULL, 0);
fdput(f);
@@ -1029,7 +1037,7 @@ int ima_measure_critical_data(const char *event_label,
if (!event_name || !event_label || !buf || !buf_len)
return -ENOPARAM;
- return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
+ return process_buffer_measurement(get_current_ns(), &init_user_ns, NULL, buf, buf_len,
event_name, CRITICAL_DATA, 0,
event_label, hash, digest,
digest_len);
@@ -1062,7 +1070,7 @@ static int __init init_ima(void)
pr_warn("Couldn't register LSM notifier, error %d\n", error);
if (!error)
- ima_update_policy_flags();
+ ima_update_policy_flags(&init_ima_ns);
return error;
}
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 11e0343f1f55..efbf7087a8ee 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -67,6 +67,7 @@ struct ima_namespace *copy_ima_ns(struct ima_namespace *old_ns,
static void destroy_ima_ns(struct ima_namespace *ns)
{
pr_debug("DESTROY ima_ns: 0x%p\n", ns);
+ ima_free_policy_rules(ns);
free_ns_status_cache(ns);
kmem_cache_free(imans_cachep, ns);
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e5aef287d14d..96e7d63167e8 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -52,7 +52,6 @@
#define INVALID_PCR(a) (((a) < 0) || \
(a) >= (sizeof_field(struct integrity_iint_cache, measured_pcrs) * 8))
-int ima_policy_flag;
static int temp_ima_appraise;
static int build_ima_appraise __ro_after_init;
@@ -233,11 +232,6 @@ static struct ima_rule_entry critical_data_rules[] __ro_after_init = {
/* An array of architecture specific rules */
static struct ima_rule_entry *arch_policy_entry __ro_after_init;
-static LIST_HEAD(ima_default_rules);
-static LIST_HEAD(ima_policy_rules);
-static LIST_HEAD(ima_temp_rules);
-static struct list_head __rcu *ima_rules = (struct list_head __rcu *)(&ima_default_rules);
-
static int ima_policy __initdata;
static int __init default_measure_policy_setup(char *str)
@@ -454,12 +448,12 @@ static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry)
* to the old, stale LSM policy. Update the IMA LSM based rules to reflect
* the reloaded LSM policy.
*/
-static void ima_lsm_update_rules(void)
+static void ima_lsm_update_rules(struct ima_namespace *ns)
{
struct ima_rule_entry *entry, *e;
int result;
- list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
+ list_for_each_entry_safe(entry, e, &ns->ima_policy_rules, list) {
if (!ima_rule_contains_lsm_cond(entry))
continue;
@@ -477,7 +471,7 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
if (event != LSM_POLICY_CHANGE)
return NOTIFY_DONE;
- ima_lsm_update_rules();
+ ima_lsm_update_rules(get_current_ns());
return NOTIFY_OK;
}
@@ -688,7 +682,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
* list when walking it. Reads are many orders of magnitude more numerous
* than writes so ima_match_policy() is classical RCU candidate.
*/
-int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
+int ima_match_policy(struct ima_namespace *ns,
+ struct user_namespace *mnt_userns, struct inode *inode,
const struct cred *cred, u32 secid, enum ima_hooks func,
int mask, int flags, int *pcr,
struct ima_template_desc **template_desc,
@@ -702,7 +697,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
*template_desc = ima_template_desc_current();
rcu_read_lock();
- ima_rules_tmp = rcu_dereference(ima_rules);
+ ima_rules_tmp = rcu_dereference(ns->ima_rules);
list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
if (!(entry->action & actmask))
@@ -760,14 +755,14 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
*
* Context: called after a policy update and at system initialization.
*/
-void ima_update_policy_flags(void)
+void ima_update_policy_flags(struct ima_namespace *ns)
{
struct ima_rule_entry *entry;
int new_policy_flag = 0;
struct list_head *ima_rules_tmp;
rcu_read_lock();
- ima_rules_tmp = rcu_dereference(ima_rules);
+ ima_rules_tmp = rcu_dereference(ns->ima_rules);
list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
/*
* SETXATTR_CHECK rules do not implement a full policy check
@@ -797,7 +792,7 @@ void ima_update_policy_flags(void)
if (!ima_appraise)
new_policy_flag &= ~IMA_APPRAISE;
- ima_policy_flag = new_policy_flag;
+ ns->ima_policy_flag = new_policy_flag;
}
static int ima_appraise_flag(enum ima_hooks func)
@@ -813,7 +808,8 @@ static int ima_appraise_flag(enum ima_hooks func)
return 0;
}
-static void add_rules(struct ima_rule_entry *entries, int count,
+static void add_rules(struct ima_namespace *ns,
+ struct ima_rule_entry *entries, int count,
enum policy_rule_list policy_rule)
{
int i = 0;
@@ -822,7 +818,7 @@ static void add_rules(struct ima_rule_entry *entries, int count,
struct ima_rule_entry *entry;
if (policy_rule & IMA_DEFAULT_POLICY)
- list_add_tail(&entries[i].list, &ima_default_rules);
+ list_add_tail(&entries[i].list, &ns->ima_default_rules);
if (policy_rule & IMA_CUSTOM_POLICY) {
entry = kmemdup(&entries[i], sizeof(*entry),
@@ -830,7 +826,7 @@ static void add_rules(struct ima_rule_entry *entries, int count,
if (!entry)
continue;
- list_add_tail(&entry->list, &ima_policy_rules);
+ list_add_tail(&entry->list, &ns->ima_policy_rules);
}
if (entries[i].action == APPRAISE) {
if (entries != build_appraise_rules)
@@ -843,9 +839,10 @@ static void add_rules(struct ima_rule_entry *entries, int count,
}
}
-static int ima_parse_rule(char *rule, struct ima_rule_entry *entry);
+static int ima_parse_rule(struct ima_namespace *ns,
+ char *rule, struct ima_rule_entry *entry);
-static int __init ima_init_arch_policy(void)
+static int __init ima_init_arch_policy(struct ima_namespace *ns)
{
const char * const *arch_rules;
const char * const *rules;
@@ -873,7 +870,7 @@ static int __init ima_init_arch_policy(void)
result = strscpy(rule, *rules, sizeof(rule));
INIT_LIST_HEAD(&arch_policy_entry[i].list);
- result = ima_parse_rule(rule, &arch_policy_entry[i]);
+ result = ima_parse_rule(ns, rule, &arch_policy_entry[i]);
if (result) {
pr_warn("Skipping unknown architecture policy rule: %s\n",
rule);
@@ -891,23 +888,23 @@ static int __init ima_init_arch_policy(void)
*
* ima_rules points to either the ima_default_rules or the new ima_policy_rules.
*/
-void __init ima_init_policy(void)
+void __init ima_init_policy(struct ima_namespace *ns)
{
int build_appraise_entries, arch_entries;
/* if !ima_policy, we load NO default rules */
if (ima_policy)
- add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules),
+ add_rules(ns, dont_measure_rules, ARRAY_SIZE(dont_measure_rules),
IMA_DEFAULT_POLICY);
switch (ima_policy) {
case ORIGINAL_TCB:
- add_rules(original_measurement_rules,
+ add_rules(ns, original_measurement_rules,
ARRAY_SIZE(original_measurement_rules),
IMA_DEFAULT_POLICY);
break;
case DEFAULT_TCB:
- add_rules(default_measurement_rules,
+ add_rules(ns, default_measurement_rules,
ARRAY_SIZE(default_measurement_rules),
IMA_DEFAULT_POLICY);
break;
@@ -921,11 +918,11 @@ void __init ima_init_policy(void)
* and custom policies, prior to other appraise rules.
* (Highest priority)
*/
- arch_entries = ima_init_arch_policy();
+ arch_entries = ima_init_arch_policy(ns);
if (!arch_entries)
pr_info("No architecture policies found\n");
else
- add_rules(arch_policy_entry, arch_entries,
+ add_rules(ns, arch_policy_entry, arch_entries,
IMA_DEFAULT_POLICY | IMA_CUSTOM_POLICY);
/*
@@ -933,7 +930,7 @@ void __init ima_init_policy(void)
* signatures, prior to other appraise rules.
*/
if (ima_use_secure_boot)
- add_rules(secure_boot_rules, ARRAY_SIZE(secure_boot_rules),
+ add_rules(ns, secure_boot_rules, ARRAY_SIZE(secure_boot_rules),
IMA_DEFAULT_POLICY);
/*
@@ -945,32 +942,32 @@ void __init ima_init_policy(void)
build_appraise_entries = ARRAY_SIZE(build_appraise_rules);
if (build_appraise_entries) {
if (ima_use_secure_boot)
- add_rules(build_appraise_rules, build_appraise_entries,
+ add_rules(ns, build_appraise_rules, build_appraise_entries,
IMA_CUSTOM_POLICY);
else
- add_rules(build_appraise_rules, build_appraise_entries,
+ add_rules(ns, build_appraise_rules, build_appraise_entries,
IMA_DEFAULT_POLICY | IMA_CUSTOM_POLICY);
}
if (ima_use_appraise_tcb)
- add_rules(default_appraise_rules,
+ add_rules(ns, default_appraise_rules,
ARRAY_SIZE(default_appraise_rules),
IMA_DEFAULT_POLICY);
if (ima_use_critical_data)
- add_rules(critical_data_rules,
+ add_rules(ns, critical_data_rules,
ARRAY_SIZE(critical_data_rules),
IMA_DEFAULT_POLICY);
atomic_set(&ima_setxattr_allowed_hash_algorithms, 0);
- ima_update_policy_flags();
+ ima_update_policy_flags(ns);
}
/* Make sure we have a valid policy, at least containing some rules. */
-int ima_check_policy(void)
+int ima_check_policy(struct ima_namespace *ns)
{
- if (list_empty(&ima_temp_rules))
+ if (list_empty(&ns->ima_temp_rules))
return -EINVAL;
return 0;
}
@@ -988,14 +985,14 @@ int ima_check_policy(void)
*/
void ima_update_policy(struct ima_namespace *ns)
{
- struct list_head *policy = &ima_policy_rules;
+ struct list_head *policy = &ns->ima_policy_rules;
- list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
+ list_splice_tail_init_rcu(&ns->ima_temp_rules, policy, synchronize_rcu);
- if (ima_rules != (struct list_head __rcu *)policy) {
- ima_policy_flag = 0;
+ if (ns->ima_rules != (struct list_head __rcu *)policy) {
+ ns->ima_policy_flag = 0;
- rcu_assign_pointer(ima_rules, policy);
+ rcu_assign_pointer(ns->ima_rules, policy);
/*
* IMA architecture specific policy rules are specified
* as strings and converted to an array of ima_entry_rules
@@ -1004,7 +1001,7 @@ void ima_update_policy(struct ima_namespace *ns)
*/
kfree(arch_policy_entry);
}
- ima_update_policy_flags();
+ ima_update_policy_flags(ns);
/* Custom IMA policy has been loaded */
ima_process_queued_keys(ns);
@@ -1077,7 +1074,8 @@ static const match_table_t policy_tokens = {
{Opt_err, NULL}
};
-static int ima_lsm_rule_init(struct ima_rule_entry *entry,
+static int ima_lsm_rule_init(struct ima_namespace *ns,
+ struct ima_rule_entry *entry,
substring_t *args, int lsm_rule, int audit_type)
{
int result;
@@ -1097,7 +1095,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
pr_warn("rule for LSM \'%s\' is undefined\n",
entry->lsm[lsm_rule].args_p);
- if (ima_rules == (struct list_head __rcu *)(&ima_default_rules)) {
+ if (ns->ima_rules == (struct list_head __rcu *)(&ns->ima_default_rules)) {
kfree(entry->lsm[lsm_rule].args_p);
entry->lsm[lsm_rule].args_p = NULL;
result = -EINVAL;
@@ -1324,7 +1322,8 @@ static unsigned int ima_parse_appraise_algos(char *arg)
return res;
}
-static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
+static int ima_parse_rule(struct ima_namespace *ns,
+ char *rule, struct ima_rule_entry *entry)
{
struct audit_buffer *ab;
char *from;
@@ -1674,37 +1673,37 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
break;
case Opt_obj_user:
ima_log_string(ab, "obj_user", args[0].from);
- result = ima_lsm_rule_init(entry, args,
+ result = ima_lsm_rule_init(ns, entry, args,
LSM_OBJ_USER,
AUDIT_OBJ_USER);
break;
case Opt_obj_role:
ima_log_string(ab, "obj_role", args[0].from);
- result = ima_lsm_rule_init(entry, args,
+ result = ima_lsm_rule_init(ns, entry, args,
LSM_OBJ_ROLE,
AUDIT_OBJ_ROLE);
break;
case Opt_obj_type:
ima_log_string(ab, "obj_type", args[0].from);
- result = ima_lsm_rule_init(entry, args,
+ result = ima_lsm_rule_init(ns, entry, args,
LSM_OBJ_TYPE,
AUDIT_OBJ_TYPE);
break;
case Opt_subj_user:
ima_log_string(ab, "subj_user", args[0].from);
- result = ima_lsm_rule_init(entry, args,
+ result = ima_lsm_rule_init(ns, entry, args,
LSM_SUBJ_USER,
AUDIT_SUBJ_USER);
break;
case Opt_subj_role:
ima_log_string(ab, "subj_role", args[0].from);
- result = ima_lsm_rule_init(entry, args,
+ result = ima_lsm_rule_init(ns, entry, args,
LSM_SUBJ_ROLE,
AUDIT_SUBJ_ROLE);
break;
case Opt_subj_type:
ima_log_string(ab, "subj_type", args[0].from);
- result = ima_lsm_rule_init(entry, args,
+ result = ima_lsm_rule_init(ns, entry, args,
LSM_SUBJ_TYPE,
AUDIT_SUBJ_TYPE);
break;
@@ -1810,7 +1809,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
* Avoid locking by allowing just one writer at a time in ima_write_policy()
* Returns the length of the rule parsed, an error code on failure
*/
-ssize_t ima_parse_add_rule(char *rule)
+ssize_t ima_parse_add_rule(struct ima_namespace *ns, char *rule)
{
static const char op[] = "update_policy";
char *p;
@@ -1834,7 +1833,7 @@ ssize_t ima_parse_add_rule(char *rule)
INIT_LIST_HEAD(&entry->list);
- result = ima_parse_rule(p, entry);
+ result = ima_parse_rule(ns, p, entry);
if (result) {
ima_free_rule(entry);
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
@@ -1843,7 +1842,7 @@ ssize_t ima_parse_add_rule(char *rule)
return result;
}
- list_add_tail(&entry->list, &ima_temp_rules);
+ list_add_tail(&entry->list, &ns->ima_temp_rules);
return len;
}
@@ -1854,12 +1853,24 @@ ssize_t ima_parse_add_rule(char *rule)
* different from the active one. There is also only one user of
* ima_delete_rules() at a time.
*/
-void ima_delete_rules(void)
+void ima_delete_rules(struct ima_namespace *ns)
{
struct ima_rule_entry *entry, *tmp;
temp_ima_appraise = 0;
- list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
+ list_for_each_entry_safe(entry, tmp, &ns->ima_temp_rules, list) {
+ list_del(&entry->list);
+ ima_free_rule(entry);
+ }
+}
+
+void ima_free_policy_rules(struct ima_namespace *ns)
+{
+ struct ima_rule_entry *entry, *tmp;
+
+ ima_delete_rules(ns);
+
+ list_for_each_entry_safe(entry, tmp, &ns->ima_policy_rules, list) {
list_del(&entry->list);
ima_free_rule(entry);
}
@@ -1890,7 +1901,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos)
struct list_head *ima_rules_tmp;
rcu_read_lock();
- ima_rules_tmp = rcu_dereference(ima_rules);
+ ima_rules_tmp = rcu_dereference(get_current_ns()->ima_rules);
list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
if (!l--) {
rcu_read_unlock();
@@ -1904,14 +1915,15 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos)
void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
{
struct ima_rule_entry *entry = v;
+ struct ima_namespace *ns = get_current_ns();
rcu_read_lock();
entry = list_entry_rcu(entry->list.next, struct ima_rule_entry, list);
rcu_read_unlock();
(*pos)++;
- return (&entry->list == &ima_default_rules ||
- &entry->list == &ima_policy_rules) ? NULL : entry;
+ return (&entry->list == &ns->ima_default_rules ||
+ &entry->list == &ns->ima_policy_rules) ? NULL : entry;
}
void ima_policy_stop(struct seq_file *m, void *v)
@@ -2177,7 +2189,7 @@ bool ima_appraise_signature(enum kernel_read_file_id id)
func = read_idmap[id] ?: FILE_CHECK;
rcu_read_lock();
- ima_rules_tmp = rcu_dereference(ima_rules);
+ ima_rules_tmp = rcu_dereference(get_current_ns()->ima_rules);
list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
if (entry->action != APPRAISE)
continue;
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 9e5e9a178253..14f334272160 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -142,7 +142,7 @@ void ima_process_queued_keys(struct ima_namespace *ns)
list_for_each_entry_safe(entry, tmp, &ns->ima_keys, list) {
if (!ns->timer_expired)
- process_buffer_measurement(&init_user_ns, NULL,
+ process_buffer_measurement(ns, &init_user_ns, NULL,
entry->payload,
entry->payload_len,
entry->keyring_name,
--
2.31.1
Move ima_htable into ima_namespace. This way a front-end like
SecurityFS can show the number of violations of an IMA namespace.
Move ima_hash_key() into ima_queue.c since it's only used there.
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 11 +++++++
security/integrity/ima/ima.h | 34 +++++++------------
security/integrity/ima/ima_api.c | 17 ++++++----
security/integrity/ima/ima_fs.c | 7 ++--
security/integrity/ima/ima_init.c | 6 ++--
security/integrity/ima/ima_init_ima_ns.c | 4 +++
security/integrity/ima/ima_main.c | 13 ++++----
security/integrity/ima/ima_queue.c | 42 ++++++++++++++----------
security/integrity/ima/ima_template.c | 4 +--
9 files changed, 78 insertions(+), 60 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index e13e63a539d8..929bf87b1bbf 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -211,6 +211,15 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
}
#endif /* CONFIG_IMA_APPRAISE */
+#define IMA_HASH_BITS 10
+#define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS)
+
+struct ima_h_table {
+ atomic_long_t len; /* number of stored measurements in the list */
+ atomic_long_t violations;
+ struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
+};
+
struct ima_namespace {
struct kref kref;
struct user_namespace *user_ns;
@@ -248,6 +257,8 @@ struct ima_namespace {
struct list_head __rcu *ima_rules;
/* current content of the policy */
int ima_policy_flag;
+
+ struct ima_h_table ima_htable;
};
extern struct ima_namespace init_ima_ns;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e295141f2478..a7e6c8fb152a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -32,9 +32,6 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
#define IMA_DIGEST_SIZE SHA1_DIGEST_SIZE
#define IMA_EVENT_NAME_LEN_MAX 255
-#define IMA_HASH_BITS 10
-#define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS)
-
#define IMA_TEMPLATE_FIELD_ID_MAX_LEN 16
#define IMA_TEMPLATE_NUM_FIELDS_MAX 15
@@ -143,7 +140,8 @@ struct ns_status {
/* Internal IMA function definitions */
int ima_init(void);
int ima_fs_init(void);
-int ima_add_template_entry(struct ima_template_entry *entry, int violation,
+int ima_add_template_entry(struct ima_namespace *ns,
+ struct ima_template_entry *entry, int violation,
const char *op, struct inode *inode,
const unsigned char *filename);
int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash);
@@ -152,7 +150,8 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
int ima_calc_field_array_hash(struct ima_field_data *field_data,
struct ima_template_entry *entry);
int ima_calc_boot_aggregate(struct ima_digest_data *hash);
-void ima_add_violation(struct file *file, const unsigned char *filename,
+void ima_add_violation(struct ima_namespace *ns,
+ struct file *file, const unsigned char *filename,
struct integrity_iint_cache *iint,
const char *op, const char *cause);
int ima_init_crypto(void);
@@ -165,8 +164,10 @@ struct ima_template_desc *ima_template_desc_current(void);
struct ima_template_desc *ima_template_desc_buf(void);
struct ima_template_desc *lookup_template_desc(const char *name);
bool ima_template_has_modsig(const struct ima_template_desc *ima_template);
-int ima_restore_measurement_entry(struct ima_template_entry *entry);
-int ima_restore_measurement_list(loff_t bufsize, void *buf);
+int ima_restore_measurement_entry(struct ima_namespace *ns,
+ struct ima_template_entry *entry);
+int ima_restore_measurement_list(struct ima_namespace *ns,
+ 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);
@@ -180,19 +181,6 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
*/
extern spinlock_t ima_queue_lock;
-struct ima_h_table {
- atomic_long_t len; /* number of stored measurements in the list */
- atomic_long_t violations;
- struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
-};
-extern struct ima_h_table ima_htable;
-
-static inline unsigned int ima_hash_key(u8 *digest)
-{
- /* there is no point in taking a hash of part of a digest */
- return (digest[0] | digest[1] << 8) % IMA_MEASURE_HTABLE_SIZE;
-}
-
#define __ima_hooks(hook) \
hook(NONE, none) \
hook(FILE_CHECK, file) \
@@ -272,7 +260,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 ima_namespace *ns,
+ struct integrity_iint_cache *iint, struct file *file,
const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
@@ -289,7 +278,8 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
int ima_alloc_init_template(struct ima_event_data *event_data,
struct ima_template_entry **entry,
struct ima_template_desc *template_desc);
-int ima_store_template(struct ima_template_entry *entry, int violation,
+int ima_store_template(struct ima_namespace *ns,
+ struct ima_template_entry *entry, int violation,
struct inode *inode,
const unsigned char *filename, int pcr);
void ima_free_template_entry(struct ima_template_entry *entry);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 808aec56dbb6..71c5517fe8bc 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -100,7 +100,8 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
*
* Returns 0 on success, error code otherwise
*/
-int ima_store_template(struct ima_template_entry *entry,
+int ima_store_template(struct ima_namespace *ns,
+ struct ima_template_entry *entry,
int violation, struct inode *inode,
const unsigned char *filename, int pcr)
{
@@ -120,7 +121,7 @@ int ima_store_template(struct ima_template_entry *entry,
}
}
entry->pcr = pcr;
- result = ima_add_template_entry(entry, violation, op, inode, filename);
+ result = ima_add_template_entry(ns, entry, violation, op, inode, filename);
return result;
}
@@ -131,7 +132,8 @@ int ima_store_template(struct ima_template_entry *entry,
* By extending the PCR with 0xFF's instead of with zeroes, the PCR
* value is invalidated.
*/
-void ima_add_violation(struct file *file, const unsigned char *filename,
+void ima_add_violation(struct ima_namespace *ns,
+ struct file *file, const unsigned char *filename,
struct integrity_iint_cache *iint,
const char *op, const char *cause)
{
@@ -145,14 +147,14 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
int result;
/* can overflow, only indicator */
- atomic_long_inc(&ima_htable.violations);
+ atomic_long_inc(&ns->ima_htable.violations);
result = ima_alloc_init_template(&event_data, &entry, NULL);
if (result < 0) {
result = -ENOMEM;
goto err_out;
}
- result = ima_store_template(entry, violation, inode,
+ result = ima_store_template(ns, entry, violation, inode,
filename, CONFIG_IMA_MEASURE_PCR_IDX);
if (result < 0)
ima_free_template_entry(entry);
@@ -299,7 +301,8 @@ 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,
+void ima_store_measurement(struct ima_namespace *ns,
+ struct integrity_iint_cache *iint,
struct file *file, const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
@@ -334,7 +337,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
return;
}
- result = ima_store_template(entry, violation, inode, filename, pcr);
+ result = ima_store_template(ns, entry, violation, inode, filename, pcr);
if ((!result || result == -EEXIST) && !(file->f_flags & O_DIRECT)) {
iint->flags |= IMA_MEASURED;
iint->measured_pcrs |= (0x1 << pcr);
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index fc0413c8c358..9df8648ad64d 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -53,7 +53,9 @@ static ssize_t ima_show_htable_violations(struct file *filp,
char __user *buf,
size_t count, loff_t *ppos)
{
- return ima_show_htable_value(buf, count, ppos, &ima_htable.violations);
+ struct ima_namespace *ns = get_current_ns();
+
+ return ima_show_htable_value(buf, count, ppos, &ns->ima_htable.violations);
}
static const struct file_operations ima_htable_violations_ops = {
@@ -65,8 +67,9 @@ static ssize_t ima_show_measurements_count(struct file *filp,
char __user *buf,
size_t count, loff_t *ppos)
{
- return ima_show_htable_value(buf, count, ppos, &ima_htable.len);
+ struct ima_namespace *ns = get_current_ns();
+ return ima_show_htable_value(buf, count, ppos, &ns->ima_htable.len);
}
static const struct file_operations ima_measurements_count_ops = {
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 2ec9a22bbddf..6104d5116a7f 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -39,7 +39,7 @@ struct tpm_chip *ima_tpm_chip;
* a different value.) Violations add a zero entry to the measurement
* list and extend the aggregate PCR value with ff...ff's.
*/
-static int __init ima_add_boot_aggregate(void)
+static int __init ima_add_boot_aggregate(struct ima_namespace *ns)
{
static const char op[] = "add_boot_aggregate";
const char *audit_cause = "ENOMEM";
@@ -86,7 +86,7 @@ static int __init ima_add_boot_aggregate(void)
goto err_out;
}
- result = ima_store_template(entry, violation, NULL,
+ result = ima_store_template(ns, entry, violation, NULL,
boot_aggregate_name,
CONFIG_IMA_MEASURE_PCR_IDX);
if (result < 0) {
@@ -145,7 +145,7 @@ int __init ima_init(void)
rc = ima_init_digests();
if (rc != 0)
return rc;
- rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry */
+ rc = ima_add_boot_aggregate(&init_ima_ns); /* boot aggregate must be first entry */
if (rc != 0)
return rc;
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index 2d644791a795..e13adc3287ed 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -40,6 +40,10 @@ int ima_init_namespace(struct ima_namespace *ns)
ns->ima_rules = (struct list_head __rcu *)(&ns->ima_default_rules);
ns->ima_policy_flag = 0;
+ atomic_long_set(&ns->ima_htable.len, 0);
+ atomic_long_set(&ns->ima_htable.violations, 0);
+ memset(&ns->ima_htable.queue, 0, sizeof(ns->ima_htable.queue));
+
return 0;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 9cf1fd7c70bf..d692c9d53a98 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -112,7 +112,8 @@ static int mmap_violation_check(enum ima_hooks func, struct file *file,
* could result in a file measurement error.
*
*/
-static void ima_rdwr_violation_check(struct file *file,
+static void ima_rdwr_violation_check(struct ima_namespace *ns,
+ struct file *file,
struct integrity_iint_cache *iint,
int must_measure,
char **pathbuf,
@@ -145,10 +146,10 @@ static void ima_rdwr_violation_check(struct file *file,
*pathname = ima_d_path(&file->f_path, pathbuf, filename);
if (send_tomtou)
- ima_add_violation(file, *pathname, iint,
+ ima_add_violation(ns, file, *pathname, iint,
"invalid_pcr", "ToMToU");
if (send_writers)
- ima_add_violation(file, *pathname, iint,
+ ima_add_violation(ns, file, *pathname, iint,
"invalid_pcr", "open_writers");
}
@@ -256,7 +257,7 @@ static int process_measurement(struct ima_namespace *ns,
}
if (!rc && violation_check)
- ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
+ ima_rdwr_violation_check(ns, file, iint, action & IMA_MEASURE,
&pathbuf, &pathname, filename);
inode_unlock(inode);
@@ -353,7 +354,7 @@ static int process_measurement(struct ima_namespace *ns,
pathname = ima_d_path(&file->f_path, &pathbuf, filename);
if (action & IMA_MEASURE)
- ima_store_measurement(iint, file, pathname,
+ ima_store_measurement(ns, iint, file, pathname,
xattr_value, xattr_len, modsig, pcr,
template_desc);
if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
@@ -969,7 +970,7 @@ int process_buffer_measurement(struct ima_namespace *ns,
goto out;
}
- ret = ima_store_template(entry, violation, NULL, event_data.buf, pcr);
+ ret = ima_store_template(ns, entry, violation, NULL, event_data.buf, pcr);
if (ret < 0) {
audit_cause = "store_entry";
ima_free_template_entry(entry);
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 532da87ce519..373154039b91 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -17,6 +17,7 @@
#include <linux/rculist.h>
#include <linux/slab.h>
+#include <linux/ima.h>
#include "ima.h"
#define AUDIT_CAUSE_LEN_MAX 32
@@ -31,21 +32,22 @@ static unsigned long binary_runtime_size;
static unsigned long binary_runtime_size = ULONG_MAX;
#endif
-/* key: inode (before secure-hashing a file) */
-struct ima_h_table ima_htable = {
- .len = ATOMIC_LONG_INIT(0),
- .violations = ATOMIC_LONG_INIT(0),
- .queue[0 ... IMA_MEASURE_HTABLE_SIZE - 1] = HLIST_HEAD_INIT
-};
-
/* mutex protects atomicity of extending measurement list
* 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);
+
+static inline unsigned int ima_hash_key(u8 *digest)
+{
+ /* there is no point in taking a hash of part of a digest */
+ return (digest[0] | digest[1] << 8) % IMA_MEASURE_HTABLE_SIZE;
+}
+
/* 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,
+static struct ima_queue_entry *ima_lookup_digest_entry(struct ima_namespace *ns,
+ u8 *digest_value,
int pcr)
{
struct ima_queue_entry *qe, *ret = NULL;
@@ -54,7 +56,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
key = ima_hash_key(digest_value);
rcu_read_lock();
- hlist_for_each_entry_rcu(qe, &ima_htable.queue[key], hnext) {
+ hlist_for_each_entry_rcu(qe, &ns->ima_htable.queue[key], hnext) {
rc = memcmp(qe->entry->digests[ima_hash_algo_idx].digest,
digest_value, hash_digest_size[ima_hash_algo]);
if ((rc == 0) && (qe->entry->pcr == pcr)) {
@@ -90,7 +92,8 @@ static int get_binary_runtime_size(struct ima_template_entry *entry)
*
* (Called with ima_extend_list_mutex held.)
*/
-static int ima_add_digest_entry(struct ima_template_entry *entry,
+static int ima_add_digest_entry(struct ima_namespace *ns,
+ struct ima_template_entry *entry,
bool update_htable)
{
struct ima_queue_entry *qe;
@@ -106,11 +109,12 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
INIT_LIST_HEAD(&qe->later);
list_add_tail_rcu(&qe->later, &ima_measurements);
- atomic_long_inc(&ima_htable.len);
+ atomic_long_inc(&ns->ima_htable.len);
if (update_htable) {
key = ima_hash_key(entry->digests[ima_hash_algo_idx].digest);
- hlist_add_head_rcu(&qe->hnext, &ima_htable.queue[key]);
- }
+ hlist_add_head_rcu(&qe->hnext, &ns->ima_htable.queue[key]);
+ } else
+ INIT_HLIST_NODE(&qe->hnext);
if (binary_runtime_size != ULONG_MAX) {
int size;
@@ -156,7 +160,8 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
* kexec, maintain the total memory size required for serializing the
* binary_runtime_measurements.
*/
-int ima_add_template_entry(struct ima_template_entry *entry, int violation,
+int ima_add_template_entry(struct ima_namespace *ns,
+ struct ima_template_entry *entry, int violation,
const char *op, struct inode *inode,
const unsigned char *filename)
{
@@ -169,14 +174,14 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
mutex_lock(&ima_extend_list_mutex);
if (!violation && !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE)) {
- if (ima_lookup_digest_entry(digest, entry->pcr)) {
+ if (ima_lookup_digest_entry(ns, digest, entry->pcr)) {
audit_cause = "hash_exists";
result = -EEXIST;
goto out;
}
}
- result = ima_add_digest_entry(entry,
+ result = ima_add_digest_entry(ns, entry,
!IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
if (result < 0) {
audit_cause = "ENOMEM";
@@ -201,12 +206,13 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
return result;
}
-int ima_restore_measurement_entry(struct ima_template_entry *entry)
+int ima_restore_measurement_entry(struct ima_namespace *ns,
+ struct ima_template_entry *entry)
{
int result = 0;
mutex_lock(&ima_extend_list_mutex);
- result = ima_add_digest_entry(entry, 0);
+ result = ima_add_digest_entry(ns, entry, 0);
mutex_unlock(&ima_extend_list_mutex);
return result;
}
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 694560396be0..2ae87eb23a59 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -400,7 +400,7 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
}
/* Restore the serialized binary measurement list without extending PCRs. */
-int ima_restore_measurement_list(loff_t size, void *buf)
+int ima_restore_measurement_list(struct ima_namespace *ns, loff_t size, void *buf)
{
char template_name[MAX_TEMPLATE_NAME_LEN];
unsigned char zero[TPM_DIGEST_SIZE] = { 0 };
@@ -516,7 +516,7 @@ int ima_restore_measurement_list(loff_t size, void *buf)
entry->pcr = !ima_canonical_fmt ? *(u32 *)(hdr[HDR_PCR].data) :
le32_to_cpu(*(__le32 *)(hdr[HDR_PCR].data));
- ret = ima_restore_measurement_entry(entry);
+ ret = ima_restore_measurement_entry(ns, entry);
if (ret < 0)
break;
--
2.31.1
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.
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 11 ++++++++
security/integrity/ima/ima.h | 12 +++++----
security/integrity/ima/ima_fs.c | 3 ++-
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 | 34 ++++++++++--------------
7 files changed, 44 insertions(+), 30 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index cc0e8c509fa2..4b5dada581e4 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..b89cd69df0de 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"
@@ -430,7 +431,7 @@ static int ima_release_policy(struct inode *inode, struct file *file)
return 0;
}
- ima_update_policy();
+ ima_update_policy(get_current_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 1a44963e8ba9..3bc2d3611739 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);
+#endif
+
return 0;
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 320ca80aacab..e5aef287d14d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -986,7 +986,7 @@ int ima_check_policy(void)
* Policy rules are never deleted so ima_policy_flag gets zeroed only once when
* we switch from the default policy to user defined.
*/
-void ima_update_policy(void)
+void ima_update_policy(struct ima_namespace *ns)
{
struct list_head *policy = &ima_policy_rules;
@@ -1007,7 +1007,7 @@ void ima_update_policy(void)
ima_update_policy_flags();
/* Custom IMA policy has been loaded */
- ima_process_queued_keys();
+ ima_process_queued_keys(ns);
}
/* Keep the enumeration in sync with the policy_tokens! */
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 93056c03bf5a..fcaa1645dba3 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -10,6 +10,7 @@
#include <linux/user_namespace.h>
#include <linux/workqueue.h>
+#include <linux/ima.h>
#include <keys/asymmetric-type.h>
#include "ima.h"
@@ -25,34 +26,27 @@ static bool ima_process_keys;
static DEFINE_MUTEX(ima_keys_lock);
static LIST_HEAD(ima_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.
- */
-static long ima_key_queue_timeout = 300000; /* 5 Minutes */
-static void ima_keys_handler(struct work_struct *work);
-static DECLARE_DELAYED_WORK(ima_keys_delayed_work, ima_keys_handler);
-static bool timer_expired;
-
/*
* This worker function frees keys that may still be
* queued up in case custom IMA policy was not loaded.
*/
-static void ima_keys_handler(struct work_struct *work)
+void ima_keys_handler(struct work_struct *work)
{
- timer_expired = true;
- ima_process_queued_keys();
+ struct ima_namespace *ns;
+
+ ns = container_of(work, struct ima_namespace, ima_keys_delayed_work.work);
+ ns->timer_expired = true;
+ ima_process_queued_keys(ns);
}
/*
* This function sets up a worker to free queued keys in case
* custom IMA policy was never loaded.
*/
-void ima_init_key_queue(void)
+void ima_init_key_queue(struct ima_namespace *ns)
{
- schedule_delayed_work(&ima_keys_delayed_work,
- msecs_to_jiffies(ima_key_queue_timeout));
+ schedule_delayed_work(&ns->ima_keys_delayed_work,
+ msecs_to_jiffies(ns->ima_key_queue_timeout));
}
static void ima_free_key_entry(struct ima_key_entry *entry)
@@ -130,7 +124,7 @@ bool ima_queue_key(struct key *keyring, const void *payload,
* This function sets ima_process_keys to true and processes queued keys.
* From here on keys will be processed right away (not queued).
*/
-void ima_process_queued_keys(void)
+void ima_process_queued_keys(struct ima_namespace *ns)
{
struct ima_key_entry *entry, *tmp;
bool process = false;
@@ -154,11 +148,11 @@ void ima_process_queued_keys(void)
if (!process)
return;
- if (!timer_expired)
- cancel_delayed_work_sync(&ima_keys_delayed_work);
+ if (!ns->timer_expired)
+ cancel_delayed_work_sync(&ns->ima_keys_delayed_work);
list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
- if (!timer_expired)
+ if (!ns->timer_expired)
process_buffer_measurement(&init_user_ns, NULL,
entry->payload,
entry->payload_len,
--
2.31.1
From: Mehmet Kayaalp <[email protected]>
The iint cache stores whether the file is measured, appraised, audited
etc. This patch moves the IMA_AUDITED flag into the per-namespace
ns_status, enabling IMA audit mechanism to audit the same file each time
it is accessed in a new namespace.
The ns_status is not looked up if the CONFIG_IMA_NS is disabled or if
any of the IMA_NS_STATUS_ACTIONS (currently only IMA_AUDIT) is not
enabled.
Read and write operations on the iint flags is replaced with function
calls. For reading, iint_flags() returns the bitwise AND of iint->flags
and ns_status->flags. The ns_status flags are masked with
IMA_NS_STATUS_FLAGS (currently only IMA_AUDITED). Similarly
set_iint_flags() only writes the masked portion to the ns_status flags,
while the iint flags is set as before. The ns_status parameter added to
ima_audit_measurement() is used with the above functions to query and
set the ns_status flags.
Signed-off-by: Mehmet Kayaalp <[email protected]>
Changelog:
v2:
* fixed flag calculation in iint_flags()
---
init/Kconfig | 3 +++
security/integrity/ima/ima.h | 23 ++++++++++++++++++++++-
security/integrity/ima/ima_api.c | 8 +++++---
security/integrity/ima/ima_main.c | 24 +++++++++++++++++-------
security/integrity/ima/ima_ns.c | 20 ++++++++++++++++++++
5 files changed, 67 insertions(+), 11 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig
index 27890607e8cb..1e1c49f1d129 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1251,6 +1251,9 @@ config IMA_NS
Allow the creation of IMA namespaces for each user namespace.
Namespaced IMA enables having IMA features work separately
in each IMA namespace.
+ Currently, only the audit status flags are stored in the namespace,
+ which allows the same file to be audited each time it is accessed
+ in a new namespace.
endif # NAMESPACES
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 28896d256e36..dd06e16c4e1c 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -282,7 +282,8 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
int pcr, const char *func_data,
bool buf_hash, u8 *digest, size_t digest_len);
void ima_audit_measurement(struct integrity_iint_cache *iint,
- const unsigned char *filename);
+ const unsigned char *filename,
+ struct ns_status *status);
int ima_alloc_init_template(struct ima_event_data *event_data,
struct ima_template_entry **entry,
struct ima_template_desc *template_desc);
@@ -426,6 +427,9 @@ static inline void ima_free_modsig(struct modsig *modsig)
}
#endif /* CONFIG_IMA_APPRAISE_MODSIG */
+#define IMA_NS_STATUS_ACTIONS IMA_AUDIT
+#define IMA_NS_STATUS_FLAGS IMA_AUDITED
+
int ima_ns_init(void);
struct ima_namespace;
int ima_init_namespace(struct ima_namespace *ns);
@@ -436,6 +440,10 @@ struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
void free_ns_status_cache(struct ima_namespace *ns);
+unsigned long iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status);
+unsigned long set_iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status, unsigned long flags);
#else
static inline struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
@@ -444,6 +452,19 @@ static inline struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
return NULL;
}
+static inline unsigned long iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status)
+{
+ return iint->flags;
+}
+
+static inline unsigned long set_iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status,
+ unsigned long flags)
+{
+ iint->flags = flags;
+ return flags;
+}
#endif /* CONFIG_IMA_NS */
/* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index a64fb0130b01..8f7bab17b784 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -342,14 +342,16 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
}
void ima_audit_measurement(struct integrity_iint_cache *iint,
- const unsigned char *filename)
+ const unsigned char *filename,
+ struct ns_status *status)
{
struct audit_buffer *ab;
char *hash;
const char *algo_name = hash_algo_name[iint->ima_hash->algo];
int i;
+ unsigned long flags = iint_flags(iint, status);
- if (iint->flags & IMA_AUDITED)
+ if (flags & IMA_AUDITED)
return;
hash = kzalloc((iint->ima_hash->length * 2) + 1, GFP_KERNEL);
@@ -372,7 +374,7 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_task_info(ab);
audit_log_end(ab);
- iint->flags |= IMA_AUDITED;
+ set_iint_flags(iint, status, flags | IMA_AUDITED);
out:
kfree(hash);
return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 465865412100..4df60dbb56f7 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -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 ns_status *status = NULL;
struct ima_template_desc *template_desc = NULL;
char *pathbuf = NULL;
char filename[NAME_MAX];
@@ -216,6 +217,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
bool violation_check;
enum hash_algo hash_algo;
unsigned int allowed_algos = 0;
+ unsigned long flags;
if (!ima_policy_flag || !S_ISREG(inode->i_mode))
return 0;
@@ -244,6 +246,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
iint = integrity_inode_get(inode);
if (!iint)
rc = -ENOMEM;
+
+ if (!rc && (action & IMA_NS_STATUS_ACTIONS)) {
+ status = ima_get_ns_status(get_current_ns(), inode);
+ if (IS_ERR(status))
+ rc = PTR_ERR(status);
+ }
}
if (!rc && violation_check)
@@ -259,11 +267,13 @@ static int process_measurement(struct file *file, const struct cred *cred,
mutex_lock(&iint->mutex);
+ flags = iint_flags(iint, status);
+
if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags))
/* reset appraisal flags if ima_inode_post_setattr was called */
- iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
- IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
- IMA_ACTION_FLAGS);
+ flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
+ IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
+ IMA_ACTION_FLAGS);
/*
* Re-evaulate the file if either the xattr has changed or the
@@ -274,7 +284,7 @@ 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;
+ flags &= ~IMA_DONE_MASK;
iint->measured_pcrs = 0;
}
@@ -282,9 +292,9 @@ static int process_measurement(struct file *file, const struct cred *cred,
* (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
* IMA_AUDIT, IMA_AUDITED)
*/
- iint->flags |= action;
+ flags = set_iint_flags(iint, status, flags | action);
action &= IMA_DO_MASK;
- action &= ~((iint->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1);
+ action &= ~((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)))
@@ -359,7 +369,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
&pathname, filename);
}
if (action & IMA_AUDIT)
- ima_audit_measurement(iint, pathname);
+ ima_audit_measurement(iint, pathname, status);
if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
rc = 0;
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 566e59524958..1fe1d910996b 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -75,6 +75,26 @@ void free_ima_ns(struct kref *kref)
destroy_ima_ns(ns);
}
+unsigned long iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status)
+{
+ if (!status)
+ return iint->flags;
+
+ return (iint->flags & ~IMA_NS_STATUS_FLAGS) |
+ (status->flags & IMA_NS_STATUS_FLAGS);
+}
+
+unsigned long set_iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status, unsigned long flags)
+{
+ iint->flags = flags;
+ if (status)
+ status->flags = flags & IMA_NS_STATUS_FLAGS;
+
+ return flags;
+}
+
static int __init imans_cache_init(void)
{
imans_cachep = KMEM_CACHE(ima_namespace, SLAB_PANIC);
--
2.31.1
Pass the securityfs_-prefixed static variables from current top level
functions so that new APIs allow callers to pass in similar parameters and
thus share most of the existing functions.
Signed-off-by: Stefan Berger <[email protected]>
---
security/inode.c | 92 ++++++++++++++++++++++++++++++------------------
1 file changed, 58 insertions(+), 34 deletions(-)
diff --git a/security/inode.c b/security/inode.c
index 71d93108de55..9299f134877f 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -88,6 +88,11 @@ static struct file_system_type fs_type = {
* this file.
* @iops: a point to a struct of inode_operations that should be used for
* this file/dir
+ * @mount: a pointer to a pointer for existing vfsmount to use or for
+ * one to create
+ * @mount_count: pointer to integer for mount_count that goes along with
+ * @mount
+ *
*
* This is the basic "create a file/dir/symlink" function for
* securityfs. It allows for a wide range of flexibility in creating
@@ -107,7 +112,8 @@ static struct file_system_type fs_type = {
static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
struct dentry *parent, void *data,
const struct file_operations *fops,
- const struct inode_operations *iops)
+ const struct inode_operations *iops,
+ struct vfsmount **mount, int *mount_count)
{
struct dentry *dentry;
struct inode *dir, *inode;
@@ -118,12 +124,12 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
pr_debug("securityfs: creating file '%s'\n",name);
- error = simple_pin_fs(&fs_type, &securityfs_mount, &securityfs_mount_count);
+ error = simple_pin_fs(&fs_type, mount, mount_count);
if (error)
return ERR_PTR(error);
if (!parent)
- parent = securityfs_mount->mnt_root;
+ parent = (*mount)->mnt_root;
dir = d_inode(parent);
@@ -168,7 +174,7 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
dentry = ERR_PTR(error);
out:
inode_unlock(dir);
- simple_release_fs(&securityfs_mount, &securityfs_mount_count);
+ simple_release_fs(mount, mount_count);
return dentry;
}
@@ -201,7 +207,9 @@ struct dentry *securityfs_create_file(const char *name, umode_t mode,
struct dentry *parent, void *data,
const struct file_operations *fops)
{
- return securityfs_create_dentry(name, mode, parent, data, fops, NULL);
+ return securityfs_create_dentry(name, mode, parent, data, fops, NULL,
+ &securityfs_mount,
+ &securityfs_mount_count);
}
EXPORT_SYMBOL_GPL(securityfs_create_file);
@@ -231,6 +239,27 @@ struct dentry *securityfs_create_dir(const char *name, struct dentry *parent)
}
EXPORT_SYMBOL_GPL(securityfs_create_dir);
+static struct dentry *_securityfs_create_symlink(const char *name,
+ struct dentry *parent,
+ const char *target,
+ const struct inode_operations *iops,
+ struct vfsmount **mount, int *mount_count)
+{
+ struct dentry *dent;
+ char *link = NULL;
+
+ if (target) {
+ link = kstrdup(target, GFP_KERNEL);
+ if (!link)
+ return ERR_PTR(-ENOMEM);
+ }
+ dent = securityfs_create_dentry(name, S_IFLNK | 0444, parent,
+ link, NULL, iops, mount, mount_count);
+ if (IS_ERR(dent))
+ kfree(link);
+
+ return dent;
+}
/**
* securityfs_create_symlink - create a symlink in the securityfs filesystem
*
@@ -262,37 +291,13 @@ struct dentry *securityfs_create_symlink(const char *name,
const char *target,
const struct inode_operations *iops)
{
- struct dentry *dent;
- char *link = NULL;
-
- if (target) {
- link = kstrdup(target, GFP_KERNEL);
- if (!link)
- return ERR_PTR(-ENOMEM);
- }
- dent = securityfs_create_dentry(name, S_IFLNK | 0444, parent,
- link, NULL, iops);
- if (IS_ERR(dent))
- kfree(link);
-
- return dent;
+ return _securityfs_create_symlink(name, parent, target, iops,
+ &securityfs_mount, &securityfs_mount_count);
}
EXPORT_SYMBOL_GPL(securityfs_create_symlink);
-/**
- * securityfs_remove - removes a file or directory from the securityfs filesystem
- *
- * @dentry: a pointer to a the dentry of the file or directory to be removed.
- *
- * This function removes a file or directory in securityfs that was previously
- * created with a call to another securityfs function (like
- * securityfs_create_file() or variants thereof.)
- *
- * This function is required to be called in order for the file to be
- * removed. No automatic cleanup of files will happen when a module is
- * removed; you are responsible here.
- */
-void securityfs_remove(struct dentry *dentry)
+static void _securityfs_remove(struct dentry *dentry,
+ struct vfsmount **mount, int *mount_count)
{
struct inode *dir;
@@ -309,8 +314,27 @@ void securityfs_remove(struct dentry *dentry)
dput(dentry);
}
inode_unlock(dir);
- simple_release_fs(&securityfs_mount, &securityfs_mount_count);
+ simple_release_fs(mount, mount_count);
+}
+
+/**
+ * securityfs_remove - removes a file or directory from the securityfs filesystem
+ *
+ * @dentry: a pointer to a the dentry of the file or directory to be removed.
+ *
+ * This function removes a file or directory in securityfs that was previously
+ * created with a call to another securityfs function (like
+ * securityfs_create_file() or variants thereof.)
+ *
+ * This function is required to be called in order for the file to be
+ * removed. No automatic cleanup of files will happen when a module is
+ * removed; you are responsible here.
+ */
+void securityfs_remove(struct dentry *dentry)
+{
+ _securityfs_remove(dentry, &securityfs_mount, &securityfs_mount_count);
}
+
EXPORT_SYMBOL_GPL(securityfs_remove);
#ifdef CONFIG_SECURITY
--
2.31.1
Implement hierarchical processing of file accesses in IMA namespaces by
walking the list of IMA namespaces towards the init_ima_ns. This way
file accesses can be audited in an IMA namespace and also be evaluated
against the IMA policies of parent IMA namespaces.
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/ima/ima_main.c | 29 +++++++++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index d692c9d53a98..42cbcaf2dc1e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -199,10 +199,10 @@ void ima_file_free(struct file *file)
ima_check_last_writer(iint, inode, file);
}
-static int process_measurement(struct ima_namespace *ns,
- struct file *file, const struct cred *cred,
- u32 secid, char *buf, loff_t size, int mask,
- enum ima_hooks func)
+static int _process_measurement(struct ima_namespace *ns,
+ struct file *file, const struct cred *cred,
+ u32 secid, char *buf, loff_t size, int mask,
+ enum ima_hooks func)
{
struct inode *inode = file_inode(file);
struct integrity_iint_cache *iint = NULL;
@@ -404,6 +404,27 @@ static int process_measurement(struct ima_namespace *ns,
return 0;
}
+static int process_measurement(struct ima_namespace *ns,
+ struct file *file, const struct cred *cred,
+ u32 secid, char *buf, loff_t size, int mask,
+ enum ima_hooks func)
+{
+ int ret = 0;
+ struct user_namespace *user_ns;
+
+ do {
+ ret = _process_measurement(ns, file, cred, secid, buf, size, mask, func);
+ if (ret)
+ break;
+ user_ns = ns->user_ns->parent;
+ if (!user_ns)
+ break;
+ ns = user_ns->ima_ns;
+ } while (1);
+
+ return ret;
+}
+
/**
* ima_file_mmap - based on policy, collect/store measurement.
* @file: pointer to the file to be measured (May be NULL)
--
2.31.1
Move the ima_write_mutex, ima_fs_flag, and valid_policy variables into
ima_namespace. This way each IMA namespace can set those variables
independently.
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 5 ++++
security/integrity/ima/ima_fs.c | 35 +++++++++++-------------
security/integrity/ima/ima_init_ima_ns.c | 4 +++
3 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 53f944469de7..889e9c70cbfb 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -261,6 +261,11 @@ struct ima_namespace {
struct ima_h_table ima_htable;
struct list_head ima_measurements;
unsigned long binary_runtime_size;
+
+ /* IMA's filesystem */
+ struct mutex ima_write_mutex;
+ unsigned long ima_fs_flags;
+ int valid_policy;
};
extern struct ima_namespace init_ima_ns;
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index c35e15fb313f..6c86f81c9998 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -25,8 +25,6 @@
#include "ima.h"
-static DEFINE_MUTEX(ima_write_mutex);
-
bool ima_canonical_fmt;
static int __init default_canonical_fmt_setup(char *str)
{
@@ -37,8 +35,6 @@ static int __init default_canonical_fmt_setup(char *str)
}
__setup("ima_canonical_fmt", default_canonical_fmt_setup);
-static int valid_policy = 1;
-
static ssize_t ima_show_htable_value(char __user *buf, size_t count,
loff_t *ppos, atomic_long_t *val)
{
@@ -320,6 +316,7 @@ static ssize_t ima_read_policy(char *path)
static ssize_t ima_write_policy(struct file *file, const char __user *buf,
size_t datalen, loff_t *ppos)
{
+ struct ima_namespace *ns = get_current_ns();
char *data;
ssize_t result;
@@ -337,7 +334,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
goto out;
}
- result = mutex_lock_interruptible(&ima_write_mutex);
+ result = mutex_lock_interruptible(&ns->ima_write_mutex);
if (result < 0)
goto out_free;
@@ -350,14 +347,14 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
1, 0);
result = -EACCES;
} else {
- result = ima_parse_add_rule(get_current_ns(), data);
+ result = ima_parse_add_rule(ns, data);
}
- mutex_unlock(&ima_write_mutex);
+ mutex_unlock(&ns->ima_write_mutex);
out_free:
kfree(data);
out:
if (result < 0)
- valid_policy = 0;
+ ns->valid_policy = 0;
return result;
}
@@ -374,8 +371,6 @@ enum ima_fs_flags {
IMA_FS_BUSY,
};
-static unsigned long ima_fs_flags;
-
#ifdef CONFIG_IMA_READ_POLICY
static const struct seq_operations ima_policy_seqops = {
.start = ima_policy_start,
@@ -390,6 +385,8 @@ static const struct seq_operations ima_policy_seqops = {
*/
static int ima_open_policy(struct inode *inode, struct file *filp)
{
+ struct ima_namespace *ns = get_current_ns();
+
if (!(filp->f_flags & O_WRONLY)) {
#ifndef CONFIG_IMA_READ_POLICY
return -EACCES;
@@ -401,7 +398,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
return seq_open(filp, &ima_policy_seqops);
#endif
}
- if (test_and_set_bit(IMA_FS_BUSY, &ima_fs_flags))
+ if (test_and_set_bit(IMA_FS_BUSY, &ns->ima_fs_flags))
return -EBUSY;
return 0;
}
@@ -415,25 +412,25 @@ 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();
+ const char *cause = ns->valid_policy ? "completed" : "failed";
if ((file->f_flags & O_ACCMODE) == O_RDONLY)
return seq_release(inode, file);
- if (valid_policy && ima_check_policy(ns) < 0) {
+ if (ns->valid_policy && ima_check_policy(ns) < 0) {
cause = "failed";
- valid_policy = 0;
+ ns->valid_policy = 0;
}
pr_info("policy update %s\n", cause);
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
- "policy_update", cause, !valid_policy, 0);
+ "policy_update", cause, !ns->valid_policy, 0);
- if (!valid_policy) {
+ if (!ns->valid_policy) {
ima_delete_rules(ns);
- valid_policy = 1;
- clear_bit(IMA_FS_BUSY, &ima_fs_flags);
+ ns->valid_policy = 1;
+ clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
return 0;
}
@@ -442,7 +439,7 @@ static int ima_release_policy(struct inode *inode, struct file *file)
securityfs_remove(ima_policy);
ima_policy = NULL;
#elif defined(CONFIG_IMA_WRITE_POLICY)
- clear_bit(IMA_FS_BUSY, &ima_fs_flags);
+ clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
#elif defined(CONFIG_IMA_READ_POLICY)
inode->i_mode &= ~S_IWUSR;
#endif
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index 57e46a10c001..22ff74e85a5f 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -49,6 +49,10 @@ int ima_init_namespace(struct ima_namespace *ns)
else
ns->binary_runtime_size = ULONG_MAX;
+ mutex_init(&ns->ima_write_mutex);
+ ns->valid_policy = 1;
+ ns->ima_fs_flags = 0;
+
return 0;
}
--
2.31.1
Move measurement list related variables into the ima_namespace. This way a
front-end like SecurityFS can show the measurement list inside an IMA
namespace.
Implement ima_free_measurements() to free a list of measurements
and call it when an IMA namespace is deleted.
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 2 ++
security/integrity/ima/ima.h | 4 +--
security/integrity/ima/ima_fs.c | 6 +++--
security/integrity/ima/ima_init_ima_ns.c | 5 ++++
security/integrity/ima/ima_ns.c | 1 +
security/integrity/ima/ima_queue.c | 33 ++++++++++++++----------
6 files changed, 33 insertions(+), 18 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 929bf87b1bbf..53f944469de7 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -259,6 +259,8 @@ struct ima_namespace {
int ima_policy_flag;
struct ima_h_table ima_htable;
+ struct list_head ima_measurements;
+ unsigned long binary_runtime_size;
};
extern struct ima_namespace init_ima_ns;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index a7e6c8fb152a..bb9763cd5fb1 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -104,7 +104,6 @@ struct ima_queue_entry {
struct list_head later; /* place in ima_measurements list */
struct ima_template_entry *entry;
};
-extern struct list_head ima_measurements; /* list of all measurements */
/* Some details preceding the binary serialized measurement list */
struct ima_kexec_hdr {
@@ -168,8 +167,9 @@ int ima_restore_measurement_entry(struct ima_namespace *ns,
struct ima_template_entry *entry);
int ima_restore_measurement_list(struct ima_namespace *ns,
loff_t bufsize, void *buf);
+void ima_free_measurements(struct ima_namespace *ns);
int ima_measurements_show(struct seq_file *m, void *v);
-unsigned long ima_get_binary_runtime_size(void);
+unsigned long ima_get_binary_runtime_size(struct ima_namespace *ns);
int ima_init_template(void);
void ima_init_template_list(void);
int __init ima_init_digests(void);
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 9df8648ad64d..c35e15fb313f 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -80,12 +80,13 @@ static const struct file_operations ima_measurements_count_ops = {
/* returns pointer to hlist_node */
static void *ima_measurements_start(struct seq_file *m, loff_t *pos)
{
+ struct ima_namespace *ns = get_current_ns();
loff_t l = *pos;
struct ima_queue_entry *qe;
/* we need a lock since pos could point beyond last element */
rcu_read_lock();
- list_for_each_entry_rcu(qe, &ima_measurements, later) {
+ list_for_each_entry_rcu(qe, &ns->ima_measurements, later) {
if (!l--) {
rcu_read_unlock();
return qe;
@@ -97,6 +98,7 @@ static void *ima_measurements_start(struct seq_file *m, loff_t *pos)
static void *ima_measurements_next(struct seq_file *m, void *v, loff_t *pos)
{
+ struct ima_namespace *ns = get_current_ns();
struct ima_queue_entry *qe = v;
/* lock protects when reading beyond last element
@@ -107,7 +109,7 @@ static void *ima_measurements_next(struct seq_file *m, void *v, loff_t *pos)
rcu_read_unlock();
(*pos)++;
- return (&qe->later == &ima_measurements) ? NULL : qe;
+ return (&qe->later == &ns->ima_measurements) ? NULL : qe;
}
static void ima_measurements_stop(struct seq_file *m, void *v)
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index e13adc3287ed..57e46a10c001 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -43,6 +43,11 @@ int ima_init_namespace(struct ima_namespace *ns)
atomic_long_set(&ns->ima_htable.len, 0);
atomic_long_set(&ns->ima_htable.violations, 0);
memset(&ns->ima_htable.queue, 0, sizeof(ns->ima_htable.queue));
+ INIT_LIST_HEAD(&ns->ima_measurements);
+ if (IS_ENABLED(CONFIG_IMA_KEXEC) && ns == &init_ima_ns)
+ ns->binary_runtime_size = 0;
+ else
+ ns->binary_runtime_size = ULONG_MAX;
return 0;
}
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index efbf7087a8ee..debe863364fd 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -69,6 +69,7 @@ static void destroy_ima_ns(struct ima_namespace *ns)
pr_debug("DESTROY ima_ns: 0x%p\n", ns);
ima_free_policy_rules(ns);
free_ns_status_cache(ns);
+ ima_free_measurements(ns);
kmem_cache_free(imans_cachep, ns);
}
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 373154039b91..f15f776918ec 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -25,13 +25,6 @@
/* pre-allocated array of tpm_digest structures to extend a PCR */
static struct tpm_digest *digests;
-LIST_HEAD(ima_measurements); /* list of all measurements */
-#ifdef CONFIG_IMA_KEXEC
-static unsigned long binary_runtime_size;
-#else
-static unsigned long binary_runtime_size = ULONG_MAX;
-#endif
-
/* mutex protects atomicity of extending measurement list
* 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.
@@ -107,7 +100,7 @@ static int ima_add_digest_entry(struct ima_namespace *ns,
qe->entry = entry;
INIT_LIST_HEAD(&qe->later);
- list_add_tail_rcu(&qe->later, &ima_measurements);
+ list_add_tail_rcu(&qe->later, &ns->ima_measurements);
atomic_long_inc(&ns->ima_htable.len);
if (update_htable) {
@@ -116,12 +109,12 @@ static int ima_add_digest_entry(struct ima_namespace *ns,
} else
INIT_HLIST_NODE(&qe->hnext);
- if (binary_runtime_size != ULONG_MAX) {
+ if (ns->binary_runtime_size != ULONG_MAX) {
int size;
size = get_binary_runtime_size(entry);
- binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ?
- binary_runtime_size + size : ULONG_MAX;
+ ns->binary_runtime_size = (ns->binary_runtime_size < ULONG_MAX - size) ?
+ ns->binary_runtime_size + size : ULONG_MAX;
}
return 0;
}
@@ -131,12 +124,12 @@ static int ima_add_digest_entry(struct ima_namespace *ns,
* entire binary_runtime_measurement list, including the ima_kexec_hdr
* structure.
*/
-unsigned long ima_get_binary_runtime_size(void)
+unsigned long ima_get_binary_runtime_size(struct ima_namespace *ns)
{
- if (binary_runtime_size >= (ULONG_MAX - sizeof(struct ima_kexec_hdr)))
+ if (ns->binary_runtime_size >= (ULONG_MAX - sizeof(struct ima_kexec_hdr)))
return ULONG_MAX;
else
- return binary_runtime_size + sizeof(struct ima_kexec_hdr);
+ return ns->binary_runtime_size + sizeof(struct ima_kexec_hdr);
}
static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
@@ -217,6 +210,18 @@ int ima_restore_measurement_entry(struct ima_namespace *ns,
return result;
}
+void ima_free_measurements(struct ima_namespace *ns)
+{
+ struct ima_queue_entry *qe, *tmp;
+
+ list_for_each_entry_safe(qe, tmp, &ns->ima_measurements, later) {
+ list_del(&qe->later);
+ if (!hlist_unhashed(&qe->hnext))
+ hlist_del(&qe->hnext);
+ kfree(qe);
+ }
+}
+
int __init ima_init_digests(void)
{
u16 digest_size;
--
2.31.1
Prefix global variables 'mount' and 'mount_count' with securityfs_ so they
are easier to distinguish as variables belonging to securityfs rather than
variables being passed in through new APIs we will introduce.
Signed-off-by: Stefan Berger <[email protected]>
---
security/inode.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/security/inode.c b/security/inode.c
index 6c326939750d..71d93108de55 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -22,8 +22,8 @@
#include <linux/lsm_hooks.h>
#include <linux/magic.h>
-static struct vfsmount *mount;
-static int mount_count;
+static struct vfsmount *securityfs_mount;
+static int securityfs_mount_count;
static void securityfs_free_inode(struct inode *inode)
{
@@ -118,12 +118,12 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
pr_debug("securityfs: creating file '%s'\n",name);
- error = simple_pin_fs(&fs_type, &mount, &mount_count);
+ error = simple_pin_fs(&fs_type, &securityfs_mount, &securityfs_mount_count);
if (error)
return ERR_PTR(error);
if (!parent)
- parent = mount->mnt_root;
+ parent = securityfs_mount->mnt_root;
dir = d_inode(parent);
@@ -168,7 +168,7 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
dentry = ERR_PTR(error);
out:
inode_unlock(dir);
- simple_release_fs(&mount, &mount_count);
+ simple_release_fs(&securityfs_mount, &securityfs_mount_count);
return dentry;
}
@@ -309,7 +309,7 @@ void securityfs_remove(struct dentry *dentry)
dput(dentry);
}
inode_unlock(dir);
- simple_release_fs(&mount, &mount_count);
+ simple_release_fs(&securityfs_mount, &securityfs_mount_count);
}
EXPORT_SYMBOL_GPL(securityfs_remove);
--
2.31.1
Use integrity_admin_ns_capable() to check corresponding capability to
allow read/write IMA policy without CAP_SYS_ADMIN but with
CAP_INTEGRITY_ADMIN.
Signed-off-by: Denis Semakin <[email protected]>
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/ima/ima_fs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 6c86f81c9998..6766bb8262f2 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -393,7 +393,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
#else
if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
return -EACCES;
- if (!capable(CAP_SYS_ADMIN))
+ if (!integrity_admin_ns_capable(ns->user_ns))
return -EPERM;
return seq_open(filp, &ima_policy_seqops);
#endif
--
2.31.1
Extend 'securityfs' for support of IMA namespacing so that each
IMA (user) namespace can have its own front-end for showing the currently
active policy, the measurement list, number of violations and so on. This
filesystem adds a new API call securityfs_ns_create_mount() for creating
a new instance of the filesystem.
The API calls the namespaced SecurityFS have the prefix securityfs_ns_ and
take additional parameters struct vfsmount ** and int *mount_count that
allow for access to different instances of this filesystem.
The filesystem can be mounted to the usual securityfs mount point like
this:
mount -t securityfs /sys/kernel/security /sys/kernel/security
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/security.h | 15 ++++
security/inode.c | 145 ++++++++++++++++++++++++++++++++++++++-
2 files changed, 159 insertions(+), 1 deletion(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 7e0ba63b5dde..13fdb09d86b2 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1929,6 +1929,21 @@ struct dentry *securityfs_create_symlink(const char *name,
const struct inode_operations *iops);
extern void securityfs_remove(struct dentry *dentry);
+extern struct dentry *securityfs_ns_create_file(const char *name, umode_t mode,
+ struct dentry *parent, void *data,
+ const struct file_operations *fops,
+ struct vfsmount **mount, int *mount_count);
+extern struct dentry *securityfs_ns_create_dir(const char *name, struct dentry *parent,
+ struct vfsmount **mount, int *mount_count);
+struct dentry *securityfs_ns_create_symlink(const char *name,
+ struct dentry *parent,
+ const char *target,
+ const struct inode_operations *iops,
+ struct vfsmount **mount, int *mount_count);
+extern void securityfs_ns_remove(struct dentry *dentry,
+ struct vfsmount **mount, int *mount_count);
+struct vfsmount *securityfs_ns_create_mount(struct user_namespace *user_ns);
+
#else /* CONFIG_SECURITYFS */
static inline struct dentry *securityfs_create_dir(const char *name,
diff --git a/security/inode.c b/security/inode.c
index 9299f134877f..2738a7b31469 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -21,6 +21,7 @@
#include <linux/security.h>
#include <linux/lsm_hooks.h>
#include <linux/magic.h>
+#include <linux/user_namespace.h>
static struct vfsmount *securityfs_mount;
static int securityfs_mount_count;
@@ -53,7 +54,7 @@ static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc)
static int securityfs_get_tree(struct fs_context *fc)
{
- return get_tree_single(fc, securityfs_fill_super);
+ return get_tree_keyed(fc, securityfs_fill_super, fc->user_ns);
}
static const struct fs_context_operations securityfs_context_ops = {
@@ -71,8 +72,34 @@ static struct file_system_type fs_type = {
.name = "securityfs",
.init_fs_context = securityfs_init_fs_context,
.kill_sb = kill_litter_super,
+ .fs_flags = FS_USERNS_MOUNT,
};
+/**
+ * securityfs_ns_create_mount - create instance of securityfs in given user namespace
+ *
+ * @user_ns: the user namespace to create the vfsmount in
+ *
+ * This function returns a pointer to the vfsmount or an error code. The vfsmount
+ * has to be used when creating or removing filesystem dentries.
+ */
+struct vfsmount *securityfs_ns_create_mount(struct user_namespace *user_ns)
+{
+ struct fs_context *fc;
+ struct vfsmount *mnt;
+
+ fc = fs_context_for_mount(&fs_type, SB_KERNMOUNT);
+ if (IS_ERR(fc))
+ return ERR_CAST(fc);
+
+ put_user_ns(fc->user_ns);
+ fc->user_ns = get_user_ns(user_ns);
+
+ mnt = fc_mount(fc);
+ put_fs_context(fc);
+ return mnt;
+}
+
/**
* securityfs_create_dentry - create a dentry in the securityfs filesystem
*
@@ -213,6 +240,40 @@ struct dentry *securityfs_create_file(const char *name, umode_t mode,
}
EXPORT_SYMBOL_GPL(securityfs_create_file);
+/**
+ * securityfs_ns_create_file - create a file in the securityfs_ns filesystem
+ *
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file. This should be a
+ * directory dentry if set. If this parameter is %NULL, then the
+ * file will be created in the root of the securityfs_ns filesystem.
+ * @data: a pointer to something that the caller will want to get to later
+ * on. The inode.i_private pointer will point to this value on
+ * the open() call.
+ * @fops: a pointer to a struct file_operations that should be used for
+ * this file.
+ * @mount: Pointer to a pointer of a an existing vfsmount
+ * @mount_count: The mount_count that goes along with the @mount
+ *
+ * This function creates a file in securityfs_ns with the given @name.
+ *
+ * This function returns a pointer to a dentry if it succeeds. This
+ * pointer must be passed to the securityfs_ns_remove() function when the file
+ * is to be removed (no automatic cleanup happens if your module is unloaded,
+ * you are responsible here). If an error occurs, the function will return
+ * the error value (via ERR_PTR).
+ */
+struct dentry *securityfs_ns_create_file(const char *name, umode_t mode,
+ struct dentry *parent, void *data,
+ const struct file_operations *fops,
+ struct vfsmount **mount, int *mount_count)
+{
+ return securityfs_create_dentry(name, mode, parent, data, fops, NULL,
+ mount, mount_count);
+}
+EXPORT_SYMBOL_GPL(securityfs_ns_create_file);
+
/**
* securityfs_create_dir - create a directory in the securityfs filesystem
*
@@ -239,6 +300,33 @@ struct dentry *securityfs_create_dir(const char *name, struct dentry *parent)
}
EXPORT_SYMBOL_GPL(securityfs_create_dir);
+/**
+ * securityfs_ns_create_dir - create a directory in the securityfs_ns filesystem
+ *
+ * @name: a pointer to a string containing the name of the directory to
+ * create.
+ * @parent: a pointer to the parent dentry for this file. This should be a
+ * directory dentry if set. If this parameter is %NULL, then the
+ * directory will be created in the root of the securityfs_ns filesystem.
+ * @mount: Pointer to a pointer of a an existing vfsmount
+ * @mount_count: The mount_count that goes along with the @mount
+ *
+ * This function creates a directory in securityfs_ns with the given @name.
+ *
+ * This function returns a pointer to a dentry if it succeeds. This
+ * pointer must be passed to the securityfs_ns_remove() function when the file
+ * is to be removed (no automatic cleanup happens if your module is unloaded,
+ * you are responsible here). If an error occurs, the function will return
+ * the error value (via ERR_PTR).
+ */
+struct dentry *securityfs_ns_create_dir(const char *name, struct dentry *parent,
+ struct vfsmount **mount, int *mount_count)
+{
+ return securityfs_ns_create_file(name, S_IFDIR | 0755, parent, NULL, NULL,
+ mount, mount_count);
+}
+EXPORT_SYMBOL_GPL(securityfs_ns_create_dir);
+
static struct dentry *_securityfs_create_symlink(const char *name,
struct dentry *parent,
const char *target,
@@ -260,6 +348,7 @@ static struct dentry *_securityfs_create_symlink(const char *name,
return dent;
}
+
/**
* securityfs_create_symlink - create a symlink in the securityfs filesystem
*
@@ -296,6 +385,39 @@ struct dentry *securityfs_create_symlink(const char *name,
}
EXPORT_SYMBOL_GPL(securityfs_create_symlink);
+/**
+ * securityfs_ns_create_symlink - create a symlink in the securityfs_ns filesystem
+ *
+ * @name: a pointer to a string containing the name of the symlink to
+ * create.
+ * @parent: a pointer to the parent dentry for the symlink. This should be a
+ * directory dentry if set. If this parameter is %NULL, then the
+ * directory will be created in the root of the securityfs_ns filesystem.
+ * @target: a pointer to a string containing the name of the symlink's target.
+ * If this parameter is %NULL, then the @iops parameter needs to be
+ * setup to handle .readlink and .get_link inode_operations.
+ * @mount: Pointer to a pointer of a an existing vfsmount
+ * @mount_count: The mount_count that goes along with the @mount
+ *
+ * This function creates a symlink in securityfs_ns with the given @name.
+ *
+ * This function returns a pointer to a dentry if it succeeds. This
+ * pointer must be passed to the securityfs_ns_remove() function when the file
+ * is to be removed (no automatic cleanup happens if your module is unloaded,
+ * you are responsible here). If an error occurs, the function will return
+ * the error value (via ERR_PTR).
+ */
+struct dentry *securityfs_ns_create_symlink(const char *name,
+ struct dentry *parent,
+ const char *target,
+ const struct inode_operations *iops,
+ struct vfsmount **mount, int *mount_count)
+{
+ return _securityfs_create_symlink(name, parent, target, iops,
+ mount, mount_count);
+}
+EXPORT_SYMBOL_GPL(securityfs_ns_create_symlink);
+
static void _securityfs_remove(struct dentry *dentry,
struct vfsmount **mount, int *mount_count)
{
@@ -337,6 +459,27 @@ void securityfs_remove(struct dentry *dentry)
EXPORT_SYMBOL_GPL(securityfs_remove);
+/**
+ * securityfs_ns_remove - removes a file or directory from the securityfs_ns filesystem
+ *
+ * @dentry: a pointer to a the dentry of the file or directory to be removed.
+ * @mount: Pointer to a pointer of a an existing vfsmount
+ * @mount_count: The mount_count that goes along with the @mount
+ *
+ * This function removes a file or directory in securityfs_ns that was previously
+ * created with a call to another securityfs_ns function (like
+ * securityfs_ns_create_file() or variants thereof.)
+ *
+ * This function is required to be called in order for the file to be
+ * removed. No automatic cleanup of files will happen when a module is
+ * removed; you are responsible here.
+ */
+void securityfs_ns_remove(struct dentry *dentry, struct vfsmount **mount, int *mount_count)
+{
+ _securityfs_remove(dentry, mount, mount_count);
+}
+EXPORT_SYMBOL_GPL(securityfs_ns_remove);
+
#ifdef CONFIG_SECURITY
static struct dentry *lsm_dentry;
static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count,
--
2.31.1
Only accept AUDIT rules for non-init_ima_ns namespaces rejecting all rules
that require support for measuring, appraisal, and hashing.
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/ima/ima_policy.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 96e7d63167e8..02e96da2faff 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1785,6 +1785,16 @@ static int ima_parse_rule(struct ima_namespace *ns,
result = -EINVAL;
break;
}
+
+ /* IMA namespace only accepts AUDIT rules */
+ if (ns != &init_ima_ns) {
+ switch (entry->action) {
+ case MEASURE:
+ case APPRAISE:
+ case HASH:
+ result = -EINVAL;
+ }
+ }
}
if (!result && !ima_validate_rule(entry))
result = -EINVAL;
--
2.31.1
From: Denis Semakin <[email protected]>
This patch introduces CAP_INTEGRITY_ADMIN, a new capability that allows
to setup IMA (Integrity Measurement Architecture) policies per container
for non-root users.
The main purpose of this new capability is discribed in this document:
https://kernsec.org/wiki/index.php/IMA_Namespacing_design_considerations
It is said: "setting the policy should be possibly without the powerful
CAP_SYS_ADMIN and there should be the opportunity to gate this with a new
capability CAP_INTEGRITY_ADMIN that allows a user to set the IMA policy
during container runtime.."
In other words it should be possible to setup IMA policies while not
giving too many privilges to the user, therefore splitting the
CAP_INTEGRITY_ADMIN off from CAP_SYS_ADMIN.
Signed-off-by: Denis Semakin <[email protected]>
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/capability.h | 6 ++++++
include/uapi/linux/capability.h | 7 ++++++-
security/selinux/include/classmap.h | 4 ++--
3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/include/linux/capability.h b/include/linux/capability.h
index 65efb74c3585..ea6d58acb95e 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -278,4 +278,10 @@ int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
int cap_convert_nscap(struct user_namespace *mnt_userns, struct dentry *dentry,
const void **ivalue, size_t size);
+static inline bool integrity_admin_ns_capable(struct user_namespace *ns)
+{
+ return ns_capable(ns, CAP_INTEGRITY_ADMIN) ||
+ ns_capable(ns, CAP_SYS_ADMIN);
+}
+
#endif /* !_LINUX_CAPABILITY_H */
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 463d1ba2232a..48b08e4b3895 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -417,7 +417,12 @@ struct vfs_ns_cap_data {
#define CAP_CHECKPOINT_RESTORE 40
-#define CAP_LAST_CAP CAP_CHECKPOINT_RESTORE
+/* Allow setup IMA policy per container independently */
+/* No necessary to be superuser */
+
+#define CAP_INTEGRITY_ADMIN 41
+
+#define CAP_LAST_CAP CAP_INTEGRITY_ADMIN
#define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 35aac62a662e..7ff532b90f09 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -28,9 +28,9 @@
#define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
"wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf", \
- "checkpoint_restore"
+ "checkpoint_restore", "integrity_admin"
-#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
+#if CAP_LAST_CAP > CAP_INTEGRITY_ADMIN
#error New capability defined, please update COMMON_CAP2_PERMS.
#endif
--
2.31.1
Extend the user_namespace structure with a refcount_teardown variable to
cause an early teardown function to be invoked. This allows the IMA
namespace to initialize a filesystem that holds one additional reference
to the user namespace it 'belongs' to. Therefore, the refount_teardown
variable will be incremented by '1' once that additional reference has
been created. Once the user namespace's reference counter is decremented
to '1', this early teardown function is invoked and the additional user
namespace reference released and the actual deletion of the user
namespace can then proceed as usual.
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/user_namespace.h | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 5249db04d62b..505e3b3748b6 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -103,6 +103,11 @@ struct user_namespace {
#ifdef CONFIG_IMA
struct ima_namespace *ima_ns;
#endif
+ /* The refcount at which to start tearing down dependent namespaces
+ * (currently only IMA) that may hold additional references to the
+ * user namespace.
+ */
+ unsigned int refcount_teardown;
} __randomize_layout;
struct ucounts {
@@ -156,8 +161,12 @@ extern void __put_user_ns(struct user_namespace *ns);
static inline void put_user_ns(struct user_namespace *ns)
{
- if (ns && refcount_dec_and_test(&ns->ns.count))
- __put_user_ns(ns);
+ if (ns) {
+ if (refcount_dec_and_test(&ns->ns.count))
+ __put_user_ns(ns);
+ else if (refcount_read(&ns->ns.count) == ns->refcount_teardown)
+ ;
+ }
}
struct seq_operations;
--
2.31.1
Setup securityfs with symlinks, directories, and files for IMA
namespacing support. The same directory structure that IMA uses on the
host is also created for the namespacing case.
Increment the user namespace's refcount_teardown value by '1' once
securityfs has been successfully setup since the initialization of the
filesystem causes an additional reference to the user namespace to be
taken. The early teardown function will delete the file system and release
the additional reference.
The securityfs file and directory ownerships cannot be set when the
IMA namespace is initialized. Therefore, delay the setup of the file
system to a later point when securityfs initializes the fs_context.
This filesystem can now be mounted as follows:
mount -t securityfs /sys/kernel/security/ /sys/kernel/security/
The following directories, symlinks, and files are then available.
$ ls -l sys/kernel/security/
total 0
lr--r--r--. 1 root root 0 Dec 2 00:18 ima -> integrity/ima
drwxr-xr-x. 3 root root 0 Dec 2 00:18 integrity
$ ls -l sys/kernel/security/ima/
total 0
-r--r-----. 1 root root 0 Dec 2 00:18 ascii_runtime_measurements
-r--r-----. 1 root root 0 Dec 2 00:18 binary_runtime_measurements
-rw-------. 1 root root 0 Dec 2 00:18 policy
-r--r-----. 1 root root 0 Dec 2 00:18 runtime_measurements_count
-r--r-----. 1 root root 0 Dec 2 00:18 violations
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 17 +++
security/inode.c | 8 ++
security/integrity/ima/ima.h | 2 +
security/integrity/ima/ima_fs.c | 157 ++++++++++++++++++++++-
security/integrity/ima/ima_init_ima_ns.c | 6 +-
security/integrity/ima/ima_ns.c | 2 +
6 files changed, 189 insertions(+), 3 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 889e9c70cbfb..a13f934f15fc 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -220,6 +220,18 @@ struct ima_h_table {
struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
};
+enum {
+ IMAFS_DENTRY_INTEGRITY_DIR = 0,
+ IMAFS_DENTRY_DIR,
+ IMAFS_DENTRY_SYMLINK,
+ IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS,
+ IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS,
+ IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT,
+ IMAFS_DENTRY_VIOLATIONS,
+ IMAFS_DENTRY_IMA_POLICY,
+ IMAFS_DENTRY_LAST
+};
+
struct ima_namespace {
struct kref kref;
struct user_namespace *user_ns;
@@ -266,6 +278,11 @@ struct ima_namespace {
struct mutex ima_write_mutex;
unsigned long ima_fs_flags;
int valid_policy;
+
+ struct dentry *dentry[IMAFS_DENTRY_LAST];
+ struct vfsmount *mount;
+ int mount_count;
+ int (*late_fs_init)(struct user_namespace *user_ns);
};
extern struct ima_namespace init_ima_ns;
diff --git a/security/inode.c b/security/inode.c
index 2738a7b31469..6223f1d838f6 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -22,6 +22,7 @@
#include <linux/lsm_hooks.h>
#include <linux/magic.h>
#include <linux/user_namespace.h>
+#include <linux/ima.h>
static struct vfsmount *securityfs_mount;
static int securityfs_mount_count;
@@ -63,6 +64,13 @@ static const struct fs_context_operations securityfs_context_ops = {
static int securityfs_init_fs_context(struct fs_context *fc)
{
+ int rc;
+
+ if (fc->user_ns->ima_ns->late_fs_init) {
+ rc = fc->user_ns->ima_ns->late_fs_init(fc->user_ns);
+ if (rc)
+ return rc;
+ }
fc->ops = &securityfs_context_ops;
return 0;
}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index bb9763cd5fb1..9bcd71bb716c 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -139,6 +139,8 @@ struct ns_status {
/* Internal IMA function definitions */
int ima_init(void);
int ima_fs_init(void);
+int ima_fs_ns_init(struct ima_namespace *ns);
+void ima_fs_ns_free(struct ima_namespace *ns);
int ima_add_template_entry(struct ima_namespace *ns,
struct ima_template_entry *entry, int violation,
const char *op, struct inode *inode,
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 6766bb8262f2..65b2af7c14dd 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -22,6 +22,7 @@
#include <linux/parser.h>
#include <linux/vmalloc.h>
#include <linux/ima.h>
+#include <linux/namei.h>
#include "ima.h"
@@ -436,8 +437,14 @@ static int ima_release_policy(struct inode *inode, struct file *file)
ima_update_policy(ns);
#if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
- securityfs_remove(ima_policy);
- ima_policy = NULL;
+ if (ns == &init_ima_ns) {
+ securityfs_remove(ima_policy);
+ ima_policy = NULL;
+ } else {
+ securityfs_ns_remove(ns->dentry[IMAFS_DENTRY_POLICY],
+ &ns->mount, &ns->mount_count);
+ ns->dentry[IMAFS_DENTRY_POLICY] = NULL;
+ }
#elif defined(CONFIG_IMA_WRITE_POLICY)
clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
#elif defined(CONFIG_IMA_READ_POLICY)
@@ -509,3 +516,149 @@ int __init ima_fs_init(void)
securityfs_remove(ima_policy);
return -1;
}
+
+static void ima_fs_ns_free_dentries(struct ima_namespace *ns)
+{
+ size_t i;
+
+ for (i = 0; i < IMAFS_DENTRY_LAST; i++) {
+ switch (i) {
+ case IMAFS_DENTRY_DIR:
+ case IMAFS_DENTRY_INTEGRITY_DIR:
+ /* files first */
+ continue;
+ }
+ securityfs_ns_remove(ns->dentry[i], &ns->mount, &ns->mount_count);
+ }
+ securityfs_ns_remove(ns->dentry[IMAFS_DENTRY_DIR],
+ &ns->mount, &ns->mount_count);
+ securityfs_ns_remove(ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR],
+ &ns->mount, &ns->mount_count);
+
+ memset(ns->dentry, 0, sizeof(ns->dentry));
+
+}
+
+/* Function to populeate namespace SecurityFS once user namespace
+ * has been configured.
+ */
+static int ima_fs_ns_late_init(struct user_namespace *user_ns)
+{
+ struct ima_namespace *ns = user_ns->ima_ns;
+ struct dentry *parent;
+
+ /* already initialized? */
+ if (ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR])
+ return 0;
+
+ ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] =
+ securityfs_ns_create_dir("integrity", NULL,
+ &ns->mount, &ns->mount_count);
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR])) {
+ ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] = NULL;
+ goto out;
+ }
+
+ ns->dentry[IMAFS_DENTRY_DIR] =
+ securityfs_ns_create_dir("ima", ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR],
+ &ns->mount, &ns->mount_count);
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR])) {
+ ns->dentry[IMAFS_DENTRY_DIR] = NULL;
+ goto out;
+ }
+
+ ns->dentry[IMAFS_DENTRY_SYMLINK] =
+ securityfs_ns_create_symlink("ima", NULL, "integrity/ima", NULL,
+ &ns->mount, &ns->mount_count);
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK])) {
+ ns->dentry[IMAFS_DENTRY_SYMLINK] = NULL;
+ goto out;
+ }
+
+ parent = ns->dentry[IMAFS_DENTRY_DIR];
+ ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS] =
+ securityfs_ns_create_file("binary_runtime_measurements",
+ S_IRUSR | S_IRGRP, parent, NULL,
+ &ima_measurements_ops,
+ &ns->mount, &ns->mount_count);
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS])) {
+ ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS] = NULL;
+ goto out;
+ }
+
+ ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS] =
+ securityfs_ns_create_file("ascii_runtime_measurements",
+ S_IRUSR | S_IRGRP, parent, NULL,
+ &ima_ascii_measurements_ops,
+ &ns->mount, &ns->mount_count);
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS])) {
+ ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS] = NULL;
+ goto out;
+ }
+
+ ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT] =
+ securityfs_ns_create_file("runtime_measurements_count",
+ S_IRUSR | S_IRGRP, parent, NULL,
+ &ima_measurements_count_ops,
+ &ns->mount, &ns->mount_count);
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT])) {
+ ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT] = NULL;
+ goto out;
+ }
+
+ ns->dentry[IMAFS_DENTRY_VIOLATIONS] =
+ securityfs_ns_create_file("violations", S_IRUSR | S_IRGRP,
+ parent, NULL, &ima_htable_violations_ops,
+ &ns->mount, &ns->mount_count);
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS])) {
+ ns->dentry[IMAFS_DENTRY_VIOLATIONS] = NULL;
+ goto out;
+ }
+
+ ns->dentry[IMAFS_DENTRY_IMA_POLICY] =
+ securityfs_ns_create_file("policy", POLICY_FILE_FLAGS,
+ parent, NULL, &ima_measure_policy_ops,
+ &ns->mount, &ns->mount_count);
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY])) {
+ ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
+ goto out;
+ }
+
+
+ return 0;
+
+out:
+ ima_fs_ns_free_dentries(ns);
+
+ return -1;
+}
+
+int ima_fs_ns_init(struct ima_namespace *ns)
+{
+ ns->mount = securityfs_ns_create_mount(ns->user_ns);
+ if (IS_ERR(ns->mount)) {
+ ns->mount = NULL;
+ return -1;
+ }
+ ns->mount_count = 1;
+
+ /* Adjust the trigger for user namespace's early teardown of dependent
+ * namespaces. Due to the filesystem there's an additional reference
+ * to the user namespace.
+ */
+ ns->user_ns->refcount_teardown += 1;
+
+ ns->late_fs_init = ima_fs_ns_late_init;
+
+ return 0;
+}
+
+void ima_fs_ns_free(struct ima_namespace *ns)
+{
+ ima_fs_ns_free_dentries(ns);
+ if (ns->mount) {
+ mntput(ns->mount);
+ ns->mount_count -= 1;
+ }
+ ns->mount = NULL;
+}
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index 22ff74e85a5f..86a89502c0c5 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -20,6 +20,8 @@
int ima_init_namespace(struct ima_namespace *ns)
{
+ int rc = 0;
+
ns->ns_status_tree = RB_ROOT;
rwlock_init(&ns->ns_status_lock);
ns->ns_status_cache = KMEM_CACHE(ns_status, SLAB_PANIC);
@@ -52,8 +54,10 @@ int ima_init_namespace(struct ima_namespace *ns)
mutex_init(&ns->ima_write_mutex);
ns->valid_policy = 1;
ns->ima_fs_flags = 0;
+ if (ns != &init_ima_ns)
+ rc = ima_fs_ns_init(ns);
- return 0;
+ return rc;
}
int __init ima_ns_init(void)
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 4260f96c4eca..9d5917c97fcc 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -67,6 +67,8 @@ struct ima_namespace *copy_ima_ns(struct ima_namespace *old_ns,
void ima_ns_userns_early_teardown(struct ima_namespace *ns)
{
+ pr_debug("%s: ns=0x%p\n", __func__, ns);
+ ima_fs_ns_free(ns);
}
EXPORT_SYMBOL(ima_ns_userns_early_teardown);
--
2.31.1
Move variables from keys queue into ima_namespace.
Some variables have to be initialized before ima_init() runs, so statically
initialize them for the init_ima_ns.
Since only init_ima_ns uses the queued keys there's no need to free the
list of queued keys when tearing down IMA namespaces.
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 11 ++++++
security/integrity/ima/ima.h | 9 ++---
security/integrity/ima/ima_asymmetric_keys.c | 5 +--
security/integrity/ima/ima_init_ima_ns.c | 5 +++
security/integrity/ima/ima_ns.c | 6 ++++
security/integrity/ima/ima_queue_keys.c | 37 +++++++-------------
6 files changed, 43 insertions(+), 30 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 4b5dada581e4..977df9155cde 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -219,6 +219,17 @@ struct ima_namespace {
struct kmem_cache *ns_status_cache;
#ifdef CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS
+ /*
+ * Flag to indicate whether a key can be processed
+ * right away or should be queued for processing later.
+ */
+ bool ima_process_keys;
+
+ /*
+ * To synchronize access to the list of keys that need to be measured
+ */
+ struct mutex ima_keys_lock;
+ struct list_head ima_keys;
/*
* If custom IMA policy is not loaded then keys queued up
* for measurement should be freed. This worker is used
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 9edab9050dc7..97eb03376855 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -250,14 +250,15 @@ struct ima_key_entry {
char *keyring_name;
};
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,
+bool ima_should_queue_key(struct ima_namespace *ns);
+bool ima_queue_key(struct ima_namespace *ns, struct key *keyring, const void *payload,
size_t payload_len);
void ima_process_queued_keys(struct ima_namespace *ns);
void ima_keys_handler(struct work_struct *work);
#else
-static inline bool ima_should_queue_key(void) { return false; }
-static inline bool ima_queue_key(struct key *keyring,
+static inline bool ima_should_queue_key(struct ima_namespace *ns) { return false; }
+static inline bool ima_queue_key(struct ima_namespace *ns,
+ struct key *keyring,
const void *payload,
size_t payload_len) { return false; }
static inline void ima_process_queued_keys(struct ima_namespace *ns) {}
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index f6aa0b47a772..b20e82eda8f4 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -30,6 +30,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
const void *payload, size_t payload_len,
unsigned long flags, bool create)
{
+ struct ima_namespace *ns = get_current_ns();
bool queued = false;
/* Only asymmetric keys are handled by this hook. */
@@ -39,8 +40,8 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
if (!payload || (payload_len == 0))
return;
- if (ima_should_queue_key())
- queued = ima_queue_key(keyring, payload, payload_len);
+ if (ima_should_queue_key(ns))
+ queued = ima_queue_key(ns, keyring, payload, payload_len);
if (queued)
return;
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index 3bc2d3611739..7b66fe598789 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -45,5 +45,10 @@ 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
};
EXPORT_SYMBOL(init_ima_ns);
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 1fe1d910996b..11e0343f1f55 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -38,6 +38,12 @@ static struct ima_namespace *create_ima_ns(struct user_namespace *user_ns)
if (err)
goto fail_free;
+#ifdef CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS
+ ns->ima_process_keys = false;
+ mutex_init(&ns->ima_keys_lock);
+ INIT_LIST_HEAD(&ns->ima_keys);
+#endif
+
return ns;
fail_free:
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index fcaa1645dba3..9e5e9a178253 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -14,17 +14,6 @@
#include <keys/asymmetric-type.h>
#include "ima.h"
-/*
- * Flag to indicate whether a key can be processed
- * right away or should be queued for processing later.
- */
-static bool ima_process_keys;
-
-/*
- * To synchronize access to the list of keys that need to be measured
- */
-static DEFINE_MUTEX(ima_keys_lock);
-static LIST_HEAD(ima_keys);
/*
* This worker function frees keys that may still be
@@ -95,7 +84,7 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
return entry;
}
-bool ima_queue_key(struct key *keyring, const void *payload,
+bool ima_queue_key(struct ima_namespace *ns, struct key *keyring, const void *payload,
size_t payload_len)
{
bool queued = false;
@@ -105,12 +94,12 @@ bool ima_queue_key(struct key *keyring, const void *payload,
if (!entry)
return false;
- mutex_lock(&ima_keys_lock);
- if (!ima_process_keys) {
- list_add_tail(&entry->list, &ima_keys);
+ mutex_lock(&ns->ima_keys_lock);
+ if (!ns->ima_process_keys) {
+ list_add_tail(&entry->list, &ns->ima_keys);
queued = true;
}
- mutex_unlock(&ima_keys_lock);
+ mutex_unlock(&ns->ima_keys_lock);
if (!queued)
ima_free_key_entry(entry);
@@ -129,7 +118,7 @@ void ima_process_queued_keys(struct ima_namespace *ns)
struct ima_key_entry *entry, *tmp;
bool process = false;
- if (ima_process_keys)
+ if (ns->ima_process_keys)
return;
/*
@@ -138,12 +127,12 @@ void ima_process_queued_keys(struct ima_namespace *ns)
* First one setting the ima_process_keys flag to true will
* process the queued keys.
*/
- mutex_lock(&ima_keys_lock);
- if (!ima_process_keys) {
- ima_process_keys = true;
+ mutex_lock(&ns->ima_keys_lock);
+ if (!ns->ima_process_keys) {
+ ns->ima_process_keys = true;
process = true;
}
- mutex_unlock(&ima_keys_lock);
+ mutex_unlock(&ns->ima_keys_lock);
if (!process)
return;
@@ -151,7 +140,7 @@ void ima_process_queued_keys(struct ima_namespace *ns)
if (!ns->timer_expired)
cancel_delayed_work_sync(&ns->ima_keys_delayed_work);
- list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
+ list_for_each_entry_safe(entry, tmp, &ns->ima_keys, list) {
if (!ns->timer_expired)
process_buffer_measurement(&init_user_ns, NULL,
entry->payload,
@@ -165,7 +154,7 @@ void ima_process_queued_keys(struct ima_namespace *ns)
}
}
-inline bool ima_should_queue_key(void)
+inline bool ima_should_queue_key(struct ima_namespace *ns)
{
- return !ima_process_keys;
+ return !ns->ima_process_keys;
}
--
2.31.1
Define an early teardown function ima_ns_userns_early_teardown() that
will be needed for early teardown of the namespaced SecurityFS used
by an IMA namespace since this holds one additional references to the
user namespace.
This function is not called yet since the refcount_teardown variable at
this point is always 0.
Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/user_namespace.h | 8 ++++++--
security/integrity/ima/ima_ns.c | 6 ++++++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 505e3b3748b6..8f7870b37c73 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -158,14 +158,18 @@ static inline struct user_namespace *get_user_ns(struct user_namespace *ns)
extern int create_user_ns(struct cred *new);
extern int unshare_userns(unsigned long unshare_flags, struct cred **new_cred);
extern void __put_user_ns(struct user_namespace *ns);
+extern void ima_ns_userns_early_teardown(struct ima_namespace *ns);
static inline void put_user_ns(struct user_namespace *ns)
{
if (ns) {
if (refcount_dec_and_test(&ns->ns.count))
__put_user_ns(ns);
- else if (refcount_read(&ns->ns.count) == ns->refcount_teardown)
- ;
+ else if (refcount_read(&ns->ns.count) == ns->refcount_teardown) {
+#ifdef CONFIG_IMA_NS
+ ima_ns_userns_early_teardown(ns->ima_ns);
+#endif
+ }
}
}
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index debe863364fd..4260f96c4eca 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -16,6 +16,7 @@
#include <linux/mount.h>
#include <linux/proc_ns.h>
#include <linux/lsm_hooks.h>
+#include <linux/user_namespace.h>
#include "ima.h"
@@ -64,6 +65,11 @@ struct ima_namespace *copy_ima_ns(struct ima_namespace *old_ns,
return create_ima_ns(user_ns);
}
+void ima_ns_userns_early_teardown(struct ima_namespace *ns)
+{
+}
+EXPORT_SYMBOL(ima_ns_userns_early_teardown);
+
static void destroy_ima_ns(struct ima_namespace *ns)
{
pr_debug("DESTROY ima_ns: 0x%p\n", ns);
--
2.31.1
On 12/2/21 21:31, Stefan Berger wrote:
> extern struct ima_namespace init_ima_ns;
> diff --git a/security/inode.c b/security/inode.c
> index 2738a7b31469..6223f1d838f6 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -22,6 +22,7 @@
> #include <linux/lsm_hooks.h>
> #include <linux/magic.h>
> #include <linux/user_namespace.h>
> +#include <linux/ima.h>
>
> static struct vfsmount *securityfs_mount;
> static int securityfs_mount_count;
> @@ -63,6 +64,13 @@ static const struct fs_context_operations securityfs_context_ops = {
>
> static int securityfs_init_fs_context(struct fs_context *fc)
> {
> + int rc;
> +
> + if (fc->user_ns->ima_ns->late_fs_init) {
> + rc = fc->user_ns->ima_ns->late_fs_init(fc->user_ns);
> + if (rc)
> + return rc;
> + }
> fc->ops = &securityfs_context_ops;
> return 0;
> }
Kernel test robot made me change it to this here:
static int securityfs_init_fs_context(struct fs_context *fc)
{
fc->ops = &securityfs_context_ops;
return ima_ns_late_fs_init(fc->user_ns);
}
With this here when CONFIG_IMA_NS is defined:
static inline int ima_ns_late_fs_init(struct user_namespace *user_ns)
{
struct ima_namespace *ns = user_ns->ima_ns;
if (ns->late_fs_init)
return ns->late_fs_init(ns);
return 0;
}
Stefan
On 12/2/2021 6:31 PM, Stefan Berger wrote:
> From: Denis Semakin <[email protected]>
>
> This patch introduces CAP_INTEGRITY_ADMIN, a new capability that allows
> to setup IMA (Integrity Measurement Architecture) policies per container
> for non-root users.
>
> The main purpose of this new capability is discribed in this document:
> https://kernsec.org/wiki/index.php/IMA_Namespacing_design_considerations
> It is said: "setting the policy should be possibly without the powerful
> CAP_SYS_ADMIN and there should be the opportunity to gate this with a new
> capability CAP_INTEGRITY_ADMIN that allows a user to set the IMA policy
> during container runtime.."
>
> In other words it should be possible to setup IMA policies while not
> giving too many privilges to the user, therefore splitting the
> CAP_INTEGRITY_ADMIN off from CAP_SYS_ADMIN.
Please use CAP_MAC_ADMIN, as discussed on the previous submission.
>
> Signed-off-by: Denis Semakin <[email protected]>
> Signed-off-by: Stefan Berger <[email protected]>
> ---
> include/linux/capability.h | 6 ++++++
> include/uapi/linux/capability.h | 7 ++++++-
> security/selinux/include/classmap.h | 4 ++--
> 3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index 65efb74c3585..ea6d58acb95e 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -278,4 +278,10 @@ int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
> int cap_convert_nscap(struct user_namespace *mnt_userns, struct dentry *dentry,
> const void **ivalue, size_t size);
>
> +static inline bool integrity_admin_ns_capable(struct user_namespace *ns)
> +{
> + return ns_capable(ns, CAP_INTEGRITY_ADMIN) ||
> + ns_capable(ns, CAP_SYS_ADMIN);
> +}
> +
> #endif /* !_LINUX_CAPABILITY_H */
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 463d1ba2232a..48b08e4b3895 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -417,7 +417,12 @@ struct vfs_ns_cap_data {
>
> #define CAP_CHECKPOINT_RESTORE 40
>
> -#define CAP_LAST_CAP CAP_CHECKPOINT_RESTORE
> +/* Allow setup IMA policy per container independently */
> +/* No necessary to be superuser */
> +
> +#define CAP_INTEGRITY_ADMIN 41
> +
> +#define CAP_LAST_CAP CAP_INTEGRITY_ADMIN
>
> #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 35aac62a662e..7ff532b90f09 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -28,9 +28,9 @@
>
> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
> "wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf", \
> - "checkpoint_restore"
> + "checkpoint_restore", "integrity_admin"
>
> -#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
> +#if CAP_LAST_CAP > CAP_INTEGRITY_ADMIN
> #error New capability defined, please update COMMON_CAP2_PERMS.
> #endif
>
On Thu, 2021-12-02 at 21:31 -0500, Stefan Berger wrote:
[...]
> static int securityfs_init_fs_context(struct fs_context *fc)
> {
> + int rc;
> +
> + if (fc->user_ns->ima_ns->late_fs_init) {
> + rc = fc->user_ns->ima_ns->late_fs_init(fc->user_ns);
> + if (rc)
> + return rc;
> + }
> fc->ops = &securityfs_context_ops;
> return 0;
> }
I know I suggested this, but to get this to work in general, it's going
to have to not be specific to IMA, so it's going to have to become
something generic like a notifier chain. The other problem is it's
only working still by accident:
> +int ima_fs_ns_init(struct ima_namespace *ns)
> +{
> + ns->mount = securityfs_ns_create_mount(ns->user_ns);
This actually triggers on the call to securityfs_init_fs_context, but
nothing happens because the callback is null. Every subsequent use of
fscontext will trigger this. The point of a keyed supeblock is that
fill_super is only called once per key, that's the place we should be
doing this. It should also probably be a blocking notifier so any
consumer of securityfs can be namespaced by registering for this
notifier.
> + if (IS_ERR(ns->mount)) {
> + ns->mount = NULL;
> + return -1;
> + }
> + ns->mount_count = 1;
This is a bit nasty, too: we're spilling the guts of mount count
tracking into IMA instead of encapsulating it inside securityfs.
> +
> + /* Adjust the trigger for user namespace's early teardown of
> dependent
> + * namespaces. Due to the filesystem there's an additional
> reference
> + * to the user namespace.
> + */
> + ns->user_ns->refcount_teardown += 1;
> +
> + ns->late_fs_init = ima_fs_ns_late_init;
> +
> + return 0;
> +}
I think what should be happening is that we shouldn't so the
simple_pin_fs, which creates the inodes, ahead of time; we should do it
inside fill_super using a notifier, meaning it gets called once per
key, creates the root dentry then triggers the notifier which
instantiates all the namespaced entries. We can still use
simple_pin_fs for this because there's no locking across fill_super.
This would mean fill_super would be called the first time the
securityfs is mounted inside the namespace.
If we do it this way, we can now make securityfs have its own mount and
mount_count inside the user namespace, which it uses internally to the
securityfs code, thus avoiding exposing them to ima or any other
namespaced consumer.
I also think we now don't need the securityfs_ns_ duplicated functions
because the callback via the notifier chain now ensures we can use the
namespace they were created in to distinguish between non namespaced
and namespaced entries.
So non-namespaced consumers of securityfs would do what they do now
(calling the securityfs_create on initialization) and namespaced
consumers would register a callback on the notifier which would get
called once for every namespace the securityfs gets mounted in.
I also theorize if we do it with notifiers, we could have a notifier on
kill_sb to tear down all the entires. If we do this, I think we don't
have to pin any more.
James
On 12/3/21 11:40, Casey Schaufler wrote:
> On 12/2/2021 6:31 PM, Stefan Berger wrote:
>> From: Denis Semakin <[email protected]>
>>
>> This patch introduces CAP_INTEGRITY_ADMIN, a new capability that allows
>> to setup IMA (Integrity Measurement Architecture) policies per container
>> for non-root users.
>>
>> The main purpose of this new capability is discribed in this document:
>> https://kernsec.org/wiki/index.php/IMA_Namespacing_design_considerations
>> It is said: "setting the policy should be possibly without the powerful
>> CAP_SYS_ADMIN and there should be the opportunity to gate this with a
>> new
>> capability CAP_INTEGRITY_ADMIN that allows a user to set the IMA policy
>> during container runtime.."
>>
>> In other words it should be possible to setup IMA policies while not
>> giving too many privilges to the user, therefore splitting the
>> CAP_INTEGRITY_ADMIN off from CAP_SYS_ADMIN.
>
> Please use CAP_MAC_ADMIN, as discussed on the previous submission.
I wasn't clear on consensus. But sure, let's go with CAP_MAC_ADMIN.
Stefan
On 12/3/21 12:03, James Bottomley wrote:
> On Thu, 2021-12-02 at 21:31 -0500, Stefan Berger wrote:
> [...]
>> static int securityfs_init_fs_context(struct fs_context *fc)
>> {
>> + int rc;
>> +
>> + if (fc->user_ns->ima_ns->late_fs_init) {
>> + rc = fc->user_ns->ima_ns->late_fs_init(fc->user_ns);
>> + if (rc)
>> + return rc;
>> + }
>> fc->ops = &securityfs_context_ops;
>> return 0;
>> }
> I know I suggested this, but to get this to work in general, it's going
> to have to not be specific to IMA, so it's going to have to become
> something generic like a notifier chain. The other problem is it's
> only working still by accident:
I had thought about this also but the rationale was:
securityfs is compiled due to CONFIG_IMA_NS and the user namespace
exists there and that has a pointer now to ima_namespace, which can have
that callback. I assumed that other namespaced subsystems could also be
reached then via such a callback, but I don't know.
I suppose any late filesystem init callchain would have to be connected
to the user_namespace somehow?
>
>> +int ima_fs_ns_init(struct ima_namespace *ns)
>> +{
>> + ns->mount = securityfs_ns_create_mount(ns->user_ns);
> This actually triggers on the call to securityfs_init_fs_context, but
> nothing happens because the callback is null. Every subsequent use of
> fscontext will trigger this. The point of a keyed supeblock is that
> fill_super is only called once per key, that's the place we should be
> doing this. It should also probably be a blocking notifier so any
> consumer of securityfs can be namespaced by registering for this
> notifier.
What I don't like about the fill_super is that it gets called too early:
[ 67.058611] securityfs_ns_create_mount @ 102 target user_ns:
ffff95c010698c80; nr_extents: 0
[ 67.059836] securityfs_fill_super @ 47 user_ns: ffff95c010698c80;
nr_extents: 0
We are switching to the target user namespace in
securityfs_ns_create_mount. The expected nr_extents at this point is 0,
since user_ns hasn't been configured, yet. But then security_fill_super
is also called with nr_extents 0. We cannot use that, it's too early!
>
>> + if (IS_ERR(ns->mount)) {
>> + ns->mount = NULL;
>> + return -1;
>> + }
>> + ns->mount_count = 1;
> This is a bit nasty, too: we're spilling the guts of mount count
> tracking into IMA instead of encapsulating it inside securityfs.
Ok, I can make this disappear.
>
>> +
>> + /* Adjust the trigger for user namespace's early teardown of
>> dependent
>> + * namespaces. Due to the filesystem there's an additional
>> reference
>> + * to the user namespace.
>> + */
>> + ns->user_ns->refcount_teardown += 1;
>> +
>> + ns->late_fs_init = ima_fs_ns_late_init;
>> +
>> + return 0;
>> +}
> I think what should be happening is that we shouldn't so the
> simple_pin_fs, which creates the inodes, ahead of time; we should do it
> inside fill_super using a notifier, meaning it gets called once per
fill_super would only work for the init_user_ns from what I can see.
> key, creates the root dentry then triggers the notifier which
> instantiates all the namespaced entries. We can still use
> simple_pin_fs for this because there's no locking across fill_super.
> This would mean fill_super would be called the first time the
> securityfs is mounted inside the namespace.
I guess I would need to know how fill_super would work or how it could
be called late/delayed as well.
>
> If we do it this way, we can now make securityfs have its own mount and
> mount_count inside the user namespace, which it uses internally to the
> securityfs code, thus avoiding exposing them to ima or any other
> namespaced consumer.
>
> I also think we now don't need the securityfs_ns_ duplicated functions
> because the callback via the notifier chain now ensures we can use the
> namespace they were created in to distinguish between non namespaced
> and namespaced entries.
Is there then no need to pass a separate vfsmount * in anymore? Where
would the vfsmount pointer reside? For now it's in ima_namespace, but it
sounds like it should be in a more centralized place? Should it also be
connected to the user_namespace so we can pick it up using get_user_ns()?
>
> So non-namespaced consumers of securityfs would do what they do now
> (calling the securityfs_create on initialization) and namespaced
> consumers would register a callback on the notifier which would get
> called once for every namespace the securityfs gets mounted in.
>
> I also theorize if we do it with notifiers, we could have a notifier on
> kill_sb to tear down all the entires. If we do this, I think we don't
> have to pin any more.
>
> James
>
>
diff --git a/security/inode.c b/security/inode.c
index ed5f1c533776..49c9839642ed 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -44,6 +44,8 @@ static int securityfs_fill_super(struct super_block
*sb, struct fs_context *fc)
static const struct tree_descr files[] = {{""}};
int error;
+ printk(KERN_INFO "%s @ %u user_ns: %px; nr_extents: %d\n",
__func__, __LINE__, fc->user_ns, fc->user_ns->uid_map.nr_extents);
+
error = simple_fill_super(sb, SECURITYFS_MAGIC, files);
if (error)
return error;
@@ -97,6 +99,8 @@ struct vfsmount *securityfs_ns_create_mount(struct
user_namespace *user_ns)
put_user_ns(fc->user_ns);
fc->user_ns = get_user_ns(user_ns);
+ printk(KERN_INFO "%s @ %u target user_ns: %px; nr_extents:
%d\n", __func__, __LINE__, fc->user_ns, fc->user_ns->uid_map.nr_extents);
+
mnt = fc_mount(fc);
put_fs_context(fc);
return mnt;
On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
> On 12/3/21 12:03, James Bottomley wrote:
> > On Thu, 2021-12-02 at 21:31 -0500, Stefan Berger wrote:
> > [...]
> > > static int securityfs_init_fs_context(struct fs_context *fc)
> > > {
> > > + int rc;
> > > +
> > > + if (fc->user_ns->ima_ns->late_fs_init) {
> > > + rc = fc->user_ns->ima_ns->late_fs_init(fc->user_ns);
> > > + if (rc)
> > > + return rc;
> > > + }
> > > fc->ops = &securityfs_context_ops;
> > > return 0;
> > > }
> > I know I suggested this, but to get this to work in general, it's
> > going to have to not be specific to IMA, so it's going to have to
> > become something generic like a notifier chain. The other problem
> > is it's only working still by accident:
>
> I had thought about this also but the rationale was:
>
> securityfs is compiled due to CONFIG_IMA_NS and the user namespace
> exists there and that has a pointer now to ima_namespace, which can
> have that callback. I assumed that other namespaced subsystems could
> also be reached then via such a callback, but I don't know.
Well securityfs is supposed to exist for LSMs. At some point each of
those is going to need to be namespaced, which may eventually be quite
a pile of callbacks, which is why I thought of a notifier.
> I suppose any late filesystem init callchain would have to be
> connected to the user_namespace somehow?
I don't think so; I think just moving some securityfs entries into the
user_namespace and managing the notifier chain from within securityfs
will do for now. [although I'd have to spec this out in code before I
knew for sure].
> > > +int ima_fs_ns_init(struct ima_namespace *ns)
> > > +{
> > > + ns->mount = securityfs_ns_create_mount(ns->user_ns);
> > This actually triggers on the call to securityfs_init_fs_context,
> > but nothing happens because the callback is null. Every subsequent
> > use of fscontext will trigger this. The point of a keyed supeblock
> > is that fill_super is only called once per key, that's the place we
> > should be doing this. It should also probably be a blocking
> > notifier so anyconsumer of securityfs can be namespaced by
> > registering for this notifier.
>
> What I don't like about the fill_super is that it gets called too
> early:
>
> [ 67.058611] securityfs_ns_create_mount @ 102 target user_ns:
> ffff95c010698c80; nr_extents: 0
> [ 67.059836] securityfs_fill_super @ 47 user_ns:
> ffff95c010698c80;
> nr_extents: 0
Right, it's being activated by securityfs_ns_create_mount which is
called as soon as the user_ns is created.
> We are switching to the target user namespace in
> securityfs_ns_create_mount. The expected nr_extents at this point is
> 0, since user_ns hasn't been configured, yet. But then
> security_fill_super is also called with nr_extents 0. We cannot use
> that, it's too early!
Exactly, so I was thinking of not having a securityfs_ns_create_mount
at all. All the securityfs_ns_create.. calls would be in the notifier
call chain. This means there's nothing to fill the superblock until an
actual mount on it is called.
> > > + if (IS_ERR(ns->mount)) {
> > > + ns->mount = NULL;
> > > + return -1;
> > > + }
> > > + ns->mount_count = 1;
> > This is a bit nasty, too: we're spilling the guts of mount count
> > tracking into IMA instead of encapsulating it inside securityfs.
>
> Ok, I can make this disappear.
>
>
> > > +
> > > + /* Adjust the trigger for user namespace's early teardown of
> > > dependent
> > > + * namespaces. Due to the filesystem there's an additional
> > > reference
> > > + * to the user namespace.
> > > + */
> > > + ns->user_ns->refcount_teardown += 1;
> > > +
> > > + ns->late_fs_init = ima_fs_ns_late_init;
> > > +
> > > + return 0;
> > > +}
> > I think what should be happening is that we shouldn't so the
> > simple_pin_fs, which creates the inodes, ahead of time; we should
> > do it inside fill_super using a notifier, meaning it gets called
> > once per
>
> fill_super would only work for the init_user_ns from what I can see.
>
>
> > key, creates the root dentry then triggers the notifier which
> > instantiates all the namespaced entries. We can still use
> > simple_pin_fs for this because there's no locking across
> > fill_super.
> > This would mean fill_super would be called the first time the
> > securityfs is mounted inside the namespace.
>
> I guess I would need to know how fill_super would work or how it
> could be called late/delayed as well.
So it would be called early in the init_user_ns by non-namespaced
consumers of securityfs, like it is now.
Namespaced consumers wouldn't call any securityfs_ns_create callbacks
to create dentries until they were notified from the fill_super
notifier, which would now only be triggered on first mount of
securityfs inside the namespace.
> > If we do it this way, we can now make securityfs have its own mount
> > and mount_count inside the user namespace, which it uses internally
> > to the securityfs code, thus avoiding exposing them to ima or any
> > other namespaced consumer.
> >
> > I also think we now don't need the securityfs_ns_ duplicated
> > functions because the callback via the notifier chain now ensures
> > we can usethe namespace they were created in to distinguish between
> > non namespaced and namespaced entries.
>
> Is there then no need to pass a separate vfsmount * in anymore?
I don't think so no. It could be entirely managed internally to
securityfs.
> Where would the vfsmount pointer reside? For now it's in
> ima_namespace, but it sounds like it should be in a more centralized
> place? Should it also be connected to the user_namespace so we can
> pick it up using get_user_ns()?
exactly. I think struct user_namespace should have two elements gated
by a #ifdef CONFIG_SECURITYFS which are the vfsmount and the
mount_count for passing into simple_pin_fs.
James
On 12/3/21 13:50, James Bottomley wrote:
> On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
>> On 12/3/21 12:03, James Bottomley wrote:
>>> On Thu, 2021-12-02 at 21:31 -0500, Stefan Berger wrote:
>>> [...]
>>>> static int securityfs_init_fs_context(struct fs_context *fc)
>>>> {
>>>> + int rc;
>>>> +
>>>> + if (fc->user_ns->ima_ns->late_fs_init) {
>>>> + rc = fc->user_ns->ima_ns->late_fs_init(fc->user_ns);
>>>> + if (rc)
>>>> + return rc;
>>>> + }
>>>> fc->ops = &securityfs_context_ops;
>>>> return 0;
>>>> }
>>> I know I suggested this, but to get this to work in general, it's
>>> going to have to not be specific to IMA, so it's going to have to
>>> become something generic like a notifier chain. The other problem
>>> is it's only working still by accident:
>> I had thought about this also but the rationale was:
>>
>> securityfs is compiled due to CONFIG_IMA_NS and the user namespace
>> exists there and that has a pointer now to ima_namespace, which can
>> have that callback. I assumed that other namespaced subsystems could
>> also be reached then via such a callback, but I don't know.
> Well securityfs is supposed to exist for LSMs. At some point each of
> those is going to need to be namespaced, which may eventually be quite
> a pile of callbacks, which is why I thought of a notifier.
>
>> I suppose any late filesystem init callchain would have to be
>> connected to the user_namespace somehow?
> I don't think so; I think just moving some securityfs entries into the
> user_namespace and managing the notifier chain from within securityfs
> will do for now. [although I'd have to spec this out in code before I
> knew for sure].
It doesn't have to be right in the user_namespace. The IMA namespace is
connected to the user namespace and holds the dentries now...
Please spec it out...
>
>>>> +int ima_fs_ns_init(struct ima_namespace *ns)
>>>> +{
>>>> + ns->mount = securityfs_ns_create_mount(ns->user_ns);
>>> This actually triggers on the call to securityfs_init_fs_context,
>>> but nothing happens because the callback is null. Every subsequent
>>> use of fscontext will trigger this. The point of a keyed supeblock
>>> is that fill_super is only called once per key, that's the place we
>>> should be doing this. It should also probably be a blocking
>>> notifier so anyconsumer of securityfs can be namespaced by
>>> registering for this notifier.
>> What I don't like about the fill_super is that it gets called too
>> early:
>>
>> [ 67.058611] securityfs_ns_create_mount @ 102 target user_ns:
>> ffff95c010698c80; nr_extents: 0
>> [ 67.059836] securityfs_fill_super @ 47 user_ns:
>> ffff95c010698c80;
>> nr_extents: 0
> Right, it's being activated by securityfs_ns_create_mount which is
> called as soon as the user_ns is created.
Well, that doesn't help us then...
>> We are switching to the target user namespace in
>> securityfs_ns_create_mount. The expected nr_extents at this point is
>> 0, since user_ns hasn't been configured, yet. But then
>> security_fill_super is also called with nr_extents 0. We cannot use
>> that, it's too early!
> Exactly, so I was thinking of not having a securityfs_ns_create_mount
> at all. All the securityfs_ns_create.. calls would be in the notifier
But we need to somehow have a call to get_tree_keyed() and have that
user namespace switched out. I don't know how else to do this other than
having some function that does that and that is now called
securityfs_ns_create_mount().
get_tree_keyed() will also call the fill_super() which is called when
securityfs_ns_create_mount() is called.
[ 196.739071] ima_fs_ns_init @ 639 before securityfs_ns_create_mount()
[ 196.740426] securityfs_init_fs_context @ 72 user_ns:
ffffffff98a3cc60; nr_extents: 1
[ 196.741519] securityfs_ns_create_mount @ 105 target user_ns:
ffff9e239753eb80; nr_extents: 0
[ 196.742657] securityfs_get_tree @ 60 before get_tree_keyed()
[ 196.743418] securityfs_fill_super @ 47 user_ns: ffff9e239753eb80;
nr_extents: 0
[ 196.744467] ima_fs_ns_init @ 641 after securityfs_ns_create_mount()
[ 196.745304] ima: Allocated hash algorithm: sha256
[ 196.757650] securityfs_init_fs_context @ 72 user_ns:
ffff9e239753eb80; nr_extents: 1
[ 196.758759] securityfs_get_tree @ 60 before get_tree_keyed()
You said it works by 'accident'. I know it works because the function
securityfs_init_fs_context() that now populates the filesystem via the
late_fs_init() is getting called twice. Does 'accident' here mean the
call sequence could change?
>
>> Where would the vfsmount pointer reside? For now it's in
>> ima_namespace, but it sounds like it should be in a more centralized
>> place? Should it also be connected to the user_namespace so we can
>> pick it up using get_user_ns()?
> exactly. I think struct user_namespace should have two elements gated
> by a #ifdef CONFIG_SECURITYFS which are the vfsmount and the
> mount_count for passing into simple_pin_fs.
Also that we can do for as long as it flies beyond the conversation
here... :-) Anyone else have an opinion ?
Stefan
>
> James
>
>
On 12/3/2021 10:50 AM, James Bottomley wrote:
> On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
>> On 12/3/21 12:03, James Bottomley wrote:
>>> On Thu, 2021-12-02 at 21:31 -0500, Stefan Berger wrote:
>>> [...]
>>>> static int securityfs_init_fs_context(struct fs_context *fc)
>>>> {
>>>> + int rc;
>>>> +
>>>> + if (fc->user_ns->ima_ns->late_fs_init) {
>>>> + rc = fc->user_ns->ima_ns->late_fs_init(fc->user_ns);
>>>> + if (rc)
>>>> + return rc;
>>>> + }
>>>> fc->ops = &securityfs_context_ops;
>>>> return 0;
>>>> }
>>> I know I suggested this, but to get this to work in general, it's
>>> going to have to not be specific to IMA, so it's going to have to
>>> become something generic like a notifier chain. The other problem
>>> is it's only working still by accident:
>> I had thought about this also but the rationale was:
>>
>> securityfs is compiled due to CONFIG_IMA_NS and the user namespace
>> exists there and that has a pointer now to ima_namespace, which can
>> have that callback. I assumed that other namespaced subsystems could
>> also be reached then via such a callback, but I don't know.
> Well securityfs is supposed to exist for LSMs. At some point each of
> those is going to need to be namespaced, which may eventually be quite
> a pile of callbacks, which is why I thought of a notifier.
While AppArmor, lockdown and the integrity family use securityfs,
SELinux and Smack do not. They have their own independent filesystems.
Implementations of namespacing for each of SELinux and Smack have been
proposed, but nothing has been adopted. It would be really handy to
namespace the infrastructure rather than each individual LSM, but I
fear that's a bigger project than anyone will be taking on any time
soon. It's likely to encounter many of the same issues that I've been
dealing with for module stacking.
>
>> I suppose any late filesystem init callchain would have to be
>> connected to the user_namespace somehow?
> I don't think so; I think just moving some securityfs entries into the
> user_namespace and managing the notifier chain from within securityfs
> will do for now. [although I'd have to spec this out in code before I
> knew for sure].
>
>>>> +int ima_fs_ns_init(struct ima_namespace *ns)
>>>> +{
>>>> + ns->mount = securityfs_ns_create_mount(ns->user_ns);
>>> This actually triggers on the call to securityfs_init_fs_context,
>>> but nothing happens because the callback is null. Every subsequent
>>> use of fscontext will trigger this. The point of a keyed supeblock
>>> is that fill_super is only called once per key, that's the place we
>>> should be doing this. It should also probably be a blocking
>>> notifier so anyconsumer of securityfs can be namespaced by
>>> registering for this notifier.
>> What I don't like about the fill_super is that it gets called too
>> early:
>>
>> [ 67.058611] securityfs_ns_create_mount @ 102 target user_ns:
>> ffff95c010698c80; nr_extents: 0
>> [ 67.059836] securityfs_fill_super @ 47 user_ns:
>> ffff95c010698c80;
>> nr_extents: 0
> Right, it's being activated by securityfs_ns_create_mount which is
> called as soon as the user_ns is created.
>
>> We are switching to the target user namespace in
>> securityfs_ns_create_mount. The expected nr_extents at this point is
>> 0, since user_ns hasn't been configured, yet. But then
>> security_fill_super is also called with nr_extents 0. We cannot use
>> that, it's too early!
> Exactly, so I was thinking of not having a securityfs_ns_create_mount
> at all. All the securityfs_ns_create.. calls would be in the notifier
> call chain. This means there's nothing to fill the superblock until an
> actual mount on it is called.
>
>>>> + if (IS_ERR(ns->mount)) {
>>>> + ns->mount = NULL;
>>>> + return -1;
>>>> + }
>>>> + ns->mount_count = 1;
>>> This is a bit nasty, too: we're spilling the guts of mount count
>>> tracking into IMA instead of encapsulating it inside securityfs.
>> Ok, I can make this disappear.
>>
>>
>>>> +
>>>> + /* Adjust the trigger for user namespace's early teardown of
>>>> dependent
>>>> + * namespaces. Due to the filesystem there's an additional
>>>> reference
>>>> + * to the user namespace.
>>>> + */
>>>> + ns->user_ns->refcount_teardown += 1;
>>>> +
>>>> + ns->late_fs_init = ima_fs_ns_late_init;
>>>> +
>>>> + return 0;
>>>> +}
>>> I think what should be happening is that we shouldn't so the
>>> simple_pin_fs, which creates the inodes, ahead of time; we should
>>> do it inside fill_super using a notifier, meaning it gets called
>>> once per
>> fill_super would only work for the init_user_ns from what I can see.
>>
>>
>>> key, creates the root dentry then triggers the notifier which
>>> instantiates all the namespaced entries. We can still use
>>> simple_pin_fs for this because there's no locking across
>>> fill_super.
>>> This would mean fill_super would be called the first time the
>>> securityfs is mounted inside the namespace.
>> I guess I would need to know how fill_super would work or how it
>> could be called late/delayed as well.
> So it would be called early in the init_user_ns by non-namespaced
> consumers of securityfs, like it is now.
>
> Namespaced consumers wouldn't call any securityfs_ns_create callbacks
> to create dentries until they were notified from the fill_super
> notifier, which would now only be triggered on first mount of
> securityfs inside the namespace.
>
>>> If we do it this way, we can now make securityfs have its own mount
>>> and mount_count inside the user namespace, which it uses internally
>>> to the securityfs code, thus avoiding exposing them to ima or any
>>> other namespaced consumer.
>>>
>>> I also think we now don't need the securityfs_ns_ duplicated
>>> functions because the callback via the notifier chain now ensures
>>> we can usethe namespace they were created in to distinguish between
>>> non namespaced and namespaced entries.
>> Is there then no need to pass a separate vfsmount * in anymore?
> I don't think so no. It could be entirely managed internally to
> securityfs.
>
>> Where would the vfsmount pointer reside? For now it's in
>> ima_namespace, but it sounds like it should be in a more centralized
>> place? Should it also be connected to the user_namespace so we can
>> pick it up using get_user_ns()?
> exactly. I think struct user_namespace should have two elements gated
> by a #ifdef CONFIG_SECURITYFS which are the vfsmount and the
> mount_count for passing into simple_pin_fs.
>
>
> James
>
>
On 12/3/21 14:11, Stefan Berger wrote:
>
> On 12/3/21 13:50, James Bottomley wrote:
>
>
>>
>>> Where would the vfsmount pointer reside? For now it's in
>>> ima_namespace, but it sounds like it should be in a more centralized
>>> place? Should it also be connected to the user_namespace so we can
>>> pick it up using get_user_ns()?
>> exactly. I think struct user_namespace should have two elements gated
>> by a #ifdef CONFIG_SECURITYFS which are the vfsmount and the
>> mount_count for passing into simple_pin_fs.
>
> Also that we can do for as long as it flies beyond the conversation
> here... :-) Anyone else have an opinion ?
I moved it now and this greatly reduced the amount of changes. The
dentries are now all in the ima_namespace and it works with one API. Thanks!
I wonder whether to move the integrity dir also into the ima_namespace.
It's generated in integrity/iint.c, so not in the IMA territory... For
the IMA namespacing case I need to create it as well, though.
https://elixir.bootlin.com/linux/latest/source/security/integrity/iint.c#L218
Stefan
On Fri, 2021-12-03 at 14:11 -0500, Stefan Berger wrote:
> On 12/3/21 13:50, James Bottomley wrote:
> > On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
[...]
> > > I suppose any late filesystem init callchain would have to be
> > > connected to the user_namespace somehow?
> >
> > I don't think so; I think just moving some securityfs entries into
> > the user_namespace and managing the notifier chain from within
> > securityfs will do for now. [although I'd have to spec this out in
> > code before I knew for sure].
>
> It doesn't have to be right in the user_namespace. The IMA namespace
> is connected to the user namespace and holds the dentries now...
>
> Please spec it out...
OK, this is what I have. fill_super turned out to be a locking
nightmare, so I triggered it from free context instead (which doesn't
have the once per keyed superblock property, so I added a flag in the
user namespace). I've got it to the point where the event is triggered
on mount and unmount, so all the entries for the namespace are added
when the filesystem is mounted and remove when it's unmounted. This
style of addition no longer needs the simple_pin_fs, because the
add/remove callbacks substitute (plus, if we pinned, the free_super
wouldn't trigger on unmount). The default behaviour still does pinning
and unpinning, but that can be keyed off the current user_namespace.
This is all on top of your current series ... some of the functions
should probably be renamed, but I kept them to show how the code was
migrating in this sketch.
James
---
From 59c45daa8698c66c3bcebfb194123977d548a9a6 Mon Sep 17 00:00:00 2001
From: James Bottomley <[email protected]>
Date: Sat, 4 Dec 2021 16:38:37 +0000
Subject: [PATCH] rework securityfs
---
include/linux/security.h | 28 +--
include/linux/user_namespace.h | 21 +-
security/inode.c | 292 ++++++++---------------
security/integrity/ima/ima.h | 3 +-
security/integrity/ima/ima_fs.c | 174 +++++---------
security/integrity/ima/ima_init_ima_ns.c | 2 -
security/integrity/ima/ima_ns.c | 7 -
7 files changed, 166 insertions(+), 361 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 83b3af3c2959..2f37651da6e5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -29,6 +29,7 @@
#include <linux/fs.h>
#include <linux/slab.h>
#include <linux/err.h>
+#include <linux/notifier.h>
#include <linux/string.h>
#include <linux/mm.h>
@@ -1919,6 +1920,13 @@ static inline void security_audit_rule_free(void *lsmrule)
#ifdef CONFIG_SECURITYFS
+enum {
+ SECURITYFS_NS_ADD,
+ SECURITYFS_NS_REMOVE,
+};
+
+extern int securityfs_register_ns_notifier(struct notifier_block *nb);
+extern int securityfs_unregister_ns_notifier(struct notifier_block *nb);
extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
struct dentry *parent, void *data,
const struct file_operations *fops);
@@ -1929,20 +1937,6 @@ struct dentry *securityfs_create_symlink(const char *name,
const struct inode_operations *iops);
extern void securityfs_remove(struct dentry *dentry);
-extern struct dentry *securityfs_ns_create_file(const char *name, umode_t mode,
- struct dentry *parent, void *data,
- const struct file_operations *fops,
- struct vfsmount **mount, int *mount_count);
-extern struct dentry *securityfs_ns_create_dir(const char *name, struct dentry *parent,
- struct vfsmount **mount, int *mount_count);
-struct dentry *securityfs_ns_create_symlink(const char *name,
- struct dentry *parent,
- const char *target,
- const struct inode_operations *iops,
- struct vfsmount **mount, int *mount_count);
-extern void securityfs_ns_remove(struct dentry *dentry,
- struct vfsmount **mount, int *mount_count);
-struct vfsmount *securityfs_ns_create_mount(struct user_namespace *user_ns);
#else /* CONFIG_SECURITYFS */
@@ -1962,9 +1956,9 @@ static inline struct dentry *securityfs_create_file(const char *name,
}
static inline struct dentry *securityfs_create_symlink(const char *name,
- struct dentry *parent,
- const char *target,
- const struct inode_operations *iops)
+ struct dentry *parent,
+ const char *target,
+ const struct inode_operations *iops)
{
return ERR_PTR(-ENODEV);
}
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 8f7870b37c73..6b8bd060d8c4 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -103,11 +103,10 @@ struct user_namespace {
#ifdef CONFIG_IMA
struct ima_namespace *ima_ns;
#endif
- /* The refcount at which to start tearing down dependent namespaces
- * (currently only IMA) that may hold additional references to the
- * user namespace.
- */
- unsigned int refcount_teardown;
+#ifdef CONFIG_SECURITYFS
+ struct vfsmount *securityfs_mount;
+ bool securityfs_notifier_sent;
+#endif
} __randomize_layout;
struct ucounts {
@@ -158,19 +157,11 @@ static inline struct user_namespace *get_user_ns(struct user_namespace *ns)
extern int create_user_ns(struct cred *new);
extern int unshare_userns(unsigned long unshare_flags, struct cred **new_cred);
extern void __put_user_ns(struct user_namespace *ns);
-extern void ima_ns_userns_early_teardown(struct ima_namespace *ns);
static inline void put_user_ns(struct user_namespace *ns)
{
- if (ns) {
- if (refcount_dec_and_test(&ns->ns.count))
- __put_user_ns(ns);
- else if (refcount_read(&ns->ns.count) == ns->refcount_teardown) {
-#ifdef CONFIG_IMA_NS
- ima_ns_userns_early_teardown(ns->ima_ns);
-#endif
- }
- }
+ if (ns && refcount_dec_and_test(&ns->ns.count))
+ __put_user_ns(ns);
}
struct seq_operations;
diff --git a/security/inode.c b/security/inode.c
index 6223f1d838f6..62ab4630dc31 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -18,15 +18,17 @@
#include <linux/pagemap.h>
#include <linux/init.h>
#include <linux/namei.h>
+#include <linux/notifier.h>
#include <linux/security.h>
#include <linux/lsm_hooks.h>
#include <linux/magic.h>
#include <linux/user_namespace.h>
#include <linux/ima.h>
-static struct vfsmount *securityfs_mount;
static int securityfs_mount_count;
+static BLOCKING_NOTIFIER_HEAD(securityfs_ns_notifier);
+
static void securityfs_free_inode(struct inode *inode)
{
if (S_ISLNK(inode->i_mode))
@@ -39,6 +41,31 @@ static const struct super_operations securityfs_super_operations = {
.free_inode = securityfs_free_inode,
};
+static struct file_system_type fs_type;
+
+static void securityfs_free_context(struct fs_context *fc)
+{
+ struct user_namespace *ns = fc->user_ns;
+ if (ns == &init_user_ns ||
+ ns->securityfs_notifier_sent)
+ return;
+
+ ns->securityfs_notifier_sent = true;
+
+ ns->securityfs_mount = vfs_kern_mount(&fs_type, SB_KERNMOUNT,
+ fs_type.name, NULL);
+ if (IS_ERR(ns->securityfs_mount)) {
+ printk(KERN_ERR "kern mount on securityfs ERROR: %ld\n",
+ PTR_ERR(ns->securityfs_mount));
+ ns->securityfs_mount = NULL;
+ return;
+ }
+
+ blocking_notifier_call_chain(&securityfs_ns_notifier,
+ SECURITYFS_NS_ADD, fc->user_ns);
+ mntput(ns->securityfs_mount);
+}
+
static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc)
{
static const struct tree_descr files[] = {{""}};
@@ -60,52 +87,44 @@ static int securityfs_get_tree(struct fs_context *fc)
static const struct fs_context_operations securityfs_context_ops = {
.get_tree = securityfs_get_tree,
+ .free = securityfs_free_context,
};
static int securityfs_init_fs_context(struct fs_context *fc)
{
- int rc;
-
- if (fc->user_ns->ima_ns->late_fs_init) {
- rc = fc->user_ns->ima_ns->late_fs_init(fc->user_ns);
- if (rc)
- return rc;
- }
fc->ops = &securityfs_context_ops;
return 0;
}
+static void securityfs_kill_super(struct super_block *sb)
+{
+ struct user_namespace *ns = sb->s_fs_info;
+
+ if (ns != &init_user_ns)
+ blocking_notifier_call_chain(&securityfs_ns_notifier,
+ SECURITYFS_NS_REMOVE,
+ sb->s_fs_info);
+ ns->securityfs_notifier_sent = false;
+ ns->securityfs_mount = NULL;
+ kill_litter_super(sb);
+}
+
static struct file_system_type fs_type = {
.owner = THIS_MODULE,
.name = "securityfs",
.init_fs_context = securityfs_init_fs_context,
- .kill_sb = kill_litter_super,
+ .kill_sb = securityfs_kill_super,
.fs_flags = FS_USERNS_MOUNT,
};
-/**
- * securityfs_ns_create_mount - create instance of securityfs in given user namespace
- *
- * @user_ns: the user namespace to create the vfsmount in
- *
- * This function returns a pointer to the vfsmount or an error code. The vfsmount
- * has to be used when creating or removing filesystem dentries.
- */
-struct vfsmount *securityfs_ns_create_mount(struct user_namespace *user_ns)
+int securityfs_register_ns_notifier(struct notifier_block *nb)
{
- struct fs_context *fc;
- struct vfsmount *mnt;
-
- fc = fs_context_for_mount(&fs_type, SB_KERNMOUNT);
- if (IS_ERR(fc))
- return ERR_CAST(fc);
-
- put_user_ns(fc->user_ns);
- fc->user_ns = get_user_ns(user_ns);
+ return blocking_notifier_chain_register(&securityfs_ns_notifier, nb);
+}
- mnt = fc_mount(fc);
- put_fs_context(fc);
- return mnt;
+int securityfs_unregister_ns_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(&securityfs_ns_notifier, nb);
}
/**
@@ -147,24 +166,27 @@ struct vfsmount *securityfs_ns_create_mount(struct user_namespace *user_ns)
static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
struct dentry *parent, void *data,
const struct file_operations *fops,
- const struct inode_operations *iops,
- struct vfsmount **mount, int *mount_count)
+ const struct inode_operations *iops)
{
struct dentry *dentry;
struct inode *dir, *inode;
int error;
+ struct user_namespace *ns = current_user_ns();
if (!(mode & S_IFMT))
mode = (mode & S_IALLUGO) | S_IFREG;
- pr_debug("securityfs: creating file '%s'\n",name);
+ pr_debug("securityfs: creating file '%s', ns=%u\n",name, ns->ns.inum);
- error = simple_pin_fs(&fs_type, mount, mount_count);
- if (error)
- return ERR_PTR(error);
+ if (ns == &init_user_ns) {
+ error = simple_pin_fs(&fs_type, &ns->securityfs_mount,
+ &securityfs_mount_count);
+ if (error)
+ return ERR_PTR(error);
+ }
if (!parent)
- parent = (*mount)->mnt_root;
+ parent = ns->securityfs_mount->mnt_root;
dir = d_inode(parent);
@@ -209,7 +231,9 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
dentry = ERR_PTR(error);
out:
inode_unlock(dir);
- simple_release_fs(mount, mount_count);
+ if (ns == &init_user_ns)
+ simple_release_fs(&ns->securityfs_mount,
+ &securityfs_mount_count);
return dentry;
}
@@ -242,46 +266,10 @@ struct dentry *securityfs_create_file(const char *name, umode_t mode,
struct dentry *parent, void *data,
const struct file_operations *fops)
{
- return securityfs_create_dentry(name, mode, parent, data, fops, NULL,
- &securityfs_mount,
- &securityfs_mount_count);
+ return securityfs_create_dentry(name, mode, parent, data, fops, NULL);
}
EXPORT_SYMBOL_GPL(securityfs_create_file);
-/**
- * securityfs_ns_create_file - create a file in the securityfs_ns filesystem
- *
- * @name: a pointer to a string containing the name of the file to create.
- * @mode: the permission that the file should have
- * @parent: a pointer to the parent dentry for this file. This should be a
- * directory dentry if set. If this parameter is %NULL, then the
- * file will be created in the root of the securityfs_ns filesystem.
- * @data: a pointer to something that the caller will want to get to later
- * on. The inode.i_private pointer will point to this value on
- * the open() call.
- * @fops: a pointer to a struct file_operations that should be used for
- * this file.
- * @mount: Pointer to a pointer of a an existing vfsmount
- * @mount_count: The mount_count that goes along with the @mount
- *
- * This function creates a file in securityfs_ns with the given @name.
- *
- * This function returns a pointer to a dentry if it succeeds. This
- * pointer must be passed to the securityfs_ns_remove() function when the file
- * is to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here). If an error occurs, the function will return
- * the error value (via ERR_PTR).
- */
-struct dentry *securityfs_ns_create_file(const char *name, umode_t mode,
- struct dentry *parent, void *data,
- const struct file_operations *fops,
- struct vfsmount **mount, int *mount_count)
-{
- return securityfs_create_dentry(name, mode, parent, data, fops, NULL,
- mount, mount_count);
-}
-EXPORT_SYMBOL_GPL(securityfs_ns_create_file);
-
/**
* securityfs_create_dir - create a directory in the securityfs filesystem
*
@@ -308,55 +296,6 @@ struct dentry *securityfs_create_dir(const char *name, struct dentry *parent)
}
EXPORT_SYMBOL_GPL(securityfs_create_dir);
-/**
- * securityfs_ns_create_dir - create a directory in the securityfs_ns filesystem
- *
- * @name: a pointer to a string containing the name of the directory to
- * create.
- * @parent: a pointer to the parent dentry for this file. This should be a
- * directory dentry if set. If this parameter is %NULL, then the
- * directory will be created in the root of the securityfs_ns filesystem.
- * @mount: Pointer to a pointer of a an existing vfsmount
- * @mount_count: The mount_count that goes along with the @mount
- *
- * This function creates a directory in securityfs_ns with the given @name.
- *
- * This function returns a pointer to a dentry if it succeeds. This
- * pointer must be passed to the securityfs_ns_remove() function when the file
- * is to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here). If an error occurs, the function will return
- * the error value (via ERR_PTR).
- */
-struct dentry *securityfs_ns_create_dir(const char *name, struct dentry *parent,
- struct vfsmount **mount, int *mount_count)
-{
- return securityfs_ns_create_file(name, S_IFDIR | 0755, parent, NULL, NULL,
- mount, mount_count);
-}
-EXPORT_SYMBOL_GPL(securityfs_ns_create_dir);
-
-static struct dentry *_securityfs_create_symlink(const char *name,
- struct dentry *parent,
- const char *target,
- const struct inode_operations *iops,
- struct vfsmount **mount, int *mount_count)
-{
- struct dentry *dent;
- char *link = NULL;
-
- if (target) {
- link = kstrdup(target, GFP_KERNEL);
- if (!link)
- return ERR_PTR(-ENOMEM);
- }
- dent = securityfs_create_dentry(name, S_IFLNK | 0444, parent,
- link, NULL, iops, mount, mount_count);
- if (IS_ERR(dent))
- kfree(link);
-
- return dent;
-}
-
/**
* securityfs_create_symlink - create a symlink in the securityfs filesystem
*
@@ -388,48 +327,40 @@ struct dentry *securityfs_create_symlink(const char *name,
const char *target,
const struct inode_operations *iops)
{
- return _securityfs_create_symlink(name, parent, target, iops,
- &securityfs_mount, &securityfs_mount_count);
+ struct dentry *dent;
+ char *link = NULL;
+
+ if (target) {
+ link = kstrdup(target, GFP_KERNEL);
+ if (!link)
+ return ERR_PTR(-ENOMEM);
+ }
+ dent = securityfs_create_dentry(name, S_IFLNK | 0444, parent,
+ link, NULL, iops);
+ if (IS_ERR(dent))
+ kfree(link);
+
+ return dent;
}
-EXPORT_SYMBOL_GPL(securityfs_create_symlink);
+EXPORT_SYMBOL(securityfs_create_symlink);
/**
- * securityfs_ns_create_symlink - create a symlink in the securityfs_ns filesystem
+ * securityfs_remove - removes a file or directory from the securityfs filesystem
*
- * @name: a pointer to a string containing the name of the symlink to
- * create.
- * @parent: a pointer to the parent dentry for the symlink. This should be a
- * directory dentry if set. If this parameter is %NULL, then the
- * directory will be created in the root of the securityfs_ns filesystem.
- * @target: a pointer to a string containing the name of the symlink's target.
- * If this parameter is %NULL, then the @iops parameter needs to be
- * setup to handle .readlink and .get_link inode_operations.
- * @mount: Pointer to a pointer of a an existing vfsmount
- * @mount_count: The mount_count that goes along with the @mount
+ * @dentry: a pointer to a the dentry of the file or directory to be removed.
*
- * This function creates a symlink in securityfs_ns with the given @name.
+ * This function removes a file or directory in securityfs that was previously
+ * created with a call to another securityfs function (like
+ * securityfs_create_file() or variants thereof.)
*
- * This function returns a pointer to a dentry if it succeeds. This
- * pointer must be passed to the securityfs_ns_remove() function when the file
- * is to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here). If an error occurs, the function will return
- * the error value (via ERR_PTR).
+ * This function is required to be called in order for the file to be
+ * removed. No automatic cleanup of files will happen when a module is
+ * removed; you are responsible here.
*/
-struct dentry *securityfs_ns_create_symlink(const char *name,
- struct dentry *parent,
- const char *target,
- const struct inode_operations *iops,
- struct vfsmount **mount, int *mount_count)
-{
- return _securityfs_create_symlink(name, parent, target, iops,
- mount, mount_count);
-}
-EXPORT_SYMBOL_GPL(securityfs_ns_create_symlink);
-
-static void _securityfs_remove(struct dentry *dentry,
- struct vfsmount **mount, int *mount_count)
+void securityfs_remove(struct dentry *dentry)
{
struct inode *dir;
+ struct user_namespace *ns = current_user_ns();
if (!dentry || IS_ERR(dentry))
return;
@@ -444,49 +375,12 @@ static void _securityfs_remove(struct dentry *dentry,
dput(dentry);
}
inode_unlock(dir);
- simple_release_fs(mount, mount_count);
+ if (ns == &init_user_ns)
+ simple_release_fs(&ns->securityfs_mount,
+ &securityfs_mount_count);
}
+EXPORT_SYMBOL(securityfs_remove);
-/**
- * securityfs_remove - removes a file or directory from the securityfs filesystem
- *
- * @dentry: a pointer to a the dentry of the file or directory to be removed.
- *
- * This function removes a file or directory in securityfs that was previously
- * created with a call to another securityfs function (like
- * securityfs_create_file() or variants thereof.)
- *
- * This function is required to be called in order for the file to be
- * removed. No automatic cleanup of files will happen when a module is
- * removed; you are responsible here.
- */
-void securityfs_remove(struct dentry *dentry)
-{
- _securityfs_remove(dentry, &securityfs_mount, &securityfs_mount_count);
-}
-
-EXPORT_SYMBOL_GPL(securityfs_remove);
-
-/**
- * securityfs_ns_remove - removes a file or directory from the securityfs_ns filesystem
- *
- * @dentry: a pointer to a the dentry of the file or directory to be removed.
- * @mount: Pointer to a pointer of a an existing vfsmount
- * @mount_count: The mount_count that goes along with the @mount
- *
- * This function removes a file or directory in securityfs_ns that was previously
- * created with a call to another securityfs_ns function (like
- * securityfs_ns_create_file() or variants thereof.)
- *
- * This function is required to be called in order for the file to be
- * removed. No automatic cleanup of files will happen when a module is
- * removed; you are responsible here.
- */
-void securityfs_ns_remove(struct dentry *dentry, struct vfsmount **mount, int *mount_count)
-{
- _securityfs_remove(dentry, mount, mount_count);
-}
-EXPORT_SYMBOL_GPL(securityfs_ns_remove);
#ifdef CONFIG_SECURITY
static struct dentry *lsm_dentry;
@@ -511,6 +405,8 @@ static int __init securityfs_init(void)
if (retval)
return retval;
+ init_user_ns.securityfs_mount = NULL;
+
retval = register_filesystem(&fs_type);
if (retval) {
sysfs_remove_mount_point(kernel_kobj, "security");
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 9bcd71bb716c..12b7df65a5ff 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -139,8 +139,7 @@ struct ns_status {
/* Internal IMA function definitions */
int ima_init(void);
int ima_fs_init(void);
-int ima_fs_ns_init(struct ima_namespace *ns);
-void ima_fs_ns_free(struct ima_namespace *ns);
+void ima_fs_ns_free(void);
int ima_add_template_entry(struct ima_namespace *ns,
struct ima_template_entry *entry, int violation,
const char *op, struct inode *inode,
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 65b2af7c14dd..26f26e8756a8 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -26,6 +26,8 @@
#include "ima.h"
+int ima_fs_ns_init(void);
+
bool ima_canonical_fmt;
static int __init default_canonical_fmt_setup(char *str)
{
@@ -360,14 +362,6 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
return result;
}
-static struct dentry *ima_dir;
-static struct dentry *ima_symlink;
-static struct dentry *binary_runtime_measurements;
-static struct dentry *ascii_runtime_measurements;
-static struct dentry *runtime_measurements_count;
-static struct dentry *violations;
-static struct dentry *ima_policy;
-
enum ima_fs_flags {
IMA_FS_BUSY,
};
@@ -437,14 +431,8 @@ static int ima_release_policy(struct inode *inode, struct file *file)
ima_update_policy(ns);
#if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
- if (ns == &init_ima_ns) {
- securityfs_remove(ima_policy);
- ima_policy = NULL;
- } else {
- securityfs_ns_remove(ns->dentry[IMAFS_DENTRY_POLICY],
- &ns->mount, &ns->mount_count);
- ns->dentry[IMAFS_DENTRY_POLICY] = NULL;
- }
+ securityfs_remove(ns->dentry[IMAFS_DENTRY_POLICY]);
+ ns->dentry[IMAFS_DENTRY_POLICY] = NULL;
#elif defined(CONFIG_IMA_WRITE_POLICY)
clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
#elif defined(CONFIG_IMA_READ_POLICY)
@@ -461,60 +449,32 @@ static const struct file_operations ima_measure_policy_ops = {
.llseek = generic_file_llseek,
};
-int __init ima_fs_init(void)
+static int ima_fs_ns_late_init(struct user_namespace *user_ns);
+static void ima_fs_ns_free_dentries(struct ima_namespace *ns);
+static int ima_ns_notify(struct notifier_block *this, unsigned long msg,
+ void *data)
{
- ima_dir = securityfs_create_dir("ima", integrity_dir);
- if (IS_ERR(ima_dir))
- return -1;
-
- ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima",
- NULL);
- if (IS_ERR(ima_symlink))
- goto out;
-
- binary_runtime_measurements =
- securityfs_create_file("binary_runtime_measurements",
- S_IRUSR | S_IRGRP, ima_dir, NULL,
- &ima_measurements_ops);
- if (IS_ERR(binary_runtime_measurements))
- goto out;
-
- ascii_runtime_measurements =
- securityfs_create_file("ascii_runtime_measurements",
- S_IRUSR | S_IRGRP, ima_dir, NULL,
- &ima_ascii_measurements_ops);
- if (IS_ERR(ascii_runtime_measurements))
- goto out;
-
- runtime_measurements_count =
- securityfs_create_file("runtime_measurements_count",
- S_IRUSR | S_IRGRP, ima_dir, NULL,
- &ima_measurements_count_ops);
- if (IS_ERR(runtime_measurements_count))
- goto out;
-
- violations =
- securityfs_create_file("violations", S_IRUSR | S_IRGRP,
- ima_dir, NULL, &ima_htable_violations_ops);
- if (IS_ERR(violations))
- goto out;
+ struct user_namespace *ns = data;
+
+ switch (msg) {
+ case SECURITYFS_NS_ADD:
+ ima_fs_ns_late_init(ns);
+ break;
+ case SECURITYFS_NS_REMOVE:
+ ima_fs_ns_free_dentries(ns->ima_ns);
+ break;
+ }
+ return 0;
+}
- ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
- ima_dir, NULL,
- &ima_measure_policy_ops);
- if (IS_ERR(ima_policy))
- goto out;
+static struct notifier_block ima_ns_notifier = {
+ .notifier_call = ima_ns_notify,
+};
- return 0;
-out:
- securityfs_remove(violations);
- securityfs_remove(runtime_measurements_count);
- securityfs_remove(ascii_runtime_measurements);
- securityfs_remove(binary_runtime_measurements);
- securityfs_remove(ima_symlink);
- securityfs_remove(ima_dir);
- securityfs_remove(ima_policy);
- return -1;
+int __init ima_fs_init(void)
+{
+ ima_fs_ns_init();
+ return ima_fs_ns_late_init(&init_user_ns);
}
static void ima_fs_ns_free_dentries(struct ima_namespace *ns)
@@ -528,12 +488,10 @@ static void ima_fs_ns_free_dentries(struct ima_namespace *ns)
/* files first */
continue;
}
- securityfs_ns_remove(ns->dentry[i], &ns->mount, &ns->mount_count);
+ securityfs_remove(ns->dentry[i]);
}
- securityfs_ns_remove(ns->dentry[IMAFS_DENTRY_DIR],
- &ns->mount, &ns->mount_count);
- securityfs_ns_remove(ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR],
- &ns->mount, &ns->mount_count);
+ securityfs_remove(ns->dentry[IMAFS_DENTRY_DIR]);
+ securityfs_remove(ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR]);
memset(ns->dentry, 0, sizeof(ns->dentry));
@@ -551,25 +509,27 @@ static int ima_fs_ns_late_init(struct user_namespace *user_ns)
if (ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR])
return 0;
- ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] =
- securityfs_ns_create_dir("integrity", NULL,
- &ns->mount, &ns->mount_count);
+ /* FIXME: update when evm and integrity are namespaced */
+ if (user_ns != &init_user_ns)
+ ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] =
+ securityfs_create_dir("integrity", NULL);
+ else
+ ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] = integrity_dir;
if (IS_ERR(ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR])) {
ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] = NULL;
goto out;
}
ns->dentry[IMAFS_DENTRY_DIR] =
- securityfs_ns_create_dir("ima", ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR],
- &ns->mount, &ns->mount_count);
+ securityfs_create_dir("ima",
+ ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR]);
if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR])) {
ns->dentry[IMAFS_DENTRY_DIR] = NULL;
goto out;
}
ns->dentry[IMAFS_DENTRY_SYMLINK] =
- securityfs_ns_create_symlink("ima", NULL, "integrity/ima", NULL,
- &ns->mount, &ns->mount_count);
+ securityfs_create_symlink("ima", NULL, "integrity/ima", NULL);
if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK])) {
ns->dentry[IMAFS_DENTRY_SYMLINK] = NULL;
goto out;
@@ -577,88 +537,62 @@ static int ima_fs_ns_late_init(struct user_namespace *user_ns)
parent = ns->dentry[IMAFS_DENTRY_DIR];
ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS] =
- securityfs_ns_create_file("binary_runtime_measurements",
+ securityfs_create_file("binary_runtime_measurements",
S_IRUSR | S_IRGRP, parent, NULL,
- &ima_measurements_ops,
- &ns->mount, &ns->mount_count);
+ &ima_measurements_ops);
if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS])) {
ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS] = NULL;
goto out;
}
ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS] =
- securityfs_ns_create_file("ascii_runtime_measurements",
+ securityfs_create_file("ascii_runtime_measurements",
S_IRUSR | S_IRGRP, parent, NULL,
- &ima_ascii_measurements_ops,
- &ns->mount, &ns->mount_count);
+ &ima_ascii_measurements_ops);
if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS])) {
ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS] = NULL;
goto out;
}
ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT] =
- securityfs_ns_create_file("runtime_measurements_count",
+ securityfs_create_file("runtime_measurements_count",
S_IRUSR | S_IRGRP, parent, NULL,
- &ima_measurements_count_ops,
- &ns->mount, &ns->mount_count);
+ &ima_measurements_count_ops);
if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT])) {
ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT] = NULL;
goto out;
}
ns->dentry[IMAFS_DENTRY_VIOLATIONS] =
- securityfs_ns_create_file("violations", S_IRUSR | S_IRGRP,
- parent, NULL, &ima_htable_violations_ops,
- &ns->mount, &ns->mount_count);
+ securityfs_create_file("violations", S_IRUSR | S_IRGRP,
+ parent, NULL, &ima_htable_violations_ops);
if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS])) {
ns->dentry[IMAFS_DENTRY_VIOLATIONS] = NULL;
goto out;
}
ns->dentry[IMAFS_DENTRY_IMA_POLICY] =
- securityfs_ns_create_file("policy", POLICY_FILE_FLAGS,
- parent, NULL, &ima_measure_policy_ops,
- &ns->mount, &ns->mount_count);
+ securityfs_create_file("policy", POLICY_FILE_FLAGS,
+ parent, NULL, &ima_measure_policy_ops);
if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY])) {
ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
goto out;
}
-
return 0;
out:
- ima_fs_ns_free_dentries(ns);
+ ima_fs_ns_free_dentries(user_ns->ima_ns);
return -1;
}
-int ima_fs_ns_init(struct ima_namespace *ns)
+int ima_fs_ns_init(void)
{
- ns->mount = securityfs_ns_create_mount(ns->user_ns);
- if (IS_ERR(ns->mount)) {
- ns->mount = NULL;
- return -1;
- }
- ns->mount_count = 1;
-
- /* Adjust the trigger for user namespace's early teardown of dependent
- * namespaces. Due to the filesystem there's an additional reference
- * to the user namespace.
- */
- ns->user_ns->refcount_teardown += 1;
-
- ns->late_fs_init = ima_fs_ns_late_init;
-
- return 0;
+ return securityfs_register_ns_notifier(&ima_ns_notifier);
}
-void ima_fs_ns_free(struct ima_namespace *ns)
+void ima_fs_ns_free(void)
{
- ima_fs_ns_free_dentries(ns);
- if (ns->mount) {
- mntput(ns->mount);
- ns->mount_count -= 1;
- }
- ns->mount = NULL;
+ securityfs_unregister_ns_notifier(&ima_ns_notifier);
}
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index 86a89502c0c5..38d075a2c38d 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -54,8 +54,6 @@ int ima_init_namespace(struct ima_namespace *ns)
mutex_init(&ns->ima_write_mutex);
ns->valid_policy = 1;
ns->ima_fs_flags = 0;
- if (ns != &init_ima_ns)
- rc = ima_fs_ns_init(ns);
return rc;
}
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 9d5917c97fcc..4c147e0c1801 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -65,13 +65,6 @@ struct ima_namespace *copy_ima_ns(struct ima_namespace *old_ns,
return create_ima_ns(user_ns);
}
-void ima_ns_userns_early_teardown(struct ima_namespace *ns)
-{
- pr_debug("%s: ns=0x%p\n", __func__, ns);
- ima_fs_ns_free(ns);
-}
-EXPORT_SYMBOL(ima_ns_userns_early_teardown);
-
static void destroy_ima_ns(struct ima_namespace *ns)
{
pr_debug("DESTROY ima_ns: 0x%p\n", ns);
--
2.33.0
On Fri, Dec 03, 2021 at 07:33:39PM -0500, Stefan Berger wrote:
>
> On 12/3/21 14:11, Stefan Berger wrote:
> >
> > On 12/3/21 13:50, James Bottomley wrote:
> >
> >
> > >
> > > > Where would the vfsmount pointer reside? For now it's in
> > > > ima_namespace, but it sounds like it should be in a more centralized
> > > > place? Should it also be connected to the user_namespace so we can
> > > > pick it up using get_user_ns()?
> > > exactly. I think struct user_namespace should have two elements gated
> > > by a #ifdef CONFIG_SECURITYFS which are the vfsmount and the
> > > mount_count for passing into simple_pin_fs.
> >
> > Also that we can do for as long as it flies beyond the conversation
> > here... :-) Anyone else have an opinion ?
>
> I moved it now and this greatly reduced the amount of changes. The dentries
> are now all in the ima_namespace and it works with one API. Thanks!
Ideally you only have one entry in struct user_namespace for ima that
encompasses all information needed; not multiple entries. Similar to
what I did for binfmt_misc
https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/commit/?h=fs.binfmt_misc&id=eb50eb90a694e05f6fd6533951a56ca3ed040761
if that works.
On Fri, Dec 03, 2021 at 11:37:14AM -0800, Casey Schaufler wrote:
> On 12/3/2021 10:50 AM, James Bottomley wrote:
> > On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
> > > On 12/3/21 12:03, James Bottomley wrote:
> > > > On Thu, 2021-12-02 at 21:31 -0500, Stefan Berger wrote:
> > > > [...]
> > > > > static int securityfs_init_fs_context(struct fs_context *fc)
> > > > > {
> > > > > + int rc;
> > > > > +
> > > > > + if (fc->user_ns->ima_ns->late_fs_init) {
> > > > > + rc = fc->user_ns->ima_ns->late_fs_init(fc->user_ns);
> > > > > + if (rc)
> > > > > + return rc;
> > > > > + }
> > > > > fc->ops = &securityfs_context_ops;
> > > > > return 0;
> > > > > }
> > > > I know I suggested this, but to get this to work in general, it's
> > > > going to have to not be specific to IMA, so it's going to have to
> > > > become something generic like a notifier chain. The other problem
> > > > is it's only working still by accident:
> > > I had thought about this also but the rationale was:
> > >
> > > securityfs is compiled due to CONFIG_IMA_NS and the user namespace
> > > exists there and that has a pointer now to ima_namespace, which can
> > > have that callback. I assumed that other namespaced subsystems could
> > > also be reached then via such a callback, but I don't know.
> > Well securityfs is supposed to exist for LSMs. At some point each of
> > those is going to need to be namespaced, which may eventually be quite
> > a pile of callbacks, which is why I thought of a notifier.
>
> While AppArmor, lockdown and the integrity family use securityfs,
> SELinux and Smack do not. They have their own independent filesystems.
> Implementations of namespacing for each of SELinux and Smack have been
> proposed, but nothing has been adopted. It would be really handy to
> namespace the infrastructure rather than each individual LSM, but I
> fear that's a bigger project than anyone will be taking on any time
> soon. It's likely to encounter many of the same issues that I've been
> dealing with for module stacking.
The main thing that bothers me is that it uses simple_pin_fs() and
simple_unpin_fs() which I would try hard to get rid of if possible. The
existence of this global pinning logic makes namespacing it properly
more difficult then it needs to be and it creates imho wonky semantics
where the last unmount doesn't really destroy the superblock. Instead
subsequents mounts resurface the same superblock. There might be an
inherent design reason why this needs to be this way but I would advise
against these semantics for anything that wants to be namespaced.
Probably the first securityfs mount in init_user_ns can follow these
semantics but ones tied to a non-initial user namespace should not as
the userns can go away. In that case the pinning logic seems strange as
conceptually the userns pins the securityfs mount as evidenced by the
fact that we key by it in get_tree_keyed().
>
> >
> > > I suppose any late filesystem init callchain would have to be
> > > connected to the user_namespace somehow?
> > I don't think so; I think just moving some securityfs entries into the
> > user_namespace and managing the notifier chain from within securityfs
> > will do for now. [although I'd have to spec this out in code before I
> > knew for sure].
> >
> > > > > +int ima_fs_ns_init(struct ima_namespace *ns)
> > > > > +{
> > > > > + ns->mount = securityfs_ns_create_mount(ns->user_ns);
> > > > This actually triggers on the call to securityfs_init_fs_context,
> > > > but nothing happens because the callback is null. Every subsequent
> > > > use of fscontext will trigger this. The point of a keyed supeblock
> > > > is that fill_super is only called once per key, that's the place we
> > > > should be doing this. It should also probably be a blocking
> > > > notifier so anyconsumer of securityfs can be namespaced by
> > > > registering for this notifier.
> > > What I don't like about the fill_super is that it gets called too
> > > early:
> > >
> > > [ 67.058611] securityfs_ns_create_mount @ 102 target user_ns:
> > > ffff95c010698c80; nr_extents: 0
> > > [ 67.059836] securityfs_fill_super @ 47 user_ns:
> > > ffff95c010698c80;
> > > nr_extents: 0
> > Right, it's being activated by securityfs_ns_create_mount which is
> > called as soon as the user_ns is created.
> >
> > > We are switching to the target user namespace in
> > > securityfs_ns_create_mount. The expected nr_extents at this point is
> > > 0, since user_ns hasn't been configured, yet. But then
> > > security_fill_super is also called with nr_extents 0. We cannot use
> > > that, it's too early!
> > Exactly, so I was thinking of not having a securityfs_ns_create_mount
> > at all. All the securityfs_ns_create.. calls would be in the notifier
> > call chain. This means there's nothing to fill the superblock until an
> > actual mount on it is called.
> >
> > > > > + if (IS_ERR(ns->mount)) {
> > > > > + ns->mount = NULL;
> > > > > + return -1;
> > > > > + }
> > > > > + ns->mount_count = 1;
> > > > This is a bit nasty, too: we're spilling the guts of mount count
> > > > tracking into IMA instead of encapsulating it inside securityfs.
> > > Ok, I can make this disappear.
> > >
> > >
> > > > > +
> > > > > + /* Adjust the trigger for user namespace's early teardown of
> > > > > dependent
> > > > > + * namespaces. Due to the filesystem there's an additional
> > > > > reference
> > > > > + * to the user namespace.
> > > > > + */
> > > > > + ns->user_ns->refcount_teardown += 1;
> > > > > +
> > > > > + ns->late_fs_init = ima_fs_ns_late_init;
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > I think what should be happening is that we shouldn't so the
> > > > simple_pin_fs, which creates the inodes, ahead of time; we should
> > > > do it inside fill_super using a notifier, meaning it gets called
> > > > once per
> > > fill_super would only work for the init_user_ns from what I can see.
> > >
> > >
> > > > key, creates the root dentry then triggers the notifier which
> > > > instantiates all the namespaced entries. We can still use
> > > > simple_pin_fs for this because there's no locking across
> > > > fill_super.
> > > > This would mean fill_super would be called the first time the
> > > > securityfs is mounted inside the namespace.
> > > I guess I would need to know how fill_super would work or how it
> > > could be called late/delayed as well.
> > So it would be called early in the init_user_ns by non-namespaced
> > consumers of securityfs, like it is now.
> >
> > Namespaced consumers wouldn't call any securityfs_ns_create callbacks
> > to create dentries until they were notified from the fill_super
> > notifier, which would now only be triggered on first mount of
> > securityfs inside the namespace.
> >
> > > > If we do it this way, we can now make securityfs have its own mount
> > > > and mount_count inside the user namespace, which it uses internally
> > > > to the securityfs code, thus avoiding exposing them to ima or any
> > > > other namespaced consumer.
> > > >
> > > > I also think we now don't need the securityfs_ns_ duplicated
> > > > functions because the callback via the notifier chain now ensures
> > > > we can usethe namespace they were created in to distinguish between
> > > > non namespaced and namespaced entries.
> > > Is there then no need to pass a separate vfsmount * in anymore?
> > I don't think so no. It could be entirely managed internally to
> > securityfs.
> >
> > > Where would the vfsmount pointer reside? For now it's in
> > > ima_namespace, but it sounds like it should be in a more centralized
> > > place? Should it also be connected to the user_namespace so we can
> > > pick it up using get_user_ns()?
> > exactly. I think struct user_namespace should have two elements gated
> > by a #ifdef CONFIG_SECURITYFS which are the vfsmount and the
> > mount_count for passing into simple_pin_fs.
> >
> >
> > James
> >
> >
>
On Mon, 2021-12-06 at 13:08 +0100, Christian Brauner wrote:
> On Fri, Dec 03, 2021 at 11:37:14AM -0800, Casey Schaufler wrote:
> > On 12/3/2021 10:50 AM, James Bottomley wrote:
> > > On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
> > > > On 12/3/21 12:03, James Bottomley wrote:
> > > > > On Thu, 2021-12-02 at 21:31 -0500, Stefan Berger wrote:
> > > > > [...]
> > > > > > static int securityfs_init_fs_context(struct fs_context
> > > > > > *fc)
> > > > > > {
> > > > > > + int rc;
> > > > > > +
> > > > > > + if (fc->user_ns->ima_ns->late_fs_init) {
> > > > > > + rc = fc->user_ns->ima_ns->late_fs_init(fc-
> > > > > > >user_ns);
> > > > > > + if (rc)
> > > > > > + return rc;
> > > > > > + }
> > > > > > fc->ops = &securityfs_context_ops;
> > > > > > return 0;
> > > > > > }
> > > > > I know I suggested this, but to get this to work in general,
> > > > > it's going to have to not be specific to IMA, so it's going
> > > > > to have to become something generic like a notifier
> > > > > chain. The other problem is it's only working still by
> > > > > accident:
> > > >
> > > > I had thought about this also but the rationale was:
> > > >
> > > > securityfs is compiled due to CONFIG_IMA_NS and the user
> > > > namespace exists there and that has a pointer now to
> > > > ima_namespace, which can have that callback. I assumed that
> > > > other namespaced subsystems could also be reached then via
> > > > such a callback, but I don't know.
> > >
> > > Well securityfs is supposed to exist for LSMs. At some point
> > > each of those is going to need to be namespaced, which may
> > > eventually be quite a pile of callbacks, which is why I thought
> > > of a notifier.
> >
> > While AppArmor, lockdown and the integrity family use securityfs,
> > SELinux and Smack do not. They have their own independent
> > filesystems. Implementations of namespacing for each of SELinux and
> > Smack have been proposed, but nothing has been adopted. It would be
> > really handy to namespace the infrastructure rather than each
> > individual LSM, but I fear that's a bigger project than anyone will
> > be taking on any time soon. It's likely to encounter many of the
> > same issues that I've been dealing with for module stacking.
>
> The main thing that bothers me is that it uses simple_pin_fs() and
> simple_unpin_fs() which I would try hard to get rid of if possible.
> The existence of this global pinning logic makes namespacing it
> properly more difficult then it needs to be and it creates imho wonky
> semantics where the last unmount doesn't really destroy the
> superblock.
So in the notifier sketch I posted, I got rid of the pinning but only
for the non root user namespace use case ... which basically means only
for converted consumers of securityfs. The last unmount of securityfs
inside the namespace now does destroy the superblock ... I checked.
The same isn't true for the last unmount of the root namespace, but
that has to be so to keep the current semantics.
> Instead subsequents mounts resurface the same superblock. There
> might be an inherent design reason why this needs to be this way but
> I would advise against these semantics for anything that wants to be
> namespaced. Probably the first securityfs mount in init_user_ns can
> follow these semantics but ones tied to a non-initial user namespace
> should not as the userns can go away. In that case the pinning logic
> seems strange as conceptually the userns pins the securityfs mount as
> evidenced by the fact that we key by it in get_tree_keyed().
Yes, that's basically what I did: pin if ns == &init_user_ns but don't
pin if not. However, I'm still not sure I got the triggers right. We
have to trigger the notifier call (which adds the namespaced file
entries) from context free, because that's the first place the
superblock mount is fully set up ... I can't do it in fill_super
because the mount isn't fully initialized (and the locking prevents
it). I did manage to get the notifier for teardown triggered from
kill_super, though.
James
On 12/5/21 23:27, James Bottomley wrote:
> On Fri, 2021-12-03 at 14:11 -0500, Stefan Berger wrote:
>> On 12/3/21 13:50, James Bottomley wrote:
>>> On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
> [...]
>>>> I suppose any late filesystem init callchain would have to be
>>>> connected to the user_namespace somehow?
>>>
>>> I don't think so; I think just moving some securityfs entries into
>>> the user_namespace and managing the notifier chain from within
>>> securityfs will do for now. [although I'd have to spec this out in
>>> code before I knew for sure].
>> It doesn't have to be right in the user_namespace. The IMA namespace
>> is connected to the user namespace and holds the dentries now...
>>
>> Please spec it out...
> OK, this is what I have. fill_super turned out to be a locking
> nightmare, so I triggered it from free context instead (which doesn't
> have the once per keyed superblock property, so I added a flag in the
> user namespace). I've got it to the point where the event is triggered
> on mount and unmount, so all the entries for the namespace are added
> when the filesystem is mounted and remove when it's unmounted. This
> style of addition no longer needs the simple_pin_fs, because the
> add/remove callbacks substitute (plus, if we pinned, the free_super
> wouldn't trigger on unmount). The default behaviour still does pinning
> and unpinning, but that can be keyed off the current user_namespace.
>
> This is all on top of your current series ... some of the functions
> should probably be renamed, but I kept them to show how the code was
> migrating in this sketch.
>
> James
>
> ---
>
> From 59c45daa8698c66c3bcebfb194123977d548a9a6 Mon Sep 17 00:00:00 2001
> From: James Bottomley <[email protected]>
> Date: Sat, 4 Dec 2021 16:38:37 +0000
> Subject: [PATCH] rework securityfs
>
> ---
>
> -
> -static void _securityfs_remove(struct dentry *dentry,
> - struct vfsmount **mount, int *mount_count)
> +void securityfs_remove(struct dentry *dentry)
> {
> struct inode *dir;
> + struct user_namespace *ns = current_user_ns();
I had problems with this in this place. So I had to use use
struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
I'll try to split up your patch and post a v3 with then. Or is it too early?
Stefan
On Fri, Dec 03, 2021 at 01:06:13PM -0500, Stefan Berger wrote:
>
> On 12/3/21 12:03, James Bottomley wrote:
> > On Thu, 2021-12-02 at 21:31 -0500, Stefan Berger wrote:
> > [...]
> > > static int securityfs_init_fs_context(struct fs_context *fc)
> > > {
> > > + int rc;
> > > +
> > > + if (fc->user_ns->ima_ns->late_fs_init) {
> > > + rc = fc->user_ns->ima_ns->late_fs_init(fc->user_ns);
> > > + if (rc)
> > > + return rc;
> > > + }
> > > fc->ops = &securityfs_context_ops;
> > > return 0;
> > > }
> > I know I suggested this, but to get this to work in general, it's going
> > to have to not be specific to IMA, so it's going to have to become
> > something generic like a notifier chain. The other problem is it's
> > only working still by accident:
>
> I had thought about this also but the rationale was:
>
> securityfs is compiled due to CONFIG_IMA_NS and the user namespace exists
> there and that has a pointer now to ima_namespace, which can have that
> callback. I assumed that other namespaced subsystems could also be reached
> then via such a callback, but I don't know.
>
> I suppose any late filesystem init callchain would have to be connected to
> the user_namespace somehow?
>
>
> >
> > > +int ima_fs_ns_init(struct ima_namespace *ns)
> > > +{
> > > + ns->mount = securityfs_ns_create_mount(ns->user_ns);
> > This actually triggers on the call to securityfs_init_fs_context, but
> > nothing happens because the callback is null. Every subsequent use of
> > fscontext will trigger this. The point of a keyed supeblock is that
> > fill_super is only called once per key, that's the place we should be
> > doing this. It should also probably be a blocking notifier so any
> > consumer of securityfs can be namespaced by registering for this
> > notifier.
>
>
> What I don't like about the fill_super is that it gets called too early:
>
> [ 67.058611] securityfs_ns_create_mount @ 102 target user_ns:
> ffff95c010698c80; nr_extents: 0
> [ 67.059836] securityfs_fill_super @ 47 user_ns: ffff95c010698c80;
> nr_extents: 0
>
> We are switching to the target user namespace in securityfs_ns_create_mount.
> The expected nr_extents at this point is 0, since user_ns hasn't been
> configured, yet. But then security_fill_super is also called with nr_extents
> 0. We cannot use that, it's too early!
So the problem is that someone could mount securityfs before any
idmappings are setup or what? How does moving the setup to a later stage
help at all? I'm struggling to make sense of this. When or even if
idmappings are written isn't under imas control. Someone could mount
securityfs without any idmappings setup. In that case they should get
what they deserve, everything owner by overflowuid/overflowgid, no? Or
you can require in fill_super that kuid 0 and kgid 0 are mapped and fail
if they aren't.
On Mon, 2021-12-06 at 09:03 -0500, Stefan Berger wrote:
> On 12/5/21 23:27, James Bottomley wrote:
> > On Fri, 2021-12-03 at 14:11 -0500, Stefan Berger wrote:
> > > On 12/3/21 13:50, James Bottomley wrote:
> > > > On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
> > [...]
> > > > > I suppose any late filesystem init callchain would have to be
> > > > > connected to the user_namespace somehow?
> > > >
> > > > I don't think so; I think just moving some securityfs entries
> > > > into
> > > > the user_namespace and managing the notifier chain from within
> > > > securityfs will do for now. [although I'd have to spec this
> > > > out in
> > > > code before I knew for sure].
> > > It doesn't have to be right in the user_namespace. The IMA
> > > namespace
> > > is connected to the user namespace and holds the dentries now...
> > >
> > > Please spec it out...
> > OK, this is what I have. fill_super turned out to be a locking
> > nightmare, so I triggered it from free context instead (which
> > doesn't
> > have the once per keyed superblock property, so I added a flag in
> > the
> > user namespace). I've got it to the point where the event is
> > triggered
> > on mount and unmount, so all the entries for the namespace are
> > added
> > when the filesystem is mounted and remove when it's
> > unmounted. This
> > style of addition no longer needs the simple_pin_fs, because the
> > add/remove callbacks substitute (plus, if we pinned, the free_super
> > wouldn't trigger on unmount). The default behaviour still does
> > pinning
> > and unpinning, but that can be keyed off the current
> > user_namespace.
> >
> > This is all on top of your current series ... some of the functions
> > should probably be renamed, but I kept them to show how the code
> > was
> > migrating in this sketch.
> >
> > James
> >
> > ---
> >
> > From 59c45daa8698c66c3bcebfb194123977d548a9a6 Mon Sep 17 00:00:00
> > 2001
> > From: James Bottomley <[email protected]>
> > Date: Sat, 4 Dec 2021 16:38:37 +0000
> > Subject: [PATCH] rework securityfs
> >
> > ---
> >
> > -
> > -static void _securityfs_remove(struct dentry *dentry,
> > - struct vfsmount **mount, int
> > *mount_count)
> > +void securityfs_remove(struct dentry *dentry)
> > {
> > struct inode *dir;
> > + struct user_namespace *ns = current_user_ns();
>
> I had problems with this in this place. So I had to use use
>
> struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
Yes, I think that works ... the owner in the parent namespace could
actually unmount it, so keying off the user namespace it was mounted on
is definitely the correct form.
> I'll try to split up your patch and post a v3 with then. Or is it too
> early?
It's never too early to see what the series is shaping up as. However,
I'm still not sure I got the right trigger for the SECURITYFS_NS_ADD
notifier, so that may still have to move ... or even that there isn't
some locking subtlety I missed in triggering SECURITY_NS_REMOVE from
kill_sb.
I also suspect Christian will want a pointer to the securityfs pieces
in struct user_namespace rather than discrete elements added directly.
James
On Mon, Dec 06, 2021 at 08:38:29AM -0500, James Bottomley wrote:
> On Mon, 2021-12-06 at 13:08 +0100, Christian Brauner wrote:
> > On Fri, Dec 03, 2021 at 11:37:14AM -0800, Casey Schaufler wrote:
> > > On 12/3/2021 10:50 AM, James Bottomley wrote:
> > > > On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
> > > > > On 12/3/21 12:03, James Bottomley wrote:
> > > > > > On Thu, 2021-12-02 at 21:31 -0500, Stefan Berger wrote:
> > > > > > [...]
> > > > > > > static int securityfs_init_fs_context(struct fs_context
> > > > > > > *fc)
> > > > > > > {
> > > > > > > + int rc;
> > > > > > > +
> > > > > > > + if (fc->user_ns->ima_ns->late_fs_init) {
> > > > > > > + rc = fc->user_ns->ima_ns->late_fs_init(fc-
> > > > > > > >user_ns);
> > > > > > > + if (rc)
> > > > > > > + return rc;
> > > > > > > + }
> > > > > > > fc->ops = &securityfs_context_ops;
> > > > > > > return 0;
> > > > > > > }
> > > > > > I know I suggested this, but to get this to work in general,
> > > > > > it's going to have to not be specific to IMA, so it's going
> > > > > > to have to become something generic like a notifier
> > > > > > chain. The other problem is it's only working still by
> > > > > > accident:
> > > > >
> > > > > I had thought about this also but the rationale was:
> > > > >
> > > > > securityfs is compiled due to CONFIG_IMA_NS and the user
> > > > > namespace exists there and that has a pointer now to
> > > > > ima_namespace, which can have that callback. I assumed that
> > > > > other namespaced subsystems could also be reached then via
> > > > > such a callback, but I don't know.
> > > >
> > > > Well securityfs is supposed to exist for LSMs. At some point
> > > > each of those is going to need to be namespaced, which may
> > > > eventually be quite a pile of callbacks, which is why I thought
> > > > of a notifier.
> > >
> > > While AppArmor, lockdown and the integrity family use securityfs,
> > > SELinux and Smack do not. They have their own independent
> > > filesystems. Implementations of namespacing for each of SELinux and
> > > Smack have been proposed, but nothing has been adopted. It would be
> > > really handy to namespace the infrastructure rather than each
> > > individual LSM, but I fear that's a bigger project than anyone will
> > > be taking on any time soon. It's likely to encounter many of the
> > > same issues that I've been dealing with for module stacking.
> >
> > The main thing that bothers me is that it uses simple_pin_fs() and
> > simple_unpin_fs() which I would try hard to get rid of if possible.
> > The existence of this global pinning logic makes namespacing it
> > properly more difficult then it needs to be and it creates imho wonky
> > semantics where the last unmount doesn't really destroy the
> > superblock.
>
> So in the notifier sketch I posted, I got rid of the pinning but only
> for the non root user namespace use case ... which basically means only
> for converted consumers of securityfs. The last unmount of securityfs
> inside the namespace now does destroy the superblock ... I checked.
Yeah, I saw. I'm struggling to follow the series but I pulled Stefan's
branch and put your patch on top of it so I peruse it.
>
> The same isn't true for the last unmount of the root namespace, but
> that has to be so to keep the current semantics.
>
> > Instead subsequents mounts resurface the same superblock. There
> > might be an inherent design reason why this needs to be this way but
> > I would advise against these semantics for anything that wants to be
> > namespaced. Probably the first securityfs mount in init_user_ns can
> > follow these semantics but ones tied to a non-initial user namespace
> > should not as the userns can go away. In that case the pinning logic
> > seems strange as conceptually the userns pins the securityfs mount as
> > evidenced by the fact that we key by it in get_tree_keyed().
>
> Yes, that's basically what I did: pin if ns == &init_user_ns but don't
> pin if not. However, I'm still not sure I got the triggers right. We
> have to trigger the notifier call (which adds the namespaced file
> entries) from context free, because that's the first place the
> superblock mount is fully set up ... I can't do it in fill_super
> because the mount isn't fully initialized (and the locking prevents
> it). I did manage to get the notifier for teardown triggered from
> kill_super, though.
Once Stefan answer my questions about fill_super I _might_ have an idea
how to improve this.
On Mon, 2021-12-06 at 15:11 +0100, Christian Brauner wrote:
> On Fri, Dec 03, 2021 at 01:06:13PM -0500, Stefan Berger wrote:
> > On 12/3/21 12:03, James Bottomley wrote:
[...]
> > > > +int ima_fs_ns_init(struct ima_namespace *ns)
> > > > +{
> > > > + ns->mount = securityfs_ns_create_mount(ns->user_ns);
> > >
> > > This actually triggers on the call to securityfs_init_fs_context,
> > > but nothing happens because the callback is null. Every
> > > subsequent use of fscontext will trigger this. The point of a
> > > keyed supeblock is that fill_super is only called once per key,
> > > that's the place we should be doing this. It should also
> > > probably be a blocking notifier so any consumer of securityfs can
> > > be namespaced by registering for this notifier.
> >
> > What I don't like about the fill_super is that it gets called too
> > early:
> >
> > [ 67.058611] securityfs_ns_create_mount @ 102 target user_ns:
> > ffff95c010698c80; nr_extents: 0
> > [ 67.059836] securityfs_fill_super @ 47 user_ns:
> > ffff95c010698c80;
> > nr_extents: 0
> >
> > We are switching to the target user namespace in
> > securityfs_ns_create_mount. The expected nr_extents at this point
> > is 0, since user_ns hasn't been configured, yet. But then
> > security_fill_super is also called with nr_extents 0. We cannot use
> > that, it's too early!
>
> So the problem is that someone could mount securityfs before any
> idmappings are setup or what?
Yes, not exactly: we put a call to initialize IMA in create_user_ns()
but it's too early to have the mappings, so we can't create the
securityfs entries in that call. We need the inode to pick up the root
owner from the s_user_ns mappings, so we can't create the dentries for
the IMA securityfs entries until those mappings exist.
I'm assuming that by the time someone tries to mount securityfs inside
the namespace, the mappings are set up, which is why triggering the
notifier to add the files on first mount seems like the best place to
put it.
> How does moving the setup to a later stage help at all? I'm
> struggling to make sense of this.
It's not moving all the setup, just the creation of the securityfs
entries.
> When or even if idmappings are written isn't under imas control.
> Someone could mount securityfs without any idmappings setup. In that
> case they should get what they deserve, everything owner by
> overflowuid/overflowgid, no?
Right, in the current scheme of doing things, if they still haven't
written the mappings by the time they do the mount, they're just going
to get nobody/nogroup as uid/gid, but that's their own fault.
> Or you can require in fill_super that kuid 0 and kgid 0 are mapped
> and fail if they aren't.
We can't create the securityfs entries in fill_super ... I already
tried and the locking just won't allow it. And if we create them ahead
of time, that create of the entries will trigger fill_super because we
need the superblock to hang the dentries off.
James
On Mon, Dec 06, 2021 at 09:21:15AM -0500, James Bottomley wrote:
> On Mon, 2021-12-06 at 15:11 +0100, Christian Brauner wrote:
> > On Fri, Dec 03, 2021 at 01:06:13PM -0500, Stefan Berger wrote:
> > > On 12/3/21 12:03, James Bottomley wrote:
> [...]
> > > > > +int ima_fs_ns_init(struct ima_namespace *ns)
> > > > > +{
> > > > > + ns->mount = securityfs_ns_create_mount(ns->user_ns);
> > > >
> > > > This actually triggers on the call to securityfs_init_fs_context,
> > > > but nothing happens because the callback is null. Every
> > > > subsequent use of fscontext will trigger this. The point of a
> > > > keyed supeblock is that fill_super is only called once per key,
> > > > that's the place we should be doing this. It should also
> > > > probably be a blocking notifier so any consumer of securityfs can
> > > > be namespaced by registering for this notifier.
> > >
> > > What I don't like about the fill_super is that it gets called too
> > > early:
> > >
> > > [ 67.058611] securityfs_ns_create_mount @ 102 target user_ns:
> > > ffff95c010698c80; nr_extents: 0
> > > [ 67.059836] securityfs_fill_super @ 47 user_ns:
> > > ffff95c010698c80;
> > > nr_extents: 0
> > >
> > > We are switching to the target user namespace in
> > > securityfs_ns_create_mount. The expected nr_extents at this point
> > > is 0, since user_ns hasn't been configured, yet. But then
> > > security_fill_super is also called with nr_extents 0. We cannot use
> > > that, it's too early!
> >
> > So the problem is that someone could mount securityfs before any
> > idmappings are setup or what?
>
> Yes, not exactly: we put a call to initialize IMA in create_user_ns()
> but it's too early to have the mappings, so we can't create the
> securityfs entries in that call. We need the inode to pick up the root
> owner from the s_user_ns mappings, so we can't create the dentries for
> the IMA securityfs entries until those mappings exist.
>
> I'm assuming that by the time someone tries to mount securityfs inside
> the namespace, the mappings are set up, which is why triggering the
> notifier to add the files on first mount seems like the best place to
> put it.
>
> > How does moving the setup to a later stage help at all? I'm
> > struggling to make sense of this.
>
> It's not moving all the setup, just the creation of the securityfs
> entries.
>
> > When or even if idmappings are written isn't under imas control.
> > Someone could mount securityfs without any idmappings setup. In that
> > case they should get what they deserve, everything owner by
> > overflowuid/overflowgid, no?
>
> Right, in the current scheme of doing things, if they still haven't
> written the mappings by the time they do the mount, they're just going
> to get nobody/nogroup as uid/gid, but that's their own fault.
>
> > Or you can require in fill_super that kuid 0 and kgid 0 are mapped
> > and fail if they aren't.
>
> We can't create the securityfs entries in fill_super ... I already
> tried and the locking just won't allow it. And if we create them ahead
What is the locking issue there exactly?
I'm looking at ima_fs_ns_late_init() and there's nothing there that
would cause obvious issues. You might not be able to use
securityfs_create_*() in there for some reason but that just means you
need to add a simple helper. Nearly every filesystem that needs to
pre-create files does it in fill_super. So I really fail to see what the
issue is currently. I mist just miss something obvious.
On Mon, 2021-12-06 at 15:42 +0100, Christian Brauner wrote:
> On Mon, Dec 06, 2021 at 09:21:15AM -0500, James Bottomley wrote:
> > On Mon, 2021-12-06 at 15:11 +0100, Christian Brauner wrote:
> > > On Fri, Dec 03, 2021 at 01:06:13PM -0500, Stefan Berger wrote:
> > > > On 12/3/21 12:03, James Bottomley wrote:
> > [...]
> > > > > > +int ima_fs_ns_init(struct ima_namespace *ns)
> > > > > > +{
> > > > > > + ns->mount = securityfs_ns_create_mount(ns->user_ns);
> > > > >
> > > > > This actually triggers on the call to
> > > > > securityfs_init_fs_context,
> > > > > but nothing happens because the callback is null. Every
> > > > > subsequent use of fscontext will trigger this. The point of
> > > > > a
> > > > > keyed supeblock is that fill_super is only called once per
> > > > > key,
> > > > > that's the place we should be doing this. It should also
> > > > > probably be a blocking notifier so any consumer of securityfs
> > > > > can
> > > > > be namespaced by registering for this notifier.
> > > >
> > > > What I don't like about the fill_super is that it gets called
> > > > too
> > > > early:
> > > >
> > > > [ 67.058611] securityfs_ns_create_mount @ 102 target user_ns:
> > > > ffff95c010698c80; nr_extents: 0
> > > > [ 67.059836] securityfs_fill_super @ 47 user_ns:
> > > > ffff95c010698c80;
> > > > nr_extents: 0
> > > >
> > > > We are switching to the target user namespace in
> > > > securityfs_ns_create_mount. The expected nr_extents at this
> > > > point
> > > > is 0, since user_ns hasn't been configured, yet. But then
> > > > security_fill_super is also called with nr_extents 0. We cannot
> > > > use
> > > > that, it's too early!
> > >
> > > So the problem is that someone could mount securityfs before any
> > > idmappings are setup or what?
> >
> > Yes, not exactly: we put a call to initialize IMA in
> > create_user_ns()
> > but it's too early to have the mappings, so we can't create the
> > securityfs entries in that call. We need the inode to pick up the
> > root
> > owner from the s_user_ns mappings, so we can't create the dentries
> > for
> > the IMA securityfs entries until those mappings exist.
> >
> > I'm assuming that by the time someone tries to mount securityfs
> > inside
> > the namespace, the mappings are set up, which is why triggering the
> > notifier to add the files on first mount seems like the best place
> > to
> > put it.
> >
> > > How does moving the setup to a later stage help at all? I'm
> > > struggling to make sense of this.
> >
> > It's not moving all the setup, just the creation of the securityfs
> > entries.
> >
> > > When or even if idmappings are written isn't under imas control.
> > > Someone could mount securityfs without any idmappings setup. In
> > > that
> > > case they should get what they deserve, everything owner by
> > > overflowuid/overflowgid, no?
> >
> > Right, in the current scheme of doing things, if they still haven't
> > written the mappings by the time they do the mount, they're just
> > going
> > to get nobody/nogroup as uid/gid, but that's their own fault.
> >
> > > Or you can require in fill_super that kuid 0 and kgid 0 are
> > > mapped
> > > and fail if they aren't.
> >
> > We can't create the securityfs entries in fill_super ... I already
> > tried and the locking just won't allow it. And if we create them
> > ahead
>
> What is the locking issue there exactly?
The main problem is we have no vfsmount and we can't create one in
there because the fill super is triggered by the vfsmount creation for
the actual mount. It's all done under the sb->s_umount semaphore.
> I'm looking at ima_fs_ns_late_init() and there's nothing there that
> would cause obvious issues. You might not be able to use
> securityfs_create_*() in there for some reason but that just means
> you need to add a simple helper. Nearly every filesystem that needs
> to pre-create files does it in fill_super. So I really fail to see
> what the issue is currently. I mist just miss something obvious.
I think we might get it to work if we keep the root dentry in the
securityfs namespace entries instead of the vfsmount; I'll investigate.
James
On Mon, Dec 06, 2021 at 08:38:29AM -0500, James Bottomley wrote:
> On Mon, 2021-12-06 at 13:08 +0100, Christian Brauner wrote:
> > On Fri, Dec 03, 2021 at 11:37:14AM -0800, Casey Schaufler wrote:
> > > On 12/3/2021 10:50 AM, James Bottomley wrote:
> > > > On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
> > > > > On 12/3/21 12:03, James Bottomley wrote:
> > > > > > On Thu, 2021-12-02 at 21:31 -0500, Stefan Berger wrote:
> > > > > > [...]
> > > > > > > static int securityfs_init_fs_context(struct fs_context
> > > > > > > *fc)
> > > > > > > {
> > > > > > > + int rc;
> > > > > > > +
> > > > > > > + if (fc->user_ns->ima_ns->late_fs_init) {
> > > > > > > + rc = fc->user_ns->ima_ns->late_fs_init(fc-
> > > > > > > >user_ns);
> > > > > > > + if (rc)
> > > > > > > + return rc;
> > > > > > > + }
> > > > > > > fc->ops = &securityfs_context_ops;
> > > > > > > return 0;
> > > > > > > }
> > > > > > I know I suggested this, but to get this to work in general,
> > > > > > it's going to have to not be specific to IMA, so it's going
> > > > > > to have to become something generic like a notifier
> > > > > > chain. The other problem is it's only working still by
> > > > > > accident:
> > > > >
> > > > > I had thought about this also but the rationale was:
> > > > >
> > > > > securityfs is compiled due to CONFIG_IMA_NS and the user
> > > > > namespace exists there and that has a pointer now to
> > > > > ima_namespace, which can have that callback. I assumed that
> > > > > other namespaced subsystems could also be reached then via
> > > > > such a callback, but I don't know.
> > > >
> > > > Well securityfs is supposed to exist for LSMs. At some point
> > > > each of those is going to need to be namespaced, which may
> > > > eventually be quite a pile of callbacks, which is why I thought
> > > > of a notifier.
> > >
> > > While AppArmor, lockdown and the integrity family use securityfs,
> > > SELinux and Smack do not. They have their own independent
> > > filesystems. Implementations of namespacing for each of SELinux and
> > > Smack have been proposed, but nothing has been adopted. It would be
> > > really handy to namespace the infrastructure rather than each
> > > individual LSM, but I fear that's a bigger project than anyone will
> > > be taking on any time soon. It's likely to encounter many of the
> > > same issues that I've been dealing with for module stacking.
> >
> > The main thing that bothers me is that it uses simple_pin_fs() and
> > simple_unpin_fs() which I would try hard to get rid of if possible.
> > The existence of this global pinning logic makes namespacing it
> > properly more difficult then it needs to be and it creates imho wonky
> > semantics where the last unmount doesn't really destroy the
> > superblock.
>
> So in the notifier sketch I posted, I got rid of the pinning but only
> for the non root user namespace use case ... which basically means only
> for converted consumers of securityfs. The last unmount of securityfs
> inside the namespace now does destroy the superblock ... I checked.
>
> The same isn't true for the last unmount of the root namespace, but
> that has to be so to keep the current semantics.
>
> > Instead subsequents mounts resurface the same superblock. There
> > might be an inherent design reason why this needs to be this way but
> > I would advise against these semantics for anything that wants to be
> > namespaced. Probably the first securityfs mount in init_user_ns can
> > follow these semantics but ones tied to a non-initial user namespace
> > should not as the userns can go away. In that case the pinning logic
> > seems strange as conceptually the userns pins the securityfs mount as
> > evidenced by the fact that we key by it in get_tree_keyed().
>
> Yes, that's basically what I did: pin if ns == &init_user_ns but don't
> pin if not. However, I'm still not sure I got the triggers right. We
> have to trigger the notifier call (which adds the namespaced file
> entries) from context free, because that's the first place the
> superblock mount is fully set up ... I can't do it in fill_super
> because the mount isn't fully initialized (and the locking prevents
> it). I did manage to get the notifier for teardown triggered from
> kill_super, though.
I don't think you need a vfsmount at all to be honest. I think this can
all be done without much ceremony. Here's a brutalist completely
untested patch outlining one approach:
From 4fc2d88d4194e3473fd545864a8bb0759036ed5e Mon Sep 17 00:00:00 2001
From: Christian Brauner <[email protected]>
Date: Mon, 6 Dec 2021 14:08:28 +0100
Subject: [PATCH] !!!! HERE BE DRAGONS - COMPLETELY UNTESTED !!!!
---
include/linux/securityfs.h | 20 +++++
include/linux/user_namespace.h | 1 +
kernel/user_namespace.c | 3 +
security/inode.c | 129 ++++++++++++++++++++++++++++++--
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_fs.c | 20 ++++-
6 files changed, 165 insertions(+), 9 deletions(-)
create mode 100644 include/linux/securityfs.h
diff --git a/include/linux/securityfs.h b/include/linux/securityfs.h
new file mode 100644
index 000000000000..2e973be160b1
--- /dev/null
+++ b/include/linux/securityfs.h
@@ -0,0 +1,20 @@
+#ifndef __LINUX_SECURITYFS_H
+#define __LINUX_SECURITYFS_H
+
+struct vfsmount;
+
+#ifdef CONFIG_SECURITYFS
+
+/*
+ * Allocated once per user_ns the first time securityfs is mounted. Can be
+ * used to stash securityfs relevant state that absolutely needs to survive
+ * super_block destruction on last umount.
+ */
+struct securityfs_info {
+ // pointer to relevant ima stuff or instance?
+ // pointer to relevant apparmor stuff or instance?
+ // pointer to relevant selinux stuff or instance?
+};
+#endif /* CONFIG_SECURITYFS */
+
+#endif /* ! __LINUX_SECURITYFS_H */
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 6b8bd060d8c4..42676f5bcd43 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -103,6 +103,7 @@ struct user_namespace {
#ifdef CONFIG_IMA
struct ima_namespace *ima_ns;
#endif
+ struct securityfs_info *securityfs_info;
#ifdef CONFIG_SECURITYFS
struct vfsmount *securityfs_mount;
bool securityfs_notifier_sent;
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index c26885343b19..d65b20d8a90b 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -211,6 +211,9 @@ static void free_user_ns(struct work_struct *work)
}
#ifdef CONFIG_IMA
put_ima_ns(ns->ima_ns);
+#endif
+#ifdef CONFIG_SECURITYFS
+ kfree(ns->securityfs_info);
#endif
retire_userns_sysctls(ns);
key_free_user_ns(ns);
diff --git a/security/inode.c b/security/inode.c
index 62ab4630dc31..1c3b2797367d 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -66,18 +66,57 @@ static void securityfs_free_context(struct fs_context *fc)
mntput(ns->securityfs_mount);
}
+
+/*
+ * This is really just a helper we would need in case we wanted to retrieve
+ * securityfs_info independent of the super_block. If that's not needed, then
+ * you can as well remove the smp_load_acquire() and the associated
+ * smp_store_release().
+ */
+struct securitfs_info *to_securityfs_info(struct user_namespace *user_ns)
+{
+
+ return smp_load_acquire(&user_ns->securityfs_info);
+}
+
static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc)
{
+ int err;
static const struct tree_descr files[] = {{""}};
- int error;
-
- error = simple_fill_super(sb, SECURITYFS_MAGIC, files);
- if (error)
- return error;
+ struct user_namespace *user_ns = sb->s_user_ns;
+ struct securityfs_info *securityfs_info;
+
+ /*
+ * Allocate a new securityfs_info instance for this userns.
+ * While multiple superblocks can exist they are keyed by userns in
+ * s_fs_info for user_ns. Hence, the vfs guarantees that
+ * securityfs_fill_super() is called exactly once whenever a
+ * securityfs superblock for a userns is created. This in turn lets us
+ * conclude that when a securityfs superblock is created for the first
+ * time for a userns there's no one racing us. Therefore we don't need
+ * any barriers when we dereference securityfs_info.
+ */
+ securityfs_info = user_ns->securityfs_info;
+ if (!securityfs_info) {
+ securityfs_info = kzalloc(sizeof(struct securityfs_info), GFP_KERNEL);
+ if (!securityfs_info)
+ return -ENOMEM;
+
+ // TODO: Initialize securityfs_info
+
+ /*
+ * Pairs with smp_load_acquire() in to_securityfs_info().
+ *
+ * Please see the commment there.
+ */
+ smp_store_release(&user_ns->securityfs_info, securityfs_info);
+ }
- sb->s_op = &securityfs_super_operations;
+ err = simple_fill_super(sb, SECURITYFS_MAGIC, files);
+ if (!err)
+ sb->s_op = &securityfs_super_operations;
- return 0;
+ return ima_fs_ns_late_init(sb);
}
static int securityfs_get_tree(struct fs_context *fc)
@@ -237,6 +276,82 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
return dentry;
}
+struct dentry *securityfs_create_dentry_ns(struct super_block *sb,
+ const char *name, umode_t mode,
+ struct dentry *parent, void *data,
+ const struct file_operations *fops,
+ const struct inode_operations *iops)
+{
+ struct dentry *dentry;
+ struct inode *dir, *inode;
+ int error;
+ struct user_namespace *ns = sb->s_user_ns;
+
+ if (!(mode & S_IFMT))
+ mode = (mode & S_IALLUGO) | S_IFREG;
+
+ pr_debug("securityfs: creating file '%s', ns=%u\n",name, ns->ns.inum);
+
+ if (ns == &init_user_ns) {
+ error = simple_pin_fs(&fs_type, &ns->securityfs_mount,
+ &securityfs_mount_count);
+ if (error)
+ return ERR_PTR(error);
+ }
+
+ /* You really just require to always pass the parent? */
+ if (!parent)
+ parent = sb->s_root;
+
+ dir = d_inode(parent);
+
+ inode_lock(dir);
+ dentry = lookup_one_len(name, parent, strlen(name));
+ if (IS_ERR(dentry))
+ goto out;
+
+ if (d_really_is_positive(dentry)) {
+ error = -EEXIST;
+ goto out1;
+ }
+
+ inode = new_inode(dir->i_sb);
+ if (!inode) {
+ error = -ENOMEM;
+ goto out1;
+ }
+
+ inode->i_ino = get_next_ino();
+ inode->i_mode = mode;
+ inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
+ inode->i_private = data;
+ if (S_ISDIR(mode)) {
+ inode->i_op = &simple_dir_inode_operations;
+ inode->i_fop = &simple_dir_operations;
+ inc_nlink(inode);
+ inc_nlink(dir);
+ } else if (S_ISLNK(mode)) {
+ inode->i_op = iops ? iops : &simple_symlink_inode_operations;
+ inode->i_link = data;
+ } else {
+ inode->i_fop = fops;
+ }
+ d_instantiate(dentry, inode);
+ dget(dentry);
+ inode_unlock(dir);
+ return dentry;
+
+out1:
+ dput(dentry);
+ dentry = ERR_PTR(error);
+out:
+ inode_unlock(dir);
+ if (ns == &init_user_ns)
+ simple_release_fs(&ns->securityfs_mount,
+ &securityfs_mount_count);
+ return dentry;
+}
+
/**
* securityfs_create_file - create a file in the securityfs filesystem
*
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 12b7df65a5ff..806f19215052 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -140,6 +140,7 @@ struct ns_status {
int ima_init(void);
int ima_fs_init(void);
void ima_fs_ns_free(void);
+int ima_fs_ns_late_init(struct super_block *sb);
int ima_add_template_entry(struct ima_namespace *ns,
struct ima_template_entry *entry, int violation,
const char *op, struct inode *inode,
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 26f26e8756a8..4b25912db448 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -500,11 +500,27 @@ static void ima_fs_ns_free_dentries(struct ima_namespace *ns)
/* Function to populeate namespace SecurityFS once user namespace
* has been configured.
*/
-static int ima_fs_ns_late_init(struct user_namespace *user_ns)
+int ima_fs_ns_late_init(struct super_block *sb)
{
- struct ima_namespace *ns = user_ns->ima_ns;
+ /*
+ * We know that s_user_ns === ima_ns->user_ns.
+ *
+ * In other words, here we can go from superblock to relevant
+ * namespaces never from namespace to superblock. Ideally we try to
+ * avoid going from namespace to superblock.
+ */
+ struct ima_namespace *ns = sb->s_user_ns->ima_ns;
struct dentry *parent;
+
+ // TODO:
+ //
+ // Port this to use new helpers that take a super_block as argument.
+ //
+ // This allows us to get rid of any vfsmount dependencies.
+ //
+ // Probably should also be renamed to something better.
+
/* already initialized? */
if (ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR])
return 0;
--
2.30.2
On Mon, 2021-12-06 at 16:44 +0100, Christian Brauner wrote:
> On Mon, Dec 06, 2021 at 08:38:29AM -0500, James Bottomley wrote:
> > On Mon, 2021-12-06 at 13:08 +0100, Christian Brauner wrote:
[...]
> > > Instead subsequents mounts resurface the same superblock. There
> > > might be an inherent design reason why this needs to be this way
> > > but I would advise against these semantics for anything that
> > > wants to be namespaced. Probably the first securityfs mount in
> > > init_user_ns can follow these semantics but ones tied to a non-
> > > initial user namespace should not as the userns can go away. In
> > > that case the pinning logic seems strange as conceptually the
> > > userns pins the securityfs mount as evidenced by the fact that we
> > > key by it in get_tree_keyed().
> >
> > Yes, that's basically what I did: pin if ns == &init_user_ns but
> > don't pin if not. However, I'm still not sure I got the triggers
> > right. We have to trigger the notifier call (which adds the
> > namespaced file entries) from context free, because that's the
> > first place the superblock mount is fully set up ... I can't do it
> > in fill_super because the mount isn't fully initialized (and the
> > locking prevents it). I did manage to get the notifier for
> > teardown triggered from kill_super, though.
>
> I don't think you need a vfsmount at all to be honest. I think this
> can all be done without much ceremony. Here's a brutalist completely
> untested patch outlining one approach:
This is what I did (incremental to Stefan's series + my previous
patch): it avoids superblock threading by switching to a root dentry in
the securityfs user namespace area ... or am I being too simple again
... ?
I'm still a bit unhappy about triggering a blocking notifier under the
umount semaphore ...
James
---
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 6b8bd060d8c4..03a0879376a0 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -104,8 +104,7 @@ struct user_namespace {
struct ima_namespace *ima_ns;
#endif
#ifdef CONFIG_SECURITYFS
- struct vfsmount *securityfs_mount;
- bool securityfs_notifier_sent;
+ struct dentry *securityfs_root;
#endif
} __randomize_layout;
diff --git a/security/inode.c b/security/inode.c
index 62ab4630dc31..863fccfd3687 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -25,6 +25,7 @@
#include <linux/user_namespace.h>
#include <linux/ima.h>
+static struct vfsmount *securityfs_mount;
static int securityfs_mount_count;
static BLOCKING_NOTIFIER_HEAD(securityfs_ns_notifier);
@@ -41,42 +42,22 @@ static const struct super_operations securityfs_super_operations = {
.free_inode = securityfs_free_inode,
};
-static struct file_system_type fs_type;
-
-static void securityfs_free_context(struct fs_context *fc)
-{
- struct user_namespace *ns = fc->user_ns;
- if (ns == &init_user_ns ||
- ns->securityfs_notifier_sent)
- return;
-
- ns->securityfs_notifier_sent = true;
-
- ns->securityfs_mount = vfs_kern_mount(&fs_type, SB_KERNMOUNT,
- fs_type.name, NULL);
- if (IS_ERR(ns->securityfs_mount)) {
- printk(KERN_ERR "kern mount on securityfs ERROR: %ld\n",
- PTR_ERR(ns->securityfs_mount));
- ns->securityfs_mount = NULL;
- return;
- }
-
- blocking_notifier_call_chain(&securityfs_ns_notifier,
- SECURITYFS_NS_ADD, fc->user_ns);
- mntput(ns->securityfs_mount);
-}
-
static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc)
{
static const struct tree_descr files[] = {{""}};
int error;
+ struct user_namespace *ns = fc->user_ns;
error = simple_fill_super(sb, SECURITYFS_MAGIC, files);
if (error)
return error;
+ ns->securityfs_root = sb->s_root;
+
sb->s_op = &securityfs_super_operations;
+ blocking_notifier_call_chain(&securityfs_ns_notifier,
+ SECURITYFS_NS_ADD, ns);
return 0;
}
@@ -87,7 +68,6 @@ static int securityfs_get_tree(struct fs_context *fc)
static const struct fs_context_operations securityfs_context_ops = {
.get_tree = securityfs_get_tree,
- .free = securityfs_free_context,
};
static int securityfs_init_fs_context(struct fs_context *fc)
@@ -104,8 +84,7 @@ static void securityfs_kill_super(struct super_block *sb)
blocking_notifier_call_chain(&securityfs_ns_notifier,
SECURITYFS_NS_REMOVE,
sb->s_fs_info);
- ns->securityfs_notifier_sent = false;
- ns->securityfs_mount = NULL;
+ ns->securityfs_root = NULL;
kill_litter_super(sb);
}
@@ -179,14 +158,18 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
pr_debug("securityfs: creating file '%s', ns=%u\n",name, ns->ns.inum);
if (ns == &init_user_ns) {
- error = simple_pin_fs(&fs_type, &ns->securityfs_mount,
+ error = simple_pin_fs(&fs_type, &securityfs_mount,
&securityfs_mount_count);
if (error)
return ERR_PTR(error);
}
- if (!parent)
- parent = ns->securityfs_mount->mnt_root;
+ if (!parent) {
+ if (ns == &init_user_ns)
+ parent = securityfs_mount->mnt_root;
+ else
+ parent = ns->securityfs_root;
+ }
dir = d_inode(parent);
@@ -232,7 +215,7 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
out:
inode_unlock(dir);
if (ns == &init_user_ns)
- simple_release_fs(&ns->securityfs_mount,
+ simple_release_fs(&securityfs_mount,
&securityfs_mount_count);
return dentry;
}
@@ -376,7 +359,7 @@ void securityfs_remove(struct dentry *dentry)
}
inode_unlock(dir);
if (ns == &init_user_ns)
- simple_release_fs(&ns->securityfs_mount,
+ simple_release_fs(&securityfs_mount,
&securityfs_mount_count);
}
EXPORT_SYMBOL(securityfs_remove);
@@ -405,8 +388,6 @@ static int __init securityfs_init(void)
if (retval)
return retval;
- init_user_ns.securityfs_mount = NULL;
-
retval = register_filesystem(&fs_type);
if (retval) {
sysfs_remove_mount_point(kernel_kobj, "security");
On 12/6/21 09:11, James Bottomley wrote:
> On Mon, 2021-12-06 at 09:03 -0500, Stefan Berger wrote:
>
> It's never too early to see what the series is shaping up as. However,
> I'm still not sure I got the right trigger for the SECURITYFS_NS_ADD
> notifier, so that may still have to move ... or even that there isn't
> some locking subtlety I missed in triggering SECURITY_NS_REMOVE from
> kill_sb.
I'll post v3 with the early changes and your Signed-off-by's added where
I made changes to files.
Stefan