| Message ID | 20211208221818.1519628-16-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Namespace IMA with audit support in IMA-ns | expand |
On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote: > Move the dentries into the ima_namespace for reuse by virtualized > SecurityFS. Implement function freeing the dentries in order of > files and symlinks before directories. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- This doesn't work as implemented, I think. What I would have preferred and what I tried to explain in the earlier review was: Keep the dentry stashing global since it is only needed for init_ima_ns. Then struct ima_namespace becomes way smaller and simpler. If you do that then it makes sense to remove the additional dget() in securityfs_create_dentry() for non-init_ima_ns. Then you can rely on auto-cleanup in .kill_sb() or on ima_securityfs_init() failure and you only need to call ima_fs_ns_free_dentries() if ns != init_ima_ns. IIuc, it seems you're currently doing one dput() too many since you're calling securityfs_remove() in the error path for non-init_ima_ns which relies on the previous increased dget() which we removed. > include/linux/ima.h | 13 ++++++ > security/integrity/ima/ima_fs.c | 72 ++++++++++++++++++--------------- > 2 files changed, 52 insertions(+), 33 deletions(-) > > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 3aaf6e806db4..4dd64e318b15 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -220,6 +220,17 @@ struct ima_h_table { > struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE]; > }; > > +enum { > + IMAFS_DENTRY_DIR = 0, > + 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 +277,8 @@ struct ima_namespace { > struct mutex ima_write_mutex; > unsigned long ima_fs_flags; > int valid_policy; > + > + struct dentry *dentry[IMAFS_DENTRY_LAST]; > }; > > extern struct ima_namespace init_ima_ns; > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c > index a749a3e79304..3810d11fb463 100644 > --- a/security/integrity/ima/ima_fs.c > +++ b/security/integrity/ima/ima_fs.c > @@ -360,14 +360,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,8 +429,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) > - securityfs_remove(ima_policy); > - ima_policy = NULL; > + securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]); > + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL; > #elif defined(CONFIG_IMA_WRITE_POLICY) > clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags); > #elif defined(CONFIG_IMA_READ_POLICY) > @@ -455,58 +447,72 @@ static const struct file_operations ima_measure_policy_ops = { > .llseek = generic_file_llseek, > }; > > -int __init ima_fs_init(void) > +static void ima_fs_ns_free_dentries(struct ima_namespace *ns) > { > - ima_dir = securityfs_create_dir("ima", integrity_dir); > - if (IS_ERR(ima_dir)) > + int i; > + > + for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--) > + securityfs_remove(ns->dentry[i]); > + > + memset(ns->dentry, 0, sizeof(ns->dentry)); > +} > + > +static int __init ima_securityfs_init(struct user_namespace *user_ns) > +{ > + struct ima_namespace *ns = user_ns->ima_ns; > + struct dentry *ima_dir; > + > + ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_dir); > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR])) > return -1; > + ima_dir = ns->dentry[IMAFS_DENTRY_DIR]; > > - ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima", > - NULL); > - if (IS_ERR(ima_symlink)) > + ns->dentry[IMAFS_DENTRY_SYMLINK] = > + securityfs_create_symlink("ima", NULL, "integrity/ima", NULL); > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK])) > goto out; > > - binary_runtime_measurements = > + ns->dentry[IMAFS_DENTRY_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)) > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS])) > goto out; > > - ascii_runtime_measurements = > + ns->dentry[IMAFS_DENTRY_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)) > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS])) > goto out; > > - runtime_measurements_count = > + ns->dentry[IMAFS_DENTRY_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)) > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT])) > goto out; > > - violations = > + ns->dentry[IMAFS_DENTRY_VIOLATIONS] = > securityfs_create_file("violations", S_IRUSR | S_IRGRP, > ima_dir, NULL, &ima_htable_violations_ops); > - if (IS_ERR(violations)) > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS])) > goto out; > > - ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS, > + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = > + securityfs_create_file("policy", POLICY_FILE_FLAGS, > ima_dir, NULL, > &ima_measure_policy_ops); > - if (IS_ERR(ima_policy)) > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY])) > goto out; > > 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); > + ima_fs_ns_free_dentries(ns); > return -1; > } > + > +int __init ima_fs_init(void) > +{ > + return ima_securityfs_init(&init_user_ns); > +} > -- > 2.31.1 > >
On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote: > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote: > > Move the dentries into the ima_namespace for reuse by virtualized > > SecurityFS. Implement function freeing the dentries in order of > > files and symlinks before directories. > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > --- > > This doesn't work as implemented, I think. > > What I would have preferred and what I tried to explain in the earlier > review was: > Keep the dentry stashing global since it is only needed for init_ima_ns. > Then struct ima_namespace becomes way smaller and simpler. > If you do that then it makes sense to remove the additional dget() in > securityfs_create_dentry() for non-init_ima_ns. > Then you can rely on auto-cleanup in .kill_sb() or on > ima_securityfs_init() failure and you only need to call > ima_fs_ns_free_dentries() if ns != init_ima_ns. > > IIuc, it seems you're currently doing one dput() too many since you're > calling securityfs_remove() in the error path for non-init_ima_ns which > relies on the previous increased dget() which we removed. If you really want to move the dentry stashing into struct ima_namespace even though it's really unnecessary then you may as well not care about the auto-cleanup and keep that additional ima_fs_ns_free_dentries(ns) call in .kill_sb(). But I really think not dragging dentry stashing into struct ima_namespace is the correct way to go about this. > > > include/linux/ima.h | 13 ++++++ > > security/integrity/ima/ima_fs.c | 72 ++++++++++++++++++--------------- > > 2 files changed, 52 insertions(+), 33 deletions(-) > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h > > index 3aaf6e806db4..4dd64e318b15 100644 > > --- a/include/linux/ima.h > > +++ b/include/linux/ima.h > > @@ -220,6 +220,17 @@ struct ima_h_table { > > struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE]; > > }; > > > > +enum { > > + IMAFS_DENTRY_DIR = 0, > > + 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 +277,8 @@ struct ima_namespace { > > struct mutex ima_write_mutex; > > unsigned long ima_fs_flags; > > int valid_policy; > > + > > + struct dentry *dentry[IMAFS_DENTRY_LAST]; > > }; > > > > extern struct ima_namespace init_ima_ns; > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c > > index a749a3e79304..3810d11fb463 100644 > > --- a/security/integrity/ima/ima_fs.c > > +++ b/security/integrity/ima/ima_fs.c > > @@ -360,14 +360,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,8 +429,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) > > - securityfs_remove(ima_policy); > > - ima_policy = NULL; > > + securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]); > > + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL; > > #elif defined(CONFIG_IMA_WRITE_POLICY) > > clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags); > > #elif defined(CONFIG_IMA_READ_POLICY) > > @@ -455,58 +447,72 @@ static const struct file_operations ima_measure_policy_ops = { > > .llseek = generic_file_llseek, > > }; > > > > -int __init ima_fs_init(void) > > +static void ima_fs_ns_free_dentries(struct ima_namespace *ns) > > { > > - ima_dir = securityfs_create_dir("ima", integrity_dir); > > - if (IS_ERR(ima_dir)) > > + int i; > > + > > + for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--) > > + securityfs_remove(ns->dentry[i]); > > + > > + memset(ns->dentry, 0, sizeof(ns->dentry)); > > +} > > + > > +static int __init ima_securityfs_init(struct user_namespace *user_ns) > > +{ > > + struct ima_namespace *ns = user_ns->ima_ns; > > + struct dentry *ima_dir; > > + > > + ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_dir); > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR])) > > return -1; > > + ima_dir = ns->dentry[IMAFS_DENTRY_DIR]; > > > > - ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima", > > - NULL); > > - if (IS_ERR(ima_symlink)) > > + ns->dentry[IMAFS_DENTRY_SYMLINK] = > > + securityfs_create_symlink("ima", NULL, "integrity/ima", NULL); > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK])) > > goto out; > > > > - binary_runtime_measurements = > > + ns->dentry[IMAFS_DENTRY_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)) > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS])) > > goto out; > > > > - ascii_runtime_measurements = > > + ns->dentry[IMAFS_DENTRY_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)) > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS])) > > goto out; > > > > - runtime_measurements_count = > > + ns->dentry[IMAFS_DENTRY_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)) > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT])) > > goto out; > > > > - violations = > > + ns->dentry[IMAFS_DENTRY_VIOLATIONS] = > > securityfs_create_file("violations", S_IRUSR | S_IRGRP, > > ima_dir, NULL, &ima_htable_violations_ops); > > - if (IS_ERR(violations)) > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS])) > > goto out; > > > > - ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS, > > + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = > > + securityfs_create_file("policy", POLICY_FILE_FLAGS, > > ima_dir, NULL, > > &ima_measure_policy_ops); > > - if (IS_ERR(ima_policy)) > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY])) > > goto out; > > > > 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); > > + ima_fs_ns_free_dentries(ns); > > return -1; > > } > > + > > +int __init ima_fs_init(void) > > +{ > > + return ima_securityfs_init(&init_user_ns); > > +} > > -- > > 2.31.1 > > > > >
On Thu, Dec 09, 2021 at 03:37:49PM +0100, Christian Brauner wrote: > On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote: > > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote: > > > Move the dentries into the ima_namespace for reuse by virtualized > > > SecurityFS. Implement function freeing the dentries in order of > > > files and symlinks before directories. > > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > > --- > > > > This doesn't work as implemented, I think. > > > > What I would have preferred and what I tried to explain in the earlier > > review was: > > Keep the dentry stashing global since it is only needed for init_ima_ns. > > Then struct ima_namespace becomes way smaller and simpler. > > If you do that then it makes sense to remove the additional dget() in > > securityfs_create_dentry() for non-init_ima_ns. > > Then you can rely on auto-cleanup in .kill_sb() or on > > ima_securityfs_init() failure and you only need to call > > ima_fs_ns_free_dentries() if ns != init_ima_ns. s/ns != init_ima_ns/ns == init_ima_ns/ > > > > IIuc, it seems you're currently doing one dput() too many since you're > > calling securityfs_remove() in the error path for non-init_ima_ns which > > relies on the previous increased dget() which we removed. > > If you really want to move the dentry stashing into struct ima_namespace > even though it's really unnecessary then you may as well not care about > the auto-cleanup and keep that additional ima_fs_ns_free_dentries(ns) > call in .kill_sb(). But I really think not dragging dentry stashing into > struct ima_namespace is the correct way to go about this. > > > > > > include/linux/ima.h | 13 ++++++ > > > security/integrity/ima/ima_fs.c | 72 ++++++++++++++++++--------------- > > > 2 files changed, 52 insertions(+), 33 deletions(-) > > > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h > > > index 3aaf6e806db4..4dd64e318b15 100644 > > > --- a/include/linux/ima.h > > > +++ b/include/linux/ima.h > > > @@ -220,6 +220,17 @@ struct ima_h_table { > > > struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE]; > > > }; > > > > > > +enum { > > > + IMAFS_DENTRY_DIR = 0, > > > + 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 +277,8 @@ struct ima_namespace { > > > struct mutex ima_write_mutex; > > > unsigned long ima_fs_flags; > > > int valid_policy; > > > + > > > + struct dentry *dentry[IMAFS_DENTRY_LAST]; > > > }; > > > > > > extern struct ima_namespace init_ima_ns; > > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c > > > index a749a3e79304..3810d11fb463 100644 > > > --- a/security/integrity/ima/ima_fs.c > > > +++ b/security/integrity/ima/ima_fs.c > > > @@ -360,14 +360,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,8 +429,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) > > > - securityfs_remove(ima_policy); > > > - ima_policy = NULL; > > > + securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]); > > > + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL; > > > #elif defined(CONFIG_IMA_WRITE_POLICY) > > > clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags); > > > #elif defined(CONFIG_IMA_READ_POLICY) > > > @@ -455,58 +447,72 @@ static const struct file_operations ima_measure_policy_ops = { > > > .llseek = generic_file_llseek, > > > }; > > > > > > -int __init ima_fs_init(void) > > > +static void ima_fs_ns_free_dentries(struct ima_namespace *ns) > > > { > > > - ima_dir = securityfs_create_dir("ima", integrity_dir); > > > - if (IS_ERR(ima_dir)) > > > + int i; > > > + > > > + for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--) > > > + securityfs_remove(ns->dentry[i]); > > > + > > > + memset(ns->dentry, 0, sizeof(ns->dentry)); > > > +} > > > + > > > +static int __init ima_securityfs_init(struct user_namespace *user_ns) > > > +{ > > > + struct ima_namespace *ns = user_ns->ima_ns; > > > + struct dentry *ima_dir; > > > + > > > + ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_dir); > > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR])) > > > return -1; > > > + ima_dir = ns->dentry[IMAFS_DENTRY_DIR]; > > > > > > - ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima", > > > - NULL); > > > - if (IS_ERR(ima_symlink)) > > > + ns->dentry[IMAFS_DENTRY_SYMLINK] = > > > + securityfs_create_symlink("ima", NULL, "integrity/ima", NULL); > > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK])) > > > goto out; > > > > > > - binary_runtime_measurements = > > > + ns->dentry[IMAFS_DENTRY_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)) > > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS])) > > > goto out; > > > > > > - ascii_runtime_measurements = > > > + ns->dentry[IMAFS_DENTRY_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)) > > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS])) > > > goto out; > > > > > > - runtime_measurements_count = > > > + ns->dentry[IMAFS_DENTRY_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)) > > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT])) > > > goto out; > > > > > > - violations = > > > + ns->dentry[IMAFS_DENTRY_VIOLATIONS] = > > > securityfs_create_file("violations", S_IRUSR | S_IRGRP, > > > ima_dir, NULL, &ima_htable_violations_ops); > > > - if (IS_ERR(violations)) > > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS])) > > > goto out; > > > > > > - ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS, > > > + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = > > > + securityfs_create_file("policy", POLICY_FILE_FLAGS, > > > ima_dir, NULL, > > > &ima_measure_policy_ops); > > > - if (IS_ERR(ima_policy)) > > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY])) > > > goto out; > > > > > > 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); > > > + ima_fs_ns_free_dentries(ns); > > > return -1; > > > } > > > + > > > +int __init ima_fs_init(void) > > > +{ > > > + return ima_securityfs_init(&init_user_ns); > > > +} > > > -- > > > 2.31.1 > > > > > > > > >
On 12/9/21 09:41, Christian Brauner wrote: > On Thu, Dec 09, 2021 at 03:37:49PM +0100, Christian Brauner wrote: >> On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote: >>> On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote: >>>> Move the dentries into the ima_namespace for reuse by virtualized >>>> SecurityFS. Implement function freeing the dentries in order of >>>> files and symlinks before directories. >>>> >>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>> --- >>> This doesn't work as implemented, I think. >>> >>> What I would have preferred and what I tried to explain in the earlier >>> review was: >>> Keep the dentry stashing global since it is only needed for init_ima_ns. >>> Then struct ima_namespace becomes way smaller and simpler. >>> If you do that then it makes sense to remove the additional dget() in >>> securityfs_create_dentry() for non-init_ima_ns. >>> Then you can rely on auto-cleanup in .kill_sb() or on >>> ima_securityfs_init() failure and you only need to call >>> ima_fs_ns_free_dentries() if ns != init_ima_ns. > s/ns != init_ima_ns/ns == init_ima_ns/ > >>> IIuc, it seems you're currently doing one dput() too many since you're >>> calling securityfs_remove() in the error path for non-init_ima_ns which >>> relies on the previous increased dget() which we removed. I thought that securityfs_remove() will now simply influence when a dentry is removed and freed. If we call it in the error cleanup path in non-init_user_ns case it would go away right there and leave nothing to do for .kill_sb() while an additional dget() would require the cleanup as well but do another cleanup then in .kill_sb() since that brings the reference count to 0 via the dput()s that it does. Am I wrong on this? >> If you really want to move the dentry stashing into struct ima_namespace >> even though it's really unnecessary then you may as well not care about >> the auto-cleanup and keep that additional ima_fs_ns_free_dentries(ns) >> call in .kill_sb(). But I really think not dragging dentry stashing into >> struct ima_namespace is the correct way to go about this. I moved the dentries into the ima_namespace so that each namespace holds a pointer to the dentries it owns and isolates them. We certainly wouldn't want to have IMA namespaces write over the current static variables and create a mess with what these are pointing to ( https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L359 ) and possible race conditions when doing parallel initialization (if that's possible at all). This also reduces the code size and we don't need two different implementations for init_user_ns and non-init_user_ns. So I don't quite understand whey we wouldn't want to have the dentries isolated via ima_namespace? >> >>>> include/linux/ima.h | 13 ++++++ >>>> security/integrity/ima/ima_fs.c | 72 ++++++++++++++++++--------------- >>>> 2 files changed, 52 insertions(+), 33 deletions(-) >>>> >>>> diff --git a/include/linux/ima.h b/include/linux/ima.h >>>> index 3aaf6e806db4..4dd64e318b15 100644 >>>> --- a/include/linux/ima.h >>>> +++ b/include/linux/ima.h >>>> @@ -220,6 +220,17 @@ struct ima_h_table { >>>> struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE]; >>>> }; >>>> >>>> +enum { >>>> + IMAFS_DENTRY_DIR = 0, >>>> + 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 +277,8 @@ struct ima_namespace { >>>> struct mutex ima_write_mutex; >>>> unsigned long ima_fs_flags; >>>> int valid_policy; >>>> + >>>> + struct dentry *dentry[IMAFS_DENTRY_LAST]; >>>> }; >>>> >>>> extern struct ima_namespace init_ima_ns; >>>> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c >>>> index a749a3e79304..3810d11fb463 100644 >>>> --- a/security/integrity/ima/ima_fs.c >>>> +++ b/security/integrity/ima/ima_fs.c >>>> @@ -360,14 +360,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,8 +429,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) >>>> - securityfs_remove(ima_policy); >>>> - ima_policy = NULL; >>>> + securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]); >>>> + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL; >>>> #elif defined(CONFIG_IMA_WRITE_POLICY) >>>> clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags); >>>> #elif defined(CONFIG_IMA_READ_POLICY) >>>> @@ -455,58 +447,72 @@ static const struct file_operations ima_measure_policy_ops = { >>>> .llseek = generic_file_llseek, >>>> }; >>>> >>>> -int __init ima_fs_init(void) >>>> +static void ima_fs_ns_free_dentries(struct ima_namespace *ns) >>>> { >>>> - ima_dir = securityfs_create_dir("ima", integrity_dir); >>>> - if (IS_ERR(ima_dir)) >>>> + int i; >>>> + >>>> + for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--) >>>> + securityfs_remove(ns->dentry[i]); >>>> + >>>> + memset(ns->dentry, 0, sizeof(ns->dentry)); >>>> +} >>>> + >>>> +static int __init ima_securityfs_init(struct user_namespace *user_ns) >>>> +{ >>>> + struct ima_namespace *ns = user_ns->ima_ns; >>>> + struct dentry *ima_dir; >>>> + >>>> + ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_dir); >>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR])) >>>> return -1; >>>> + ima_dir = ns->dentry[IMAFS_DENTRY_DIR]; >>>> >>>> - ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima", >>>> - NULL); >>>> - if (IS_ERR(ima_symlink)) >>>> + ns->dentry[IMAFS_DENTRY_SYMLINK] = >>>> + securityfs_create_symlink("ima", NULL, "integrity/ima", NULL); >>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK])) >>>> goto out; >>>> >>>> - binary_runtime_measurements = >>>> + ns->dentry[IMAFS_DENTRY_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)) >>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS])) >>>> goto out; >>>> >>>> - ascii_runtime_measurements = >>>> + ns->dentry[IMAFS_DENTRY_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)) >>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS])) >>>> goto out; >>>> >>>> - runtime_measurements_count = >>>> + ns->dentry[IMAFS_DENTRY_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)) >>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT])) >>>> goto out; >>>> >>>> - violations = >>>> + ns->dentry[IMAFS_DENTRY_VIOLATIONS] = >>>> securityfs_create_file("violations", S_IRUSR | S_IRGRP, >>>> ima_dir, NULL, &ima_htable_violations_ops); >>>> - if (IS_ERR(violations)) >>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS])) >>>> goto out; >>>> >>>> - ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS, >>>> + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = >>>> + securityfs_create_file("policy", POLICY_FILE_FLAGS, >>>> ima_dir, NULL, >>>> &ima_measure_policy_ops); >>>> - if (IS_ERR(ima_policy)) >>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY])) >>>> goto out; >>>> >>>> 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); >>>> + ima_fs_ns_free_dentries(ns); >>>> return -1; >>>> } >>>> + >>>> +int __init ima_fs_init(void) >>>> +{ >>>> + return ima_securityfs_init(&init_user_ns); >>>> +} >>>> -- >>>> 2.31.1 >>>> >>>>
On Thu, 2021-12-09 at 15:37 +0100, Christian Brauner wrote: > On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote: > > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote: > > > Move the dentries into the ima_namespace for reuse by virtualized > > > SecurityFS. Implement function freeing the dentries in order of > > > files and symlinks before directories. > > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > > --- > > > > This doesn't work as implemented, I think. > > > > What I would have preferred and what I tried to explain in the > > earlier review was: > > Keep the dentry stashing global since it is only needed for > > init_ima_ns. > > Then struct ima_namespace becomes way smaller and simpler. > > If you do that then it makes sense to remove the additional dget() > > in securityfs_create_dentry() for non-init_ima_ns. > > Then you can rely on auto-cleanup in .kill_sb() or on > > ima_securityfs_init() failure and you only need to call > > ima_fs_ns_free_dentries() if ns != init_ima_ns. > > > > IIuc, it seems you're currently doing one dput() too many since > > you're calling securityfs_remove() in the error path for non- > > init_ima_ns which relies on the previous increased dget() which we > > removed. > > If you really want to move the dentry stashing into struct > ima_namespace even though it's really unnecessary then you may as > well not care about the auto-cleanup and keep that additional > ima_fs_ns_free_dentries(ns) call in .kill_sb(). But I really think > not dragging dentry stashing into struct ima_namespace is the correct > way to go about this. We, unfortunately, do have one case we can't avoid stashing for the policy file. It's this code in ima_release_policy: > #if !defined(CONFIG_IMA_WRITE_POLICY) && > !defined(CONFIG_IMA_READ_POLICY) > securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]); > ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL; > What it does is that in certain config options, the policy file entry gets removed from the securityfs ima directory after you write to it. James
On Thu, Dec 09, 2021 at 10:00:59AM -0500, Stefan Berger wrote: > > On 12/9/21 09:41, Christian Brauner wrote: > > On Thu, Dec 09, 2021 at 03:37:49PM +0100, Christian Brauner wrote: > > > On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote: > > > > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote: > > > > > Move the dentries into the ima_namespace for reuse by virtualized > > > > > SecurityFS. Implement function freeing the dentries in order of > > > > > files and symlinks before directories. > > > > > > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > > > > --- > > > > This doesn't work as implemented, I think. > > > > > > > > What I would have preferred and what I tried to explain in the earlier > > > > review was: > > > > Keep the dentry stashing global since it is only needed for init_ima_ns. > > > > Then struct ima_namespace becomes way smaller and simpler. > > > > If you do that then it makes sense to remove the additional dget() in > > > > securityfs_create_dentry() for non-init_ima_ns. > > > > Then you can rely on auto-cleanup in .kill_sb() or on > > > > ima_securityfs_init() failure and you only need to call > > > > ima_fs_ns_free_dentries() if ns != init_ima_ns. > > s/ns != init_ima_ns/ns == init_ima_ns/ > > > > > > IIuc, it seems you're currently doing one dput() too many since you're > > > > calling securityfs_remove() in the error path for non-init_ima_ns which > > > > relies on the previous increased dget() which we removed. > > I thought that securityfs_remove() will now simply influence when a dentry > is removed and freed. If we call it in the error cleanup path in > non-init_user_ns case it would go away right there and leave nothing to do > for .kill_sb() while an additional dget() would require the cleanup as well > but do another cleanup then in .kill_sb() since that brings the reference > count to 0 via the dput()s that it does. Am I wrong on this? With your change you get one dget() from lookup_one_len() in securityfs_create_dentry() for non-init_ima_ns. That's added to the dcache via d_instantiate(). If you call securityfs_dentry_remove() in the error path or anywhere else it does: dir = d_inode(dentry->d_parent); inode_lock(dir); if (simple_positive(dentry)) { if (d_is_dir(dentry)) simple_rmdir(dir, dentry); else simple_unlink(dir, dentry); dput(dentry); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ That dput() right there is for the additional dget() in securityfs_create_dentry() but we didn't take that. So the dput() is one too many now since simple_rmdir() and simple_unlink() will have consumed one already. (You should be able to easily see this if you compile with sanitizers on and let your init function fail somewhere in the middle.) (What usually should happen is sm like this: void binderfs_remove_file(struct dentry *dentry) { struct inode *parent_inode; parent_inode = d_inode(dentry->d_parent); inode_lock(parent_inode); if (simple_positive(dentry)) { dget(dentry); simple_unlink(parent_inode, dentry); d_delete(dentry); dput(dentry); } inode_unlock(parent_inode); }) > > > > > If you really want to move the dentry stashing into struct ima_namespace > > > even though it's really unnecessary then you may as well not care about > > > the auto-cleanup and keep that additional ima_fs_ns_free_dentries(ns) > > > call in .kill_sb(). But I really think not dragging dentry stashing into > > > struct ima_namespace is the correct way to go about this. > > > I moved the dentries into the ima_namespace so that each namespace holds a > pointer to the dentries it owns and isolates them. We certainly wouldn't > want to have IMA namespaces write over the current static variables and > create a mess with what these are pointing to ( https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L359 > ) and possible race conditions when doing parallel initialization (if that's > possible at all). This also reduces the code size and we don't need two > different implementations for init_user_ns and non-init_user_ns. So I don't > quite understand whey we wouldn't want to have the dentries isolated via > ima_namespace? My point was this: Afaict, nowhere in ima are the stashed dentries needed apart from ima_policy_release() which is the .release method of the file_operations where the policy dentry is removed. The dentries only exist because for pre-namespaced ima if you created a dentry going through securityfs_create_dentry() you're pinning the super_block of init-securityfs via simple_pin_fs(). That obliges you to call securityfs_remove() later on in order to call simple_unpin_fs() for all dentries. But for namespaced-ima with namespaced-securityfs there is no more call to simple_{pin,unpin}_fs(). Consequently you don't need to stash the dentries anywhere to have them available for removal later on. They will be automatically cleaned up during .kill_sb(). The one exception I was unaware of reading the code before is in ima_policy_release(). So apologies, I didn't see that. There you remove: static int ima_release_policy(struct inode *inode, struct file *file) { 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 (ns->valid_policy && ima_check_policy(ns) < 0) { cause = "failed"; ns->valid_policy = 0; } pr_info("policy update %s\n", cause); integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL, "policy_update", cause, !ns->valid_policy, 0); if (!ns->valid_policy) { ima_delete_rules(ns); ns->valid_policy = 1; clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags); return 0; } ima_update_policy(ns); #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY) securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]); ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL; ^^^^^^^^^^^^^^^^^^^^^^^^^ But even so, why then stash all those dentries if the only dentry that you ever remove while ima is active - and ima isn't a module so can't be unloaded - is the IMAFS_DENTRY_IMA_POLICY. Simply stash the single dentry in struct ima_namespace and forget about all the other ones and avoid wasting memory. But maybe I'm misunderstanding something. I'm going to get my booster shot and hopefully I'll be able to work tomorrow and later today but I wouldn't bet on it. Christian
On Thu, 2021-12-09 at 10:30 -0500, James Bottomley wrote: > On Thu, 2021-12-09 at 15:37 +0100, Christian Brauner wrote: > > On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote: > > > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote: > > > > Move the dentries into the ima_namespace for reuse by > > > > virtualized > > > > SecurityFS. Implement function freeing the dentries in order of > > > > files and symlinks before directories. > > > > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > > > --- > > > > > > This doesn't work as implemented, I think. > > > > > > What I would have preferred and what I tried to explain in the > > > earlier review was: > > > Keep the dentry stashing global since it is only needed for > > > init_ima_ns. > > > Then struct ima_namespace becomes way smaller and simpler. > > > If you do that then it makes sense to remove the additional > > > dget() in securityfs_create_dentry() for non-init_ima_ns. > > > Then you can rely on auto-cleanup in .kill_sb() or on > > > ima_securityfs_init() failure and you only need to call > > > ima_fs_ns_free_dentries() if ns != init_ima_ns. > > > > > > IIuc, it seems you're currently doing one dput() too many since > > > you're calling securityfs_remove() in the error path for non- > > > init_ima_ns which relies on the previous increased dget() which > > > we removed. > > > > If you really want to move the dentry stashing into struct > > ima_namespace even though it's really unnecessary then you may as > > well not care about the auto-cleanup and keep that additional > > ima_fs_ns_free_dentries(ns) call in .kill_sb(). But I really think > > not dragging dentry stashing into struct ima_namespace is the > > correct way to go about this. > > We, unfortunately, do have one case we can't avoid stashing for the > policy file. It's this code in ima_release_policy: > > > #if !defined(CONFIG_IMA_WRITE_POLICY) && > > !defined(CONFIG_IMA_READ_POLICY) > > securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]); > > ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL; > > > > What it does is that in certain config options, the policy file entry > gets removed from the securityfs ima directory after you write to it. This is what I have incremental to v5 that corrects all of this. It actually keeps every dentry reference (including init_user_ns ones) at 1 so they can be reaped on unmount. For the remove case it does d_delete and then puts the only reference. This means securityfs_remove() works for the namespaced policy file as well. I also got rid of the spurious initialized check in ima_securityfs_init because it prevents you doing a mount;umount;mount on securityfs within a namespace. There's still the problem that if you write the policy, making the file disappear then unmount and remount securityfs it will come back. My guess for fixing this is that we only stash the policy file reference, create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or something and refuse to create it for that value. James --- From 7de285a81ff06b6e0eb2c6db24810aeef9f6dd17 Mon Sep 17 00:00:00 2001 From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: Thu, 9 Dec 2021 19:33:49 +0000 Subject: [PATCH] fix dentry ref counting --- security/inode.c | 12 ++---------- security/integrity/ima/ima_fs.c | 4 ---- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/security/inode.c b/security/inode.c index eaccba7017d9..b53152f7a625 100644 --- a/security/inode.c +++ b/security/inode.c @@ -178,8 +178,6 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode, inode->i_fop = fops; } d_instantiate(dentry, inode); - if (ns == &init_user_ns) - dget(dentry); inode_unlock(dir); return dentry; @@ -317,21 +315,15 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink); void securityfs_remove(struct dentry *dentry) { struct user_namespace *ns = dentry->d_sb->s_user_ns; - struct inode *dir; if (!dentry || IS_ERR(dentry)) return; - dir = d_inode(dentry->d_parent); - inode_lock(dir); if (simple_positive(dentry)) { - if (d_is_dir(dentry)) - simple_rmdir(dir, dentry); - else - simple_unlink(dir, dentry); + d_delete(dentry); dput(dentry); } - inode_unlock(dir); + if (ns == &init_user_ns) simple_release_fs(&init_securityfs_mount, &init_securityfs_mount_count); diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index 778983fd9a73..077a6ff46858 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -466,10 +466,6 @@ int ima_securityfs_init(struct user_namespace *user_ns, struct dentry *root) struct ima_namespace *ns = user_ns->ima_ns; struct dentry *ima_dir; - /* already initialized? */ - if (ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR]) - return 0; - /* FIXME: update when evm and integrity are namespaced */ if (user_ns != &init_user_ns) { ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] =
On 12/9/21 14:38, James Bottomley wrote: > On Thu, 2021-12-09 at 10:30 -0500, James Bottomley wrote: >> On Thu, 2021-12-09 at 15:37 +0100, Christian Brauner wrote: >>> On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote: >>>> On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote: >>>>> Move the dentries into the ima_namespace for reuse by >>>>> virtualized >>>>> SecurityFS. Implement function freeing the dentries in order of >>>>> files and symlinks before directories. >>>>> >>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>>> --- >>>> This doesn't work as implemented, I think. >>>> >>>> What I would have preferred and what I tried to explain in the >>>> earlier review was: >>>> Keep the dentry stashing global since it is only needed for >>>> init_ima_ns. >>>> Then struct ima_namespace becomes way smaller and simpler. >>>> If you do that then it makes sense to remove the additional >>>> dget() in securityfs_create_dentry() for non-init_ima_ns. >>>> Then you can rely on auto-cleanup in .kill_sb() or on >>>> ima_securityfs_init() failure and you only need to call >>>> ima_fs_ns_free_dentries() if ns != init_ima_ns. >>>> >>>> IIuc, it seems you're currently doing one dput() too many since >>>> you're calling securityfs_remove() in the error path for non- >>>> init_ima_ns which relies on the previous increased dget() which >>>> we removed. >>> If you really want to move the dentry stashing into struct >>> ima_namespace even though it's really unnecessary then you may as >>> well not care about the auto-cleanup and keep that additional >>> ima_fs_ns_free_dentries(ns) call in .kill_sb(). But I really think >>> not dragging dentry stashing into struct ima_namespace is the >>> correct way to go about this. >> We, unfortunately, do have one case we can't avoid stashing for the >> policy file. It's this code in ima_release_policy: >> >>> #if !defined(CONFIG_IMA_WRITE_POLICY) && >>> !defined(CONFIG_IMA_READ_POLICY) >>> securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]); >>> ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL; >>> >> What it does is that in certain config options, the policy file entry >> gets removed from the securityfs ima directory after you write to it. > This is what I have incremental to v5 that corrects all of this. It > actually keeps every dentry reference (including init_user_ns ones) at > 1 so they can be reaped on unmount. For the remove case it does > d_delete and then puts the only reference. This means > securityfs_remove() works for the namespaced policy file as well. I fixed it now as well but do another dget() on securityfs_removed() for ns != init_user_ns. Ok, I will add what you have below. > > I also got rid of the spurious initialized check in ima_securityfs_init > because it prevents you doing a mount;umount;mount on securityfs within > a namespace. > > There's still the problem that if you write the policy, making the file > disappear then unmount and remount securityfs it will come back. My > guess for fixing this is that we only stash the policy file reference, > create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or > something and refuse to create it for that value. What about boolean to remember this? I just added this. > > James > > --- > > From 7de285a81ff06b6e0eb2c6db24810aeef9f6dd17 Mon Sep 17 00:00:00 2001 > From: James Bottomley <James.Bottomley@HansenPartnership.com> > Date: Thu, 9 Dec 2021 19:33:49 +0000 > Subject: [PATCH] fix dentry ref counting > > --- > security/inode.c | 12 ++---------- > security/integrity/ima/ima_fs.c | 4 ---- > 2 files changed, 2 insertions(+), 14 deletions(-) > > diff --git a/security/inode.c b/security/inode.c > index eaccba7017d9..b53152f7a625 100644 > --- a/security/inode.c > +++ b/security/inode.c > @@ -178,8 +178,6 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode, > inode->i_fop = fops; > } > d_instantiate(dentry, inode); > - if (ns == &init_user_ns) > - dget(dentry); > inode_unlock(dir); > return dentry; > > @@ -317,21 +315,15 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink); > void securityfs_remove(struct dentry *dentry) > { > struct user_namespace *ns = dentry->d_sb->s_user_ns; > - struct inode *dir; > > if (!dentry || IS_ERR(dentry)) > return; > > - dir = d_inode(dentry->d_parent); > - inode_lock(dir); > if (simple_positive(dentry)) { > - if (d_is_dir(dentry)) > - simple_rmdir(dir, dentry); > - else > - simple_unlink(dir, dentry); > + d_delete(dentry); > dput(dentry); > } > - inode_unlock(dir); > + > if (ns == &init_user_ns) > simple_release_fs(&init_securityfs_mount, > &init_securityfs_mount_count); > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c > index 778983fd9a73..077a6ff46858 100644 > --- a/security/integrity/ima/ima_fs.c > +++ b/security/integrity/ima/ima_fs.c > @@ -466,10 +466,6 @@ int ima_securityfs_init(struct user_namespace *user_ns, struct dentry *root) > struct ima_namespace *ns = user_ns->ima_ns; > struct dentry *ima_dir; > > - /* already initialized? */ > - if (ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR]) > - return 0; > - > /* FIXME: update when evm and integrity are namespaced */ > if (user_ns != &init_user_ns) { > ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] =
On Thu, Dec 09, 2021 at 02:38:13PM -0500, James Bottomley wrote: > On Thu, 2021-12-09 at 10:30 -0500, James Bottomley wrote: > > On Thu, 2021-12-09 at 15:37 +0100, Christian Brauner wrote: > > > On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote: > > > > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote: > > > > > Move the dentries into the ima_namespace for reuse by > > > > > virtualized > > > > > SecurityFS. Implement function freeing the dentries in order of > > > > > files and symlinks before directories. > > > > > > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > > > > --- > > > > > > > > This doesn't work as implemented, I think. > > > > > > > > What I would have preferred and what I tried to explain in the > > > > earlier review was: > > > > Keep the dentry stashing global since it is only needed for > > > > init_ima_ns. > > > > Then struct ima_namespace becomes way smaller and simpler. > > > > If you do that then it makes sense to remove the additional > > > > dget() in securityfs_create_dentry() for non-init_ima_ns. > > > > Then you can rely on auto-cleanup in .kill_sb() or on > > > > ima_securityfs_init() failure and you only need to call > > > > ima_fs_ns_free_dentries() if ns != init_ima_ns. > > > > > > > > IIuc, it seems you're currently doing one dput() too many since > > > > you're calling securityfs_remove() in the error path for non- > > > > init_ima_ns which relies on the previous increased dget() which > > > > we removed. > > > > > > If you really want to move the dentry stashing into struct > > > ima_namespace even though it's really unnecessary then you may as > > > well not care about the auto-cleanup and keep that additional > > > ima_fs_ns_free_dentries(ns) call in .kill_sb(). But I really think > > > not dragging dentry stashing into struct ima_namespace is the > > > correct way to go about this. > > > > We, unfortunately, do have one case we can't avoid stashing for the > > policy file. It's this code in ima_release_policy: > > > > > #if !defined(CONFIG_IMA_WRITE_POLICY) && > > > !defined(CONFIG_IMA_READ_POLICY) > > > securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]); > > > ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL; > > > > > > > What it does is that in certain config options, the policy file entry > > gets removed from the securityfs ima directory after you write to it. > > This is what I have incremental to v5 that corrects all of this. It > actually keeps every dentry reference (including init_user_ns ones) at > 1 so they can be reaped on unmount. For the remove case it does > d_delete and then puts the only reference. This means > securityfs_remove() works for the namespaced policy file as well. > > I also got rid of the spurious initialized check in ima_securityfs_init > because it prevents you doing a mount;umount;mount on securityfs within > a namespace. > > There's still the problem that if you write the policy, making the file > disappear then unmount and remount securityfs it will come back. My > guess for fixing this is that we only stash the policy file reference, > create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or > something and refuse to create it for that value. Some sort of indicator that gets stashed in struct ima_ns that the file does not get recreated on consecutive mounts. That shouldn't be hard to fix. > > James > > --- > > From 7de285a81ff06b6e0eb2c6db24810aeef9f6dd17 Mon Sep 17 00:00:00 2001 > From: James Bottomley <James.Bottomley@HansenPartnership.com> > Date: Thu, 9 Dec 2021 19:33:49 +0000 > Subject: [PATCH] fix dentry ref counting > > --- > security/inode.c | 12 ++---------- > security/integrity/ima/ima_fs.c | 4 ---- > 2 files changed, 2 insertions(+), 14 deletions(-) > > diff --git a/security/inode.c b/security/inode.c > index eaccba7017d9..b53152f7a625 100644 > --- a/security/inode.c > +++ b/security/inode.c > @@ -178,8 +178,6 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode, > inode->i_fop = fops; > } > d_instantiate(dentry, inode); > - if (ns == &init_user_ns) > - dget(dentry); > inode_unlock(dir); > return dentry; > > @@ -317,21 +315,15 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink); > void securityfs_remove(struct dentry *dentry) > { > struct user_namespace *ns = dentry->d_sb->s_user_ns; > - struct inode *dir; > > if (!dentry || IS_ERR(dentry)) > return; > > - dir = d_inode(dentry->d_parent); > - inode_lock(dir); > if (simple_positive(dentry)) { > - if (d_is_dir(dentry)) > - simple_rmdir(dir, dentry); > - else > - simple_unlink(dir, dentry); > + d_delete(dentry); Not, that doesn't work. You can't just call d_delete() and dput() and even if I wouldn't advise it. And you also can't do this without taking the inode lock on the directory. simple_rmdir()/simple_unlink() take care to update various inode fields in the parent dir and handle link counts. This really wants to be sm like struct inode *parent_inode; parent_inode = d_inode(dentry->d_parent); inode_lock(parent_inode); if (simple_positive(dentry)) { dget(dentry); if (d_is_dir(dentry) simple_unlink(parent_inode, dentry); else simple_unlink(parent_inode, dentry); d_delete(dentry); dput(dentry); } inode_unlock(parent_inode); > dput(dentry); > } > - inode_unlock(dir); > + > if (ns == &init_user_ns) > simple_release_fs(&init_securityfs_mount, > &init_securityfs_mount_count); > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c > index 778983fd9a73..077a6ff46858 100644 > --- a/security/integrity/ima/ima_fs.c > +++ b/security/integrity/ima/ima_fs.c > @@ -466,10 +466,6 @@ int ima_securityfs_init(struct user_namespace *user_ns, struct dentry *root) > struct ima_namespace *ns = user_ns->ima_ns; > struct dentry *ima_dir; > > - /* already initialized? */ > - if (ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR]) > - return 0; > - > /* FIXME: update when evm and integrity are namespaced */ > if (user_ns != &init_user_ns) { > ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] = > -- > 2.33.0 > > >
On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: > > There's still the problem that if you write the policy, making the file > > disappear then unmount and remount securityfs it will come back. My > > guess for fixing this is that we only stash the policy file reference, > > create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or > > something and refuse to create it for that value. > > Some sort of indicator that gets stashed in struct ima_ns that the file > does not get recreated on consecutive mounts. That shouldn't be hard to > fix. The policy file disappearing is for backwards compatibility, prior to being able to extend the custom policy. For embedded usecases, allowing the policy to be written exactly once might makes sense. Do we really want/need to continue to support removing the policy in namespaces? thanks, Mimi
On 12/10/21 07:09, Mimi Zohar wrote: > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: >>> There's still the problem that if you write the policy, making the file >>> disappear then unmount and remount securityfs it will come back. My >>> guess for fixing this is that we only stash the policy file reference, >>> create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or >>> something and refuse to create it for that value. >> Some sort of indicator that gets stashed in struct ima_ns that the file >> does not get recreated on consecutive mounts. That shouldn't be hard to >> fix. > The policy file disappearing is for backwards compatibility, prior to > being able to extend the custom policy. For embedded usecases, > allowing the policy to be written exactly once might makes sense. Do > we really want/need to continue to support removing the policy in > namespaces? I don't have an answer but should the behavior for the same #define in this case be different for host and namespaces? Or should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when IMA_NS is selected? > > thanks, > > Mimi >
On Fri, 2021-12-10 at 07:09 -0500, Mimi Zohar wrote: > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: > > > There's still the problem that if you write the policy, making > > > the file disappear then unmount and remount securityfs it will > > > come back. My guess for fixing this is that we only stash the > > > policy file reference, create it if NULL but then set the pointer > > > to PTR_ERR(-EINVAL) or something and refuse to create it for that > > > value. > > > > Some sort of indicator that gets stashed in struct ima_ns that the > > file does not get recreated on consecutive mounts. That shouldn't > > be hard to fix. Yes, Stefan said he was doing that. > The policy file disappearing is for backwards compatibility, prior to > being able to extend the custom policy. For embedded usecases, > allowing the policy to be written exactly once might makes sense. Do > we really want/need to continue to support removing the policy in > namespaces? The embedded world tends also to be a big consumer of namespaces, so if this semantic is for them, likely it should remain in the namespaced IMA. But how necessary is the semantic? If we got rid of it from the whole of IMA, what would break? If we can't think of anything it could likely be removed from both namespaced and non-namespaced IMA. James
On Fri, 2021-12-10 at 07:40 -0500, James Bottomley wrote: > On Fri, 2021-12-10 at 07:09 -0500, Mimi Zohar wrote: > > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: > > > > There's still the problem that if you write the policy, making > > > > the file disappear then unmount and remount securityfs it will > > > > come back. My guess for fixing this is that we only stash the > > > > policy file reference, create it if NULL but then set the pointer > > > > to PTR_ERR(-EINVAL) or something and refuse to create it for that > > > > value. > > > > > > Some sort of indicator that gets stashed in struct ima_ns that the > > > file does not get recreated on consecutive mounts. That shouldn't > > > be hard to fix. > > Yes, Stefan said he was doing that. > > > The policy file disappearing is for backwards compatibility, prior to > > being able to extend the custom policy. For embedded usecases, > > allowing the policy to be written exactly once might makes sense. Do > > we really want/need to continue to support removing the policy in > > namespaces? > > The embedded world tends also to be a big consumer of namespaces, so if > this semantic is for them, likely it should remain in the namespaced > IMA. Think of a simple device that loads a custom IMA policy, which never changes once loaded. > > But how necessary is the semantic? If we got rid of it from the whole > of IMA, what would break? If we can't think of anything it could likely > be removed from both namespaced and non-namespaced IMA. The question isn't an issue of "breaking", but of leaking info. If this isn't a real concern, then the ability of removing the securityfs isn't needed. thanks, Mimi
On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote: > On 12/10/21 07:09, Mimi Zohar wrote: > > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: > >>> There's still the problem that if you write the policy, making the file > >>> disappear then unmount and remount securityfs it will come back. My > >>> guess for fixing this is that we only stash the policy file reference, > >>> create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or > >>> something and refuse to create it for that value. > >> Some sort of indicator that gets stashed in struct ima_ns that the file > >> does not get recreated on consecutive mounts. That shouldn't be hard to > >> fix. > > The policy file disappearing is for backwards compatibility, prior to > > being able to extend the custom policy. For embedded usecases, > > allowing the policy to be written exactly once might makes sense. Do > > we really want/need to continue to support removing the policy in > > namespaces? > > I don't have an answer but should the behavior for the same #define in > this case be different for host and namespaces? Or should we just > 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when IMA_NS is selected? The latter option sounds good. Being able to analyze the namespace policy is really important. thanks, Mimi
On 12/10/21 08:02, Mimi Zohar wrote: > On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote: >> On 12/10/21 07:09, Mimi Zohar wrote: >>> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: >>>>> There's still the problem that if you write the policy, making the file >>>>> disappear then unmount and remount securityfs it will come back. My >>>>> guess for fixing this is that we only stash the policy file reference, >>>>> create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or >>>>> something and refuse to create it for that value. >>>> Some sort of indicator that gets stashed in struct ima_ns that the file >>>> does not get recreated on consecutive mounts. That shouldn't be hard to >>>> fix. >>> The policy file disappearing is for backwards compatibility, prior to >>> being able to extend the custom policy. For embedded usecases, >>> allowing the policy to be written exactly once might makes sense. Do >>> we really want/need to continue to support removing the policy in >>> namespaces? >> I don't have an answer but should the behavior for the same #define in >> this case be different for host and namespaces? Or should we just >> 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when IMA_NS is selected? > The latter option sounds good. Being able to analyze the namespace > policy is really important. Ok, I will adjust the Kconfig for this then. This then warrants the question whether to move the dentry into the ima_namespace. The current code looks like this. #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY) securityfs_remove(ns->policy_dentry); ns->policy_dentry = NULL; ns->policy_dentry_removed = true; #elif defined(CONFIG_IMA_WRITE_POLICY) With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above wouldn't be necessary anymore but I find it 'cleaner' to still have the dentry isolated rather than it being a global static as it was before... > > thanks, > > Mimi >
On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote: > On 12/10/21 08:02, Mimi Zohar wrote: > > On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote: > > > On 12/10/21 07:09, Mimi Zohar wrote: > > > > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: > > > > > > There's still the problem that if you write the policy, > > > > > > making the file disappear then unmount and remount > > > > > > securityfs it will come back. My guess for fixing this is > > > > > > that we only stash the policy file reference, > > > > > > create it if NULL but then set the pointer to PTR_ERR(- > > > > > > EINVAL) or something and refuse to create it for that > > > > > > value. > > > > > Some sort of indicator that gets stashed in struct ima_ns > > > > > that the file does not get recreated on consecutive mounts. > > > > > That shouldn't be hard to fix. > > > > The policy file disappearing is for backwards compatibility, > > > > prior to being able to extend the custom policy. For embedded > > > > usecases, allowing the policy to be written exactly once might > > > > makes sense. Do we really want/need to continue to support > > > > removing the policy in namespaces? > > > I don't have an answer but should the behavior for the same > > > #define in this case be different for host and namespaces? Or > > > should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when > > > IMA_NS is selected? > > The latter option sounds good. Being able to analyze the namespace > > policy is really important. > > Ok, I will adjust the Kconfig for this then. This then warrants the > question whether to move the dentry into the ima_namespace. The > current code looks like this. > > #if !defined(CONFIG_IMA_WRITE_POLICY) && > !defined(CONFIG_IMA_READ_POLICY) > securityfs_remove(ns->policy_dentry); > ns->policy_dentry = NULL; > ns->policy_dentry_removed = true; > #elif defined(CONFIG_IMA_WRITE_POLICY) > > With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above > wouldn't be necessary anymore but I find it 'cleaner' to still have > the dentry isolated rather than it being a global static as it was > before... This is really, really why you don't want the semantics inside the namespace to differ from those outside, because it creates confusion for the people reading the code, especially with magically forced config options like this. I'd strongly suggest you either keep the semantic in the namespace or eliminate it entirely. If you really, really have to make the namespace behave differently, then use global variables and put a big comment on that code saying it can never be reached once CONFIG_IMA_NS is enabled. James
On Fri, 2021-12-10 at 09:26 -0500, James Bottomley wrote: > On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote: > > On 12/10/21 08:02, Mimi Zohar wrote: > > > On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote: > > > > On 12/10/21 07:09, Mimi Zohar wrote: > > > > > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: > > > > > > > There's still the problem that if you write the policy, > > > > > > > making the file disappear then unmount and remount > > > > > > > securityfs it will come back. My guess for fixing this is > > > > > > > that we only stash the policy file reference, > > > > > > > create it if NULL but then set the pointer to PTR_ERR(- > > > > > > > EINVAL) or something and refuse to create it for that > > > > > > > value. > > > > > > Some sort of indicator that gets stashed in struct ima_ns > > > > > > that the file does not get recreated on consecutive mounts. > > > > > > That shouldn't be hard to fix. > > > > > The policy file disappearing is for backwards compatibility, > > > > > prior to being able to extend the custom policy. For embedded > > > > > usecases, allowing the policy to be written exactly once might > > > > > makes sense. Do we really want/need to continue to support > > > > > removing the policy in namespaces? > > > > I don't have an answer but should the behavior for the same > > > > #define in this case be different for host and namespaces? Or > > > > should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when > > > > IMA_NS is selected? > > > The latter option sounds good. Being able to analyze the namespace > > > policy is really important. > > > > Ok, I will adjust the Kconfig for this then. This then warrants the > > question whether to move the dentry into the ima_namespace. The > > current code looks like this. > > > > #if !defined(CONFIG_IMA_WRITE_POLICY) && > > !defined(CONFIG_IMA_READ_POLICY) > > securityfs_remove(ns->policy_dentry); > > ns->policy_dentry = NULL; > > ns->policy_dentry_removed = true; > > #elif defined(CONFIG_IMA_WRITE_POLICY) > > > > With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above > > wouldn't be necessary anymore but I find it 'cleaner' to still have > > the dentry isolated rather than it being a global static as it was > > before... > > This is really, really why you don't want the semantics inside the > namespace to differ from those outside, because it creates confusion > for the people reading the code, especially with magically forced > config options like this. I'd strongly suggest you either keep the > semantic in the namespace or eliminate it entirely. > > If you really, really have to make the namespace behave differently, > then use global variables and put a big comment on that code saying it > can never be reached once CONFIG_IMA_NS is enabled. The problem seems to be with removing the securityfs policy file. Instead of removing it, just make it inacessible for the "if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)" case. thanks, Mimi
On 12/10/21 10:26, Mimi Zohar wrote: > On Fri, 2021-12-10 at 09:26 -0500, James Bottomley wrote: >> On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote: >>> On 12/10/21 08:02, Mimi Zohar wrote: >>>> On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote: >>>>> On 12/10/21 07:09, Mimi Zohar wrote: >>>>>> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: >>>>>>>> There's still the problem that if you write the policy, >>>>>>>> making the file disappear then unmount and remount >>>>>>>> securityfs it will come back. My guess for fixing this is >>>>>>>> that we only stash the policy file reference, >>>>>>>> create it if NULL but then set the pointer to PTR_ERR(- >>>>>>>> EINVAL) or something and refuse to create it for that >>>>>>>> value. >>>>>>> Some sort of indicator that gets stashed in struct ima_ns >>>>>>> that the file does not get recreated on consecutive mounts. >>>>>>> That shouldn't be hard to fix. >>>>>> The policy file disappearing is for backwards compatibility, >>>>>> prior to being able to extend the custom policy. For embedded >>>>>> usecases, allowing the policy to be written exactly once might >>>>>> makes sense. Do we really want/need to continue to support >>>>>> removing the policy in namespaces? >>>>> I don't have an answer but should the behavior for the same >>>>> #define in this case be different for host and namespaces? Or >>>>> should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when >>>>> IMA_NS is selected? >>>> The latter option sounds good. Being able to analyze the namespace >>>> policy is really important. >>> Ok, I will adjust the Kconfig for this then. This then warrants the >>> question whether to move the dentry into the ima_namespace. The >>> current code looks like this. >>> >>> #if !defined(CONFIG_IMA_WRITE_POLICY) && >>> !defined(CONFIG_IMA_READ_POLICY) >>> securityfs_remove(ns->policy_dentry); >>> ns->policy_dentry = NULL; >>> ns->policy_dentry_removed = true; >>> #elif defined(CONFIG_IMA_WRITE_POLICY) >>> >>> With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above >>> wouldn't be necessary anymore but I find it 'cleaner' to still have >>> the dentry isolated rather than it being a global static as it was >>> before... >> This is really, really why you don't want the semantics inside the >> namespace to differ from those outside, because it creates confusion >> for the people reading the code, especially with magically forced >> config options like this. I'd strongly suggest you either keep the >> semantic in the namespace or eliminate it entirely. >> >> If you really, really have to make the namespace behave differently, >> then use global variables and put a big comment on that code saying it >> can never be reached once CONFIG_IMA_NS is enabled. > The problem seems to be with removing the securityfs policy file. > Instead of removing it, just make it inacessible for the "if > !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)" > case. So we would then leave it up to the one building the kernel to select the proper compile time options (suggested ones being IMA_WRITE_POLICY and IMA_READ_POLICY being enabled?) and behavior of host and IMA namespace is then the same per those options? Removing the file didn't seem the problem to me but more like whether the host should ever behave differently from the namespace. Stefan > > thanks, > > Mimi >
On Fri, 2021-12-10 at 10:32 -0500, Stefan Berger wrote: > On 12/10/21 10:26, Mimi Zohar wrote: > > On Fri, 2021-12-10 at 09:26 -0500, James Bottomley wrote: > >> On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote: > >>> On 12/10/21 08:02, Mimi Zohar wrote: > >>>> On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote: > >>>>> On 12/10/21 07:09, Mimi Zohar wrote: > >>>>>> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: > >>>>>>>> There's still the problem that if you write the policy, > >>>>>>>> making the file disappear then unmount and remount > >>>>>>>> securityfs it will come back. My guess for fixing this is > >>>>>>>> that we only stash the policy file reference, > >>>>>>>> create it if NULL but then set the pointer to PTR_ERR(- > >>>>>>>> EINVAL) or something and refuse to create it for that > >>>>>>>> value. > >>>>>>> Some sort of indicator that gets stashed in struct ima_ns > >>>>>>> that the file does not get recreated on consecutive mounts. > >>>>>>> That shouldn't be hard to fix. > >>>>>> The policy file disappearing is for backwards compatibility, > >>>>>> prior to being able to extend the custom policy. For embedded > >>>>>> usecases, allowing the policy to be written exactly once might > >>>>>> makes sense. Do we really want/need to continue to support > >>>>>> removing the policy in namespaces? > >>>>> I don't have an answer but should the behavior for the same > >>>>> #define in this case be different for host and namespaces? Or > >>>>> should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when > >>>>> IMA_NS is selected? > >>>> The latter option sounds good. Being able to analyze the namespace > >>>> policy is really important. > >>> Ok, I will adjust the Kconfig for this then. This then warrants the > >>> question whether to move the dentry into the ima_namespace. The > >>> current code looks like this. > >>> > >>> #if !defined(CONFIG_IMA_WRITE_POLICY) && > >>> !defined(CONFIG_IMA_READ_POLICY) > >>> securityfs_remove(ns->policy_dentry); > >>> ns->policy_dentry = NULL; > >>> ns->policy_dentry_removed = true; > >>> #elif defined(CONFIG_IMA_WRITE_POLICY) > >>> > >>> With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above > >>> wouldn't be necessary anymore but I find it 'cleaner' to still have > >>> the dentry isolated rather than it being a global static as it was > >>> before... > >> This is really, really why you don't want the semantics inside the > >> namespace to differ from those outside, because it creates confusion > >> for the people reading the code, especially with magically forced > >> config options like this. I'd strongly suggest you either keep the > >> semantic in the namespace or eliminate it entirely. > >> > >> If you really, really have to make the namespace behave differently, > >> then use global variables and put a big comment on that code saying it > >> can never be reached once CONFIG_IMA_NS is enabled. > > The problem seems to be with removing the securityfs policy file. > > Instead of removing it, just make it inacessible for the "if > > !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)" > > case. > > So we would then leave it up to the one building the kernel to select > the proper compile time options (suggested ones being IMA_WRITE_POLICY > and IMA_READ_POLICY being enabled?) and behavior of host and IMA > namespace is then the same per those options? Removing the file didn't > seem the problem to me but more like whether the host should ever behave > differently from the namespace. You proposed "select IMA_WRITE_POLICY and IMA_READ_POLICY'" when IMA_NS is selected. At least IMA_READ_POLICY should be enabled for namespaces. In addition, if removing the securityfs file after a custom policy is loaded complicates namespacing, then don't remove it. thanks, Mimi
On 12/10/21 10:48, Mimi Zohar wrote: > On Fri, 2021-12-10 at 10:32 -0500, Stefan Berger wrote: >> On 12/10/21 10:26, Mimi Zohar wrote: >>> On Fri, 2021-12-10 at 09:26 -0500, James Bottomley wrote: >>>> On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote: >>>>> On 12/10/21 08:02, Mimi Zohar wrote: >>>>>> On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote: >>>>>>> On 12/10/21 07:09, Mimi Zohar wrote: >>>>>>>> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: >>>>>>>>>> There's still the problem that if you write the policy, >>>>>>>>>> making the file disappear then unmount and remount >>>>>>>>>> securityfs it will come back. My guess for fixing this is >>>>>>>>>> that we only stash the policy file reference, >>>>>>>>>> create it if NULL but then set the pointer to PTR_ERR(- >>>>>>>>>> EINVAL) or something and refuse to create it for that >>>>>>>>>> value. >>>>>>>>> Some sort of indicator that gets stashed in struct ima_ns >>>>>>>>> that the file does not get recreated on consecutive mounts. >>>>>>>>> That shouldn't be hard to fix. >>>>>>>> The policy file disappearing is for backwards compatibility, >>>>>>>> prior to being able to extend the custom policy. For embedded >>>>>>>> usecases, allowing the policy to be written exactly once might >>>>>>>> makes sense. Do we really want/need to continue to support >>>>>>>> removing the policy in namespaces? >>>>>>> I don't have an answer but should the behavior for the same >>>>>>> #define in this case be different for host and namespaces? Or >>>>>>> should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when >>>>>>> IMA_NS is selected? >>>>>> The latter option sounds good. Being able to analyze the namespace >>>>>> policy is really important. >>>>> Ok, I will adjust the Kconfig for this then. This then warrants the >>>>> question whether to move the dentry into the ima_namespace. The >>>>> current code looks like this. >>>>> >>>>> #if !defined(CONFIG_IMA_WRITE_POLICY) && >>>>> !defined(CONFIG_IMA_READ_POLICY) >>>>> securityfs_remove(ns->policy_dentry); >>>>> ns->policy_dentry = NULL; >>>>> ns->policy_dentry_removed = true; >>>>> #elif defined(CONFIG_IMA_WRITE_POLICY) >>>>> >>>>> With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above >>>>> wouldn't be necessary anymore but I find it 'cleaner' to still have >>>>> the dentry isolated rather than it being a global static as it was >>>>> before... >>>> This is really, really why you don't want the semantics inside the >>>> namespace to differ from those outside, because it creates confusion >>>> for the people reading the code, especially with magically forced >>>> config options like this. I'd strongly suggest you either keep the >>>> semantic in the namespace or eliminate it entirely. >>>> >>>> If you really, really have to make the namespace behave differently, >>>> then use global variables and put a big comment on that code saying it >>>> can never be reached once CONFIG_IMA_NS is enabled. >>> The problem seems to be with removing the securityfs policy file. >>> Instead of removing it, just make it inacessible for the "if >>> !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)" >>> case. >> So we would then leave it up to the one building the kernel to select >> the proper compile time options (suggested ones being IMA_WRITE_POLICY >> and IMA_READ_POLICY being enabled?) and behavior of host and IMA >> namespace is then the same per those options? Removing the file didn't >> seem the problem to me but more like whether the host should ever behave >> differently from the namespace. > You proposed "select IMA_WRITE_POLICY and IMA_READ_POLICY'" when IMA_NS > is selected. At least IMA_READ_POLICY should be enabled for > namespaces. > > In addition, if removing the securityfs file after a custom policy is > loaded complicates namespacing, then don't remove it. If we just leave the selection of the compile time options to the user (suggested ones being IMA_WRITE_POLICY and IMA_READ_POLICY being enabled) we don't need to make any changes. Choices are for IMA_NS: 1) Leave compile-time options to the user and suggest IMA_WRITE_POLICY and IMA_READ_POLICY 2) Auto-select IMA_WRITE_POLICY and IMA_READ_POLICY for IMA_NS and still remove file for non-IMA_NS case and have the dentry in the ima_namespace 3) Auto-select IMA_WRITE_POLICY and IMA_READ_POLICY for IMA_NS and still remove file for non-IMA_NS case and use global dentry 4) Changing mode bits on the file/inode to avoid having the dentry for v6 I just leave things as they are and we can then see what we need. > > thanks, > > Mimi >
On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: > On Thu, Dec 09, 2021 at 02:38:13PM -0500, James Bottomley wrote: [...] > > @@ -317,21 +315,15 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink); > > void securityfs_remove(struct dentry *dentry) > > { > > struct user_namespace *ns = dentry->d_sb->s_user_ns; > > - struct inode *dir; > > > > if (!dentry || IS_ERR(dentry)) > > return; > > > > - dir = d_inode(dentry->d_parent); > > - inode_lock(dir); > > if (simple_positive(dentry)) { > > - if (d_is_dir(dentry)) > > - simple_rmdir(dir, dentry); > > - else > > - simple_unlink(dir, dentry); > > + d_delete(dentry); > > Not, that doesn't work. You can't just call d_delete() and dput() and > even if I wouldn't advise it. And you also can't do this without > taking the inode lock on the directory. Agreed on that > simple_rmdir()/simple_unlink() take care to update various inode > fields in the parent dir and handle link counts. This really wants to > be sm like > > struct inode *parent_inode; > > parent_inode = d_inode(dentry->d_parent); > inode_lock(parent_inode); > if (simple_positive(dentry)) { > dget(dentry); > if (d_is_dir(dentry) > simple_unlink(parent_inode, dentry); > else > simple_unlink(parent_inode, dentry); > d_delete(dentry); > dput(dentry); > } > inode_unlock(parent_inode); It just slightly annoys me how the simple_ functions change fields in an inode that is about to be evicted ... it seems redundant; plus we shouldn't care if the object we're deleting is a directory or file. I also don't think we need the additional dget because the only consumer is policy file removal and the opener of that file will have done a dget. The inode lock now prevents us racing with another remove in the case of two simultaneous writes. How about struct inode *parent_inode; parent_inode = d_inode(dentry->d_parent); inode_lock(parent_inode); if (simple_positive(dentry)) { drop_nlink(parent_inode); d_delete(dentry); dput(dentry); } inode_unlock(parent_inode); James
On Sun, Dec 12, 2021 at 09:13:12AM -0500, James Bottomley wrote: > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: > > On Thu, Dec 09, 2021 at 02:38:13PM -0500, James Bottomley wrote: > [...] > > > @@ -317,21 +315,15 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink); > > > void securityfs_remove(struct dentry *dentry) > > > { > > > struct user_namespace *ns = dentry->d_sb->s_user_ns; > > > - struct inode *dir; > > > > > > if (!dentry || IS_ERR(dentry)) > > > return; > > > > > > - dir = d_inode(dentry->d_parent); > > > - inode_lock(dir); > > > if (simple_positive(dentry)) { > > > - if (d_is_dir(dentry)) > > > - simple_rmdir(dir, dentry); > > > - else > > > - simple_unlink(dir, dentry); > > > + d_delete(dentry); > > > > Not, that doesn't work. You can't just call d_delete() and dput() and > > even if I wouldn't advise it. And you also can't do this without > > taking the inode lock on the directory. > > Agreed on that > > > simple_rmdir()/simple_unlink() take care to update various inode > > fields in the parent dir and handle link counts. This really wants to > > be sm like > > > > struct inode *parent_inode; > > > > parent_inode = d_inode(dentry->d_parent); > > inode_lock(parent_inode); > > if (simple_positive(dentry)) { > > dget(dentry); > > if (d_is_dir(dentry) > > simple_unlink(parent_inode, dentry); > > else > > simple_unlink(parent_inode, dentry); > > d_delete(dentry); > > dput(dentry); > > } > > inode_unlock(parent_inode); > > It just slightly annoys me how the simple_ functions change fields in > an inode that is about to be evicted ... it seems redundant; plus we > shouldn't care if the object we're deleting is a directory or file. I > also don't think we need the additional dget because the only consumer > is policy file removal and the opener of that file will have done a > dget. The inode lock now prevents us racing with another remove in the > case of two simultaneous writes. > > How about > > struct inode *parent_inode; > > parent_inode = d_inode(dentry->d_parent); > inode_lock(parent_inode); > if (simple_positive(dentry)) { > drop_nlink(parent_inode); > d_delete(dentry); > dput(dentry); > } > inode_unlock(parent_inode); It doesn't just change fields in an inode that is about to be evicted. It changes fields in the parent inode. If you're deleting any file or directory your function currently fails to update mtime and ctime for the parent directory. What you're doing below also isn't all that future proof or safe for callers other than ima. Consider a future caller that might want to call securityfs_remove() with .open = first_file() .relase = first_release( securityfs_remove(second_file) ) .open = second_file() If your securityfs_remove() is called from the first file's release function while the second_file is still open and thus holds a reference and won't go way during first_release()'s securityfs_remove() call you have just failed to update relevant inode fields of a file that can still be queried via stat* functions and can be used to create other files below it. In addition, if someone accidently calls your securityfs_remove() on a directory that is not-empty you're effectively deleting the directory without deleting the files in that directory first whereas simple_rmdir() would tell you to go away. If a user later needs an .unlink/.rmdir method for securityfs or allows calls of securityfs_remove() on the same dentry from concurrent locations you need the dget() in securityfs_remove() even if the inode_lock() is exclusive otherwise you can end up doing a dput() too many, iirc. I would recommened to not turn this into a nih exercise for simple vfs functionality. Your version isn't even significantly simpler. The securityfs_remove() function doesn't need to be reinvented. void securityfs_remove(struct dentry *dentry) { struct inode *dir; if (WARN_ON(!dentry || IS_ERR(dentry))) return; dir = d_inode(dentry->d_parent); inode_lock(dir); if (simple_positive(dentry)) { dget(dentry); if (d_is_dir(dentry)) simple_rmdir(dir, dentry); else simple_unlink(dir, dentry); d_delete(dentry); dput(dentry); } inode_unlock(dir); } I'm not claiming or trying to make this the most minimal version of securityfs_remove() that we could possibly get but I'm making it one where we don't have to worry that there's a subtle corner case that'll bite us in the future just because we tried to hand-massage a function that isn't in any hotpath.
diff --git a/include/linux/ima.h b/include/linux/ima.h index 3aaf6e806db4..4dd64e318b15 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -220,6 +220,17 @@ struct ima_h_table { struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE]; }; +enum { + IMAFS_DENTRY_DIR = 0, + 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 +277,8 @@ struct ima_namespace { struct mutex ima_write_mutex; unsigned long ima_fs_flags; int valid_policy; + + struct dentry *dentry[IMAFS_DENTRY_LAST]; }; extern struct ima_namespace init_ima_ns; diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index a749a3e79304..3810d11fb463 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -360,14 +360,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,8 +429,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) - securityfs_remove(ima_policy); - ima_policy = NULL; + securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]); + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL; #elif defined(CONFIG_IMA_WRITE_POLICY) clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags); #elif defined(CONFIG_IMA_READ_POLICY) @@ -455,58 +447,72 @@ static const struct file_operations ima_measure_policy_ops = { .llseek = generic_file_llseek, }; -int __init ima_fs_init(void) +static void ima_fs_ns_free_dentries(struct ima_namespace *ns) { - ima_dir = securityfs_create_dir("ima", integrity_dir); - if (IS_ERR(ima_dir)) + int i; + + for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--) + securityfs_remove(ns->dentry[i]); + + memset(ns->dentry, 0, sizeof(ns->dentry)); +} + +static int __init ima_securityfs_init(struct user_namespace *user_ns) +{ + struct ima_namespace *ns = user_ns->ima_ns; + struct dentry *ima_dir; + + ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_dir); + if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR])) return -1; + ima_dir = ns->dentry[IMAFS_DENTRY_DIR]; - ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima", - NULL); - if (IS_ERR(ima_symlink)) + ns->dentry[IMAFS_DENTRY_SYMLINK] = + securityfs_create_symlink("ima", NULL, "integrity/ima", NULL); + if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK])) goto out; - binary_runtime_measurements = + ns->dentry[IMAFS_DENTRY_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)) + if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS])) goto out; - ascii_runtime_measurements = + ns->dentry[IMAFS_DENTRY_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)) + if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS])) goto out; - runtime_measurements_count = + ns->dentry[IMAFS_DENTRY_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)) + if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT])) goto out; - violations = + ns->dentry[IMAFS_DENTRY_VIOLATIONS] = securityfs_create_file("violations", S_IRUSR | S_IRGRP, ima_dir, NULL, &ima_htable_violations_ops); - if (IS_ERR(violations)) + if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS])) goto out; - ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS, + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = + securityfs_create_file("policy", POLICY_FILE_FLAGS, ima_dir, NULL, &ima_measure_policy_ops); - if (IS_ERR(ima_policy)) + if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY])) goto out; 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); + ima_fs_ns_free_dentries(ns); return -1; } + +int __init ima_fs_init(void) +{ + return ima_securityfs_init(&init_user_ns); +}
Move the dentries into the ima_namespace for reuse by virtualized SecurityFS. Implement function freeing the dentries in order of files and symlinks before directories. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- include/linux/ima.h | 13 ++++++ security/integrity/ima/ima_fs.c | 72 ++++++++++++++++++--------------- 2 files changed, 52 insertions(+), 33 deletions(-)