| Message ID | 20211207202127.1508689-17-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Namespace IMA with audit support in IMA-ns | expand |
On Tue, Dec 07, 2021 at 03:21:27PM -0500, Stefan Berger wrote: > 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. > > 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 is in securityfs_fill_super. > > 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 <stefanb@linux.ibm.com> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > --- > include/linux/ima.h | 17 ++++++++++++++++- > security/inode.c | 12 +++++++++++- > security/integrity/ima/ima_fs.c | 33 ++++++++++++++++++++++++++------- > 3 files changed, 53 insertions(+), 9 deletions(-) > > diff --git a/include/linux/ima.h b/include/linux/ima.h > index bfb978a7f8d5..a8017272d78d 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -66,6 +66,10 @@ static inline const char * const *arch_get_ima_policy(void) > } > #endif > > +extern int ima_fs_ns_init(struct user_namespace *user_ns, > + struct dentry *root); > +extern void ima_fs_ns_free_dentries(struct user_namespace *user_ns); > + > #else > static inline enum hash_algo ima_get_current_hash_algo(void) > { > @@ -154,6 +158,15 @@ static inline int ima_measure_critical_data(const char *event_label, > return -ENOENT; > } > > +static inline int ima_fs_ns_init(struct user_namespace *ns, struct dentry *root) > +{ > + return 0; > +} > + > +static inline void ima_fs_ns_free_dentries(struct user_namespace *user_ns) > +{ > +} > + > #endif /* CONFIG_IMA */ > > #ifndef CONFIG_IMA_KEXEC > @@ -221,7 +234,8 @@ struct ima_h_table { > }; > > enum { > - IMAFS_DENTRY_DIR = 0, > + IMAFS_DENTRY_INTEGRITY_DIR = 0, > + IMAFS_DENTRY_DIR, > IMAFS_DENTRY_SYMLINK, > IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS, > IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS, > @@ -333,6 +347,7 @@ static inline struct ima_namespace *get_current_ns(void) > { > return &init_ima_ns; > } > + > #endif /* CONFIG_IMA_NS */ > > #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING) > diff --git a/security/inode.c b/security/inode.c > index 121ac1874dde..10ee20917f42 100644 > --- a/security/inode.c > +++ b/security/inode.c > @@ -16,6 +16,7 @@ > #include <linux/fs_context.h> > #include <linux/mount.h> > #include <linux/pagemap.h> > +#include <linux/ima.h> > #include <linux/init.h> > #include <linux/namei.h> > #include <linux/security.h> > @@ -41,6 +42,7 @@ static const struct super_operations securityfs_super_operations = { > static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) > { > static const struct tree_descr files[] = {{""}}; > + struct user_namespace *ns = fc->user_ns; > int error; > > error = simple_fill_super(sb, SECURITYFS_MAGIC, files); > @@ -49,7 +51,10 @@ static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) > > sb->s_op = &securityfs_super_operations; > > - return 0; > + if (ns != &init_user_ns) > + error = ima_fs_ns_init(ns, sb->s_root); > + > + return error; > } > > static int securityfs_get_tree(struct fs_context *fc) > @@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct fs_context *fc) > > static void securityfs_kill_super(struct super_block *sb) > { > + struct user_namespace *ns = sb->s_fs_info; > + > + if (ns != &init_user_ns) > + ima_fs_ns_free_dentries(ns); Say securityfs is unmounted. Then all the inodes and dentries become invalid. It's not allowed to hold on to any dentries or inodes after the super_block is shut down. So I just want to be sure that nothing in ima can access these dentries after securityfs is unmounted. To put it another way: why are they stored in struct ima_namespace in the first place? If you don't pin a filesystem when creating files or directories like you do for securityfs in init_ima_ns then you don't need to hold on to them as they will be automatically be wiped during umount.
On Wed, Dec 08, 2021 at 01:58:14PM +0100, Christian Brauner wrote: > On Tue, Dec 07, 2021 at 03:21:27PM -0500, Stefan Berger wrote: > > 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. > > > > 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 is in securityfs_fill_super. > > > > 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 <stefanb@linux.ibm.com> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > --- > > include/linux/ima.h | 17 ++++++++++++++++- > > security/inode.c | 12 +++++++++++- > > security/integrity/ima/ima_fs.c | 33 ++++++++++++++++++++++++++------- > > 3 files changed, 53 insertions(+), 9 deletions(-) > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h > > index bfb978a7f8d5..a8017272d78d 100644 > > --- a/include/linux/ima.h > > +++ b/include/linux/ima.h > > @@ -66,6 +66,10 @@ static inline const char * const *arch_get_ima_policy(void) > > } > > #endif > > > > +extern int ima_fs_ns_init(struct user_namespace *user_ns, > > + struct dentry *root); > > +extern void ima_fs_ns_free_dentries(struct user_namespace *user_ns); > > + > > #else > > static inline enum hash_algo ima_get_current_hash_algo(void) > > { > > @@ -154,6 +158,15 @@ static inline int ima_measure_critical_data(const char *event_label, > > return -ENOENT; > > } > > > > +static inline int ima_fs_ns_init(struct user_namespace *ns, struct dentry *root) > > +{ > > + return 0; > > +} > > + > > +static inline void ima_fs_ns_free_dentries(struct user_namespace *user_ns) > > +{ > > +} > > + > > #endif /* CONFIG_IMA */ > > > > #ifndef CONFIG_IMA_KEXEC > > @@ -221,7 +234,8 @@ struct ima_h_table { > > }; > > > > enum { > > - IMAFS_DENTRY_DIR = 0, > > + IMAFS_DENTRY_INTEGRITY_DIR = 0, > > + IMAFS_DENTRY_DIR, > > IMAFS_DENTRY_SYMLINK, > > IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS, > > IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS, > > @@ -333,6 +347,7 @@ static inline struct ima_namespace *get_current_ns(void) > > { > > return &init_ima_ns; > > } > > + > > #endif /* CONFIG_IMA_NS */ > > > > #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING) > > diff --git a/security/inode.c b/security/inode.c > > index 121ac1874dde..10ee20917f42 100644 > > --- a/security/inode.c > > +++ b/security/inode.c > > @@ -16,6 +16,7 @@ > > #include <linux/fs_context.h> > > #include <linux/mount.h> > > #include <linux/pagemap.h> > > +#include <linux/ima.h> > > #include <linux/init.h> > > #include <linux/namei.h> > > #include <linux/security.h> > > @@ -41,6 +42,7 @@ static const struct super_operations securityfs_super_operations = { > > static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) > > { > > static const struct tree_descr files[] = {{""}}; > > + struct user_namespace *ns = fc->user_ns; > > int error; > > > > error = simple_fill_super(sb, SECURITYFS_MAGIC, files); > > @@ -49,7 +51,10 @@ static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) > > > > sb->s_op = &securityfs_super_operations; > > > > - return 0; > > + if (ns != &init_user_ns) > > + error = ima_fs_ns_init(ns, sb->s_root); > > + > > + return error; > > } > > > > static int securityfs_get_tree(struct fs_context *fc) > > @@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct fs_context *fc) > > > > static void securityfs_kill_super(struct super_block *sb) > > { > > + struct user_namespace *ns = sb->s_fs_info; > > + > > + if (ns != &init_user_ns) > > + ima_fs_ns_free_dentries(ns); > > Say securityfs is unmounted. Then all the inodes and dentries become > invalid. It's not allowed to hold on to any dentries or inodes after the > super_block is shut down. So I just want to be sure that nothing in ima > can access these dentries after securityfs is unmounted. > > To put it another way: why are they stored in struct ima_namespace in > the first place? If you don't pin a filesystem when creating files or > directories like you do for securityfs in init_ima_ns then you don't > need to hold on to them as they will be automatically be wiped during > umount. The way I see it you need to do the following: If securityfs is mounted in a userns and fill_super is called you need to call int ima_fs_ns_init(struct user_namespace *user_ns, (which you really should call ima_securitfs_init()...) and when you create those entries for non-init-securityfs you just need sm like: struct dentry *dentry; /* XXXX useless comment XXXX: * The lookup_one_len() function will always return with an * increased refcount on the dentry that you need to release. */ dentry = lookup_one_len(name, parent, strlen(name)); if (IS_ERR(dentry)) return dentry; /* Return error if the file/dir already exists. */ if (d_really_is_positive(dentry)) { /* * XXXX useless comment XXXX: * Put the reference from lookup_one_len() */ dput(dentry); return ERR_PTR(-EEXIST); } inode = new_inode(dir->i_sb); if (!inode) { error = -ENOMEM; goto out1; } // DO A LOT OF OTHER STUFF d_instantiate(dentry, new_inode); // DON'T CALL dget() again The point is to not increase the refcount again like securityfs_create_dentry() does after d_instantiate which requires you to call securityfs_remove(). That's unnecessary for the non-init_user_ns-securityfs case and then you don't need all that cleanup stuff in kill_super() and can just rely on d_genocide() and the dcache shrinker to do all the required work. Don't hold on to objects that can go away beneath you in any structs. Stashing them in ima_namespace will just make people think that these things can be accessed without any lifetime concerns which is imho an invitation to disaster in the long run.
On Wed, 2021-12-08 at 13:58 +0100, Christian Brauner wrote: > On Tue, Dec 07, 2021 at 03:21:27PM -0500, Stefan Berger wrote: [...] > > @@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct > > fs_context *fc) > > > > static void securityfs_kill_super(struct super_block *sb) > > { > > + struct user_namespace *ns = sb->s_fs_info; > > + > > + if (ns != &init_user_ns) > > + ima_fs_ns_free_dentries(ns); > > Say securityfs is unmounted. Then all the inodes and dentries become > invalid. It's not allowed to hold on to any dentries or inodes after > the super_block is shut down. So I just want to be sure that nothing > in ima can access these dentries after securityfs is unmounted. > > To put it another way: why are they stored in struct ima_namespace in > the first place? If you don't pin a filesystem when creating files or > directories like you do for securityfs in init_ima_ns then you don't > need to hold on to them as they will be automatically be wiped during > umount. For IMA this is true because IMA can't be a module. However, a modular consumer, like the TPM, must be able to remove its entries from a mounted securityfs because the code that serves the operations is going away. In order to do this removal, it needs the dentries somewhere. The current convention seems to be everything has a directory in the top level, so we could call d_genocide() on this directory and not have to worry about storing the dentries underneath, but I think we can't avoid storing the dentry for the top level directory. James
On Wed, Dec 08, 2021 at 09:11:09AM -0500, James Bottomley wrote: > On Wed, 2021-12-08 at 13:58 +0100, Christian Brauner wrote: > > On Tue, Dec 07, 2021 at 03:21:27PM -0500, Stefan Berger wrote: > [...] > > > @@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct > > > fs_context *fc) > > > > > > static void securityfs_kill_super(struct super_block *sb) > > > { > > > + struct user_namespace *ns = sb->s_fs_info; > > > + > > > + if (ns != &init_user_ns) > > > + ima_fs_ns_free_dentries(ns); > > > > Say securityfs is unmounted. Then all the inodes and dentries become > > invalid. It's not allowed to hold on to any dentries or inodes after > > the super_block is shut down. So I just want to be sure that nothing > > in ima can access these dentries after securityfs is unmounted. > > > > To put it another way: why are they stored in struct ima_namespace in > > the first place? If you don't pin a filesystem when creating files or > > directories like you do for securityfs in init_ima_ns then you don't > > need to hold on to them as they will be automatically be wiped during > > umount. > > For IMA this is true because IMA can't be a module. However, a modular This thread is about ima and its stashing of dentries in struct ima_namespace. That things might be different for other consumers is uninteresting for this specific case, I think. > consumer, like the TPM, must be able to remove its entries from a > mounted securityfs because the code that serves the operations is going > away. In order to do this removal, it needs the dentries somewhere. That still doesn't require you to take an additional reference on the dentry per se. Aside from this brings in a whole different and way bigger issue as that requires way more fundamental work since this is about a (pseudo or proper) device. It's not even clear that this should have entries outside of init_user_ns-securityfs. > The current convention seems to be everything has a directory in the > top level, so we could call d_genocide() on this directory and not have > to worry about storing the dentries underneath, but I think we can't > avoid storing the dentry for the top level directory. I have not heard an argument why ima needs to stash these dentries as it doesn't remove them once created until umount.
On Wed, 2021-12-08 at 15:46 +0100, Christian Brauner wrote: > On Wed, Dec 08, 2021 at 09:11:09AM -0500, James Bottomley wrote: > > On Wed, 2021-12-08 at 13:58 +0100, Christian Brauner wrote: > > > On Tue, Dec 07, 2021 at 03:21:27PM -0500, Stefan Berger wrote: > > [...] > > > > @@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct > > > > fs_context *fc) > > > > > > > > static void securityfs_kill_super(struct super_block *sb) > > > > { > > > > + struct user_namespace *ns = sb->s_fs_info; > > > > + > > > > + if (ns != &init_user_ns) > > > > + ima_fs_ns_free_dentries(ns); > > > > > > Say securityfs is unmounted. Then all the inodes and dentries > > > become invalid. It's not allowed to hold on to any dentries or > > > inodes after the super_block is shut down. So I just want to be > > > sure that nothing in ima can access these dentries after > > > securityfs is unmounted. > > > > > > To put it another way: why are they stored in struct > > > ima_namespace in the first place? If you don't pin a filesystem > > > when creating files or directories like you do for securityfs in > > > init_ima_ns then you don't need to hold on to them as they will > > > be automatically be wiped during umount. > > > > For IMA this is true because IMA can't be a module. However, a > > modular > > This thread is about ima and its stashing of dentries in struct > ima_namespace. That things might be different for other consumers is > uninteresting for this specific case, I think. Well, yes, but the patch series also includes namespacing securityfs. We have to get that right for all consumers, including the modular ones. So I think the way it works is we don't need a remove callback in kill_sb() if we don't raise the dentry refcount in create. However, we still need to return the dentry to allow for stashing and we still need to be able to cope with remove being called for the namespaced entries ... for teardown on error in the IMA case and module removal+teardown on error in other cases. > > consumer, like the TPM, must be able to remove its entries from a > > mounted securityfs because the code that serves the operations is > > going away. In order to do this removal, it needs the dentries > > somewhere. > > That still doesn't require you to take an additional reference on the > dentry per se. No, I don't believe it does ... however it does require a stash somewhere. > Aside from this brings in a whole different and way bigger issue as > that requires way more fundamental work since this is about a (pseudo > or proper) device. It's not even clear that this should have entries > outside of init_user_ns-securityfs. adding vTPMs is a use case that people want to support, so I don't want to get to that bit and find it all doesn't work. > > The current convention seems to be everything has a directory in > > the top level, so we could call d_genocide() on this directory and > > not have to worry about storing the dentries underneath, but I > > think we can't avoid storing the dentry for the top level > > directory. > > I have not heard an argument why ima needs to stash these dentries as > it doesn't remove them once created until umount. I'm not saying IMA does, I'm just saying we still need the dentry returned by the API in case a consumer does need to stash. James
On Wed, Dec 08, 2021 at 10:04:55AM -0500, James Bottomley wrote: > On Wed, 2021-12-08 at 15:46 +0100, Christian Brauner wrote: > > On Wed, Dec 08, 2021 at 09:11:09AM -0500, James Bottomley wrote: > > > On Wed, 2021-12-08 at 13:58 +0100, Christian Brauner wrote: > > > > On Tue, Dec 07, 2021 at 03:21:27PM -0500, Stefan Berger wrote: > > > [...] > > > > > @@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct > > > > > fs_context *fc) > > > > > > > > > > static void securityfs_kill_super(struct super_block *sb) > > > > > { > > > > > + struct user_namespace *ns = sb->s_fs_info; > > > > > + > > > > > + if (ns != &init_user_ns) > > > > > + ima_fs_ns_free_dentries(ns); > > > > > > > > Say securityfs is unmounted. Then all the inodes and dentries > > > > become invalid. It's not allowed to hold on to any dentries or > > > > inodes after the super_block is shut down. So I just want to be > > > > sure that nothing in ima can access these dentries after > > > > securityfs is unmounted. > > > > > > > > To put it another way: why are they stored in struct > > > > ima_namespace in the first place? If you don't pin a filesystem > > > > when creating files or directories like you do for securityfs in > > > > init_ima_ns then you don't need to hold on to them as they will > > > > be automatically be wiped during umount. > > > > > > For IMA this is true because IMA can't be a module. However, a > > > modular > > > > This thread is about ima and its stashing of dentries in struct > > ima_namespace. That things might be different for other consumers is > > uninteresting for this specific case, I think. > > Well, yes, but the patch series also includes namespacing securityfs. > We have to get that right for all consumers, including the modular > ones. So I think the way it works is we don't need a remove callback > in kill_sb() if we don't raise the dentry refcount in create. However, > we still need to return the dentry to allow for stashing and we still > need to be able to cope with remove being called for the namespaced > entries ... for teardown on error in the IMA case and module > removal+teardown on error in other cases. This is a two-way street. Securityfs namespacing places requirements on the callers as well. I won't bend generic vfs infrastucture to our will because some users want to remove dentries at random points. It is on the users to make sure that they don't cause UAFs when securityfs is umounted. And that isn't that hard to do. You just need to guard removal in .kill_sb() with a lock against a concurrent securityfs_remove() call that some piece of code might want to issue and make sure that any stashed stuff is properly invalidated. The point is that we don't need all this right now since we only have ima as user. I did not say that it cannot be done I said we don't need to do it for ima. So I feel discussing this point further is deterring the patches more than it helps them.
On 12/8/21 07:58, Christian Brauner wrote: > On Tue, Dec 07, 2021 at 03:21:27PM -0500, Stefan Berger wrote: >> >> #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING) >> diff --git a/security/inode.c b/security/inode.c >> index 121ac1874dde..10ee20917f42 100644 >> --- a/security/inode.c >> +++ b/security/inode.c >> @@ -16,6 +16,7 @@ >> #include <linux/fs_context.h> >> #include <linux/mount.h> >> #include <linux/pagemap.h> >> +#include <linux/ima.h> >> #include <linux/init.h> >> #include <linux/namei.h> >> #include <linux/security.h> >> @@ -41,6 +42,7 @@ static const struct super_operations securityfs_super_operations = { >> static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) >> { >> static const struct tree_descr files[] = {{""}}; >> + struct user_namespace *ns = fc->user_ns; >> int error; >> >> error = simple_fill_super(sb, SECURITYFS_MAGIC, files); >> @@ -49,7 +51,10 @@ static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) >> >> sb->s_op = &securityfs_super_operations; >> >> - return 0; >> + if (ns != &init_user_ns) >> + error = ima_fs_ns_init(ns, sb->s_root); >> + >> + return error; >> } >> >> static int securityfs_get_tree(struct fs_context *fc) >> @@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct fs_context *fc) >> >> static void securityfs_kill_super(struct super_block *sb) >> { >> + struct user_namespace *ns = sb->s_fs_info; >> + >> + if (ns != &init_user_ns) >> + ima_fs_ns_free_dentries(ns); > Say securityfs is unmounted. Then all the inodes and dentries become > invalid. It's not allowed to hold on to any dentries or inodes after the > super_block is shut down. So I just want to be sure that nothing in ima > can access these dentries after securityfs is unmounted. > To put it another way: why are they stored in struct ima_namespace in > the first place? If you don't pin a filesystem when creating files or > directories like you do for securityfs in init_ima_ns then you don't > need to hold on to them as they will be automatically be wiped during > umount. The reason was so that securityfs for init_ima_ns and IMA namespaces could share the code assigning to dentries to keep around and can clean up if an error occurs while creating a dentry. What about this: We keep the dentries in the ima_namespace, modify the code creating the dentries in securityfs_create_dentry() to only take the additional reference in case of init_user_ns (I suppose this is what you suggest) and then keep 'static void ima_fs_ns_free_dentries()' only for removing the dentries for the error case and never call it from securityfs_kill_super()? Would that be acceptable?
On Wed, Dec 08, 2021 at 10:39:48AM -0500, Stefan Berger wrote: > > On 12/8/21 07:58, Christian Brauner wrote: > > On Tue, Dec 07, 2021 at 03:21:27PM -0500, Stefan Berger wrote: > > > #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING) > > > diff --git a/security/inode.c b/security/inode.c > > > index 121ac1874dde..10ee20917f42 100644 > > > --- a/security/inode.c > > > +++ b/security/inode.c > > > @@ -16,6 +16,7 @@ > > > #include <linux/fs_context.h> > > > #include <linux/mount.h> > > > #include <linux/pagemap.h> > > > +#include <linux/ima.h> > > > #include <linux/init.h> > > > #include <linux/namei.h> > > > #include <linux/security.h> > > > @@ -41,6 +42,7 @@ static const struct super_operations securityfs_super_operations = { > > > static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) > > > { > > > static const struct tree_descr files[] = {{""}}; > > > + struct user_namespace *ns = fc->user_ns; > > > int error; > > > error = simple_fill_super(sb, SECURITYFS_MAGIC, files); > > > @@ -49,7 +51,10 @@ static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) > > > sb->s_op = &securityfs_super_operations; > > > - return 0; > > > + if (ns != &init_user_ns) > > > + error = ima_fs_ns_init(ns, sb->s_root); > > > + > > > + return error; > > > } > > > static int securityfs_get_tree(struct fs_context *fc) > > > @@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct fs_context *fc) > > > static void securityfs_kill_super(struct super_block *sb) > > > { > > > + struct user_namespace *ns = sb->s_fs_info; > > > + > > > + if (ns != &init_user_ns) > > > + ima_fs_ns_free_dentries(ns); > > Say securityfs is unmounted. Then all the inodes and dentries become > > invalid. It's not allowed to hold on to any dentries or inodes after the > > super_block is shut down. So I just want to be sure that nothing in ima > > can access these dentries after securityfs is unmounted. > > > To put it another way: why are they stored in struct ima_namespace in > > the first place? If you don't pin a filesystem when creating files or > > directories like you do for securityfs in init_ima_ns then you don't > > need to hold on to them as they will be automatically be wiped during > > umount. > > > The reason was so that securityfs for init_ima_ns and IMA namespaces could > share the code assigning to dentries to keep around and can clean up if an > error occurs while creating a dentry. > > What about this: We keep the dentries in the ima_namespace, modify the code > creating the dentries in securityfs_create_dentry() to only take the > additional reference in case of init_user_ns (I suppose this is what you > suggest) and then keep 'static void ima_fs_ns_free_dentries()' only for > removing the dentries for the error case and never call it from > securityfs_kill_super()? Would that be acceptable? If you create a range of dentries in fill_super post sb->s_root is properly allocated and you fail in the middle you can simply return from fill_super without bothering to clean them up as the vfs will automatically clean those up when the dcache shrinker runs (If you've increased the refcount as these functions do currently you need to decrease it of course.).
diff --git a/include/linux/ima.h b/include/linux/ima.h index bfb978a7f8d5..a8017272d78d 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -66,6 +66,10 @@ static inline const char * const *arch_get_ima_policy(void) } #endif +extern int ima_fs_ns_init(struct user_namespace *user_ns, + struct dentry *root); +extern void ima_fs_ns_free_dentries(struct user_namespace *user_ns); + #else static inline enum hash_algo ima_get_current_hash_algo(void) { @@ -154,6 +158,15 @@ static inline int ima_measure_critical_data(const char *event_label, return -ENOENT; } +static inline int ima_fs_ns_init(struct user_namespace *ns, struct dentry *root) +{ + return 0; +} + +static inline void ima_fs_ns_free_dentries(struct user_namespace *user_ns) +{ +} + #endif /* CONFIG_IMA */ #ifndef CONFIG_IMA_KEXEC @@ -221,7 +234,8 @@ struct ima_h_table { }; enum { - IMAFS_DENTRY_DIR = 0, + IMAFS_DENTRY_INTEGRITY_DIR = 0, + IMAFS_DENTRY_DIR, IMAFS_DENTRY_SYMLINK, IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS, IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS, @@ -333,6 +347,7 @@ static inline struct ima_namespace *get_current_ns(void) { return &init_ima_ns; } + #endif /* CONFIG_IMA_NS */ #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING) diff --git a/security/inode.c b/security/inode.c index 121ac1874dde..10ee20917f42 100644 --- a/security/inode.c +++ b/security/inode.c @@ -16,6 +16,7 @@ #include <linux/fs_context.h> #include <linux/mount.h> #include <linux/pagemap.h> +#include <linux/ima.h> #include <linux/init.h> #include <linux/namei.h> #include <linux/security.h> @@ -41,6 +42,7 @@ static const struct super_operations securityfs_super_operations = { static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) { static const struct tree_descr files[] = {{""}}; + struct user_namespace *ns = fc->user_ns; int error; error = simple_fill_super(sb, SECURITYFS_MAGIC, files); @@ -49,7 +51,10 @@ static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) sb->s_op = &securityfs_super_operations; - return 0; + if (ns != &init_user_ns) + error = ima_fs_ns_init(ns, sb->s_root); + + return error; } static int securityfs_get_tree(struct fs_context *fc) @@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct fs_context *fc) static void securityfs_kill_super(struct super_block *sb) { + struct user_namespace *ns = sb->s_fs_info; + + if (ns != &init_user_ns) + ima_fs_ns_free_dentries(ns); + kill_litter_super(sb); } diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index e2cee1457aaa..ea366c8e68c6 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -447,8 +447,9 @@ static const struct file_operations ima_measure_policy_ops = { .llseek = generic_file_llseek, }; -static void ima_fs_ns_free_dentries(struct ima_namespace *ns) +void ima_fs_ns_free_dentries(struct user_namespace *user_ns) { + struct ima_namespace *ns = user_ns->ima_ns; int i; for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--) @@ -457,18 +458,36 @@ static void ima_fs_ns_free_dentries(struct ima_namespace *ns) memset(ns->dentry, 0, sizeof(ns->dentry)); } -static int __init ima_fs_ns_init(struct user_namespace *user_ns) +int ima_fs_ns_init(struct user_namespace *user_ns, + struct dentry *root) { struct ima_namespace *ns = user_ns->ima_ns; struct dentry *ima_dir; - ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_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] = + securityfs_create_dir("integrity", root); + if (IS_ERR(ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR])) { + ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] = NULL; + return -1; + } + } else + ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] = integrity_dir; + + ns->dentry[IMAFS_DENTRY_DIR] = + securityfs_create_dir("ima", + ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR]); if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR])) - return -1; + goto out; ima_dir = ns->dentry[IMAFS_DENTRY_DIR]; ns->dentry[IMAFS_DENTRY_SYMLINK] = - securityfs_create_symlink("ima", NULL, "integrity/ima", NULL); + securityfs_create_symlink("ima", root, "integrity/ima", NULL); if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK])) goto out; @@ -508,11 +527,11 @@ static int __init ima_fs_ns_init(struct user_namespace *user_ns) return 0; out: - ima_fs_ns_free_dentries(ns); + ima_fs_ns_free_dentries(user_ns); return -1; } int __init ima_fs_init(void) { - return ima_fs_ns_init(&init_user_ns); + return ima_fs_ns_init(&init_user_ns, NULL); }