| Message ID | 20231130231948.2545638-5-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 12/1/2023 12:19 AM, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Before the security field of kernel objects could be shared among LSMs with > the LSM stacking feature, IMA and EVM had to rely on an alternative storage > of inode metadata. The association between inode metadata and inode is > maintained through an rbtree. > > With the reservation mechanism offered by the LSM infrastructure, the > rbtree is no longer necessary, as each LSM could reserve a space in the > security blob for each inode. > > With the 'integrity' LSM removed, and with the 'ima' LSM taking its role, > reserve space from the 'ima' LSM for a pointer to the integrity_iint_cache > structure directly, rather than embedding the whole structure in the inode > security blob, to minimize the changes and to avoid waste of memory. > > If IMA is disabled, EVM faces the same problems as before making it an > LSM, metadata verification fails for new files due to not setting the > IMA_NEW_FILE flag in ima_post_path_mknod(), and evm_verifyxattr() > returns INTEGRITY_UNKNOWN since IMA didn't call integrity_inode_get(). > > The only difference caused to moving the integrity metadata management > to the 'ima' LSM is the fact that EVM cannot take advantage of cached > verification results, and has to do the verification again. However, > this case should never happen, since the only public API available to > all kernel components, evm_verifyxattr(), does not work if IMA is > disabled. This needs some explanation on how EVM can be used currently. EVM verifies inode metadata in the set* LSM hooks, eventually updates the HMAC in the post_set* hooks. If integrity metadata are not available (IMA is disabled), EVM has to do the inode metadata verification every time, which means that this patch set would introduce a performance regression compared to when integrity metadata were always available and managed by the 'integrity' LSM. However, the get* LSM hooks are not mediated, user space can freely get a corrupted inode metadata and EVM would not tell anything. So, at this point it is clear that the main use case of EVM was a kernel component querying EVM about the integrity of inode metadata, by calling evm_verifyxattr(). One suitable place where this function can be called is the d_instantiate LSM hook, when an LSM is getting xattrs from the filesystem to populate the inode security blob. But as I mentioned, evm_verifyxattr() does not work if IMA is disabled, so there should not be systems using this configuration for which we are introducing a performance regression. Roberto > Introduce two primitives for getting and setting the pointer of > integrity_iint_cache in the security blob, respectively > integrity_inode_get_iint() and integrity_inode_set_iint(). This would make > the code more understandable, as they directly replace rbtree operations. > > Locking is not needed, as access to inode metadata is not shared, it is per > inode. > > Keep the blob size and the new primitives definition at the common level in > security/integrity rather than moving them in IMA itself, so that EVM can > still call integrity_inode_get() and integrity_iint_find() while IMA is > disabled. Just add an extra check in integrity_inode_get() to return NULL > if that is the case. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > security/integrity/iint.c | 70 ++++--------------------------- > security/integrity/ima/ima_main.c | 1 + > security/integrity/integrity.h | 20 ++++++++- > 3 files changed, 29 insertions(+), 62 deletions(-) > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > index c36054041b84..8fc9455dda11 100644 > --- a/security/integrity/iint.c > +++ b/security/integrity/iint.c > @@ -14,56 +14,25 @@ > #include <linux/slab.h> > #include <linux/init.h> > #include <linux/spinlock.h> > -#include <linux/rbtree.h> > #include <linux/file.h> > #include <linux/uaccess.h> > #include <linux/security.h> > #include <linux/lsm_hooks.h> > #include "integrity.h" > > -static struct rb_root integrity_iint_tree = RB_ROOT; > -static DEFINE_RWLOCK(integrity_iint_lock); > static struct kmem_cache *iint_cache __ro_after_init; > > struct dentry *integrity_dir; > > -/* > - * __integrity_iint_find - return the iint associated with an inode > - */ > -static struct integrity_iint_cache *__integrity_iint_find(struct inode *inode) > -{ > - struct integrity_iint_cache *iint; > - struct rb_node *n = integrity_iint_tree.rb_node; > - > - while (n) { > - iint = rb_entry(n, struct integrity_iint_cache, rb_node); > - > - if (inode < iint->inode) > - n = n->rb_left; > - else if (inode > iint->inode) > - n = n->rb_right; > - else > - return iint; > - } > - > - return NULL; > -} > - > /* > * integrity_iint_find - return the iint associated with an inode > */ > struct integrity_iint_cache *integrity_iint_find(struct inode *inode) > { > - struct integrity_iint_cache *iint; > - > if (!IS_IMA(inode)) > return NULL; > > - read_lock(&integrity_iint_lock); > - iint = __integrity_iint_find(inode); > - read_unlock(&integrity_iint_lock); > - > - return iint; > + return integrity_inode_get_iint(inode); > } > > #define IMA_MAX_NESTING (FILESYSTEM_MAX_STACK_DEPTH+1) > @@ -123,9 +92,7 @@ static void iint_free(struct integrity_iint_cache *iint) > */ > struct integrity_iint_cache *integrity_inode_get(struct inode *inode) > { > - struct rb_node **p; > - struct rb_node *node, *parent = NULL; > - struct integrity_iint_cache *iint, *test_iint; > + struct integrity_iint_cache *iint; > > /* > * After removing the 'integrity' LSM, the 'ima' LSM calls > @@ -144,31 +111,10 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) > > iint_init_always(iint, inode); > > - write_lock(&integrity_iint_lock); > - > - p = &integrity_iint_tree.rb_node; > - while (*p) { > - parent = *p; > - test_iint = rb_entry(parent, struct integrity_iint_cache, > - rb_node); > - if (inode < test_iint->inode) { > - p = &(*p)->rb_left; > - } else if (inode > test_iint->inode) { > - p = &(*p)->rb_right; > - } else { > - write_unlock(&integrity_iint_lock); > - kmem_cache_free(iint_cache, iint); > - return test_iint; > - } > - } > - > iint->inode = inode; > - node = &iint->rb_node; > inode->i_flags |= S_IMA; > - rb_link_node(node, parent, p); > - rb_insert_color(node, &integrity_iint_tree); > + integrity_inode_set_iint(inode, iint); > > - write_unlock(&integrity_iint_lock); > return iint; > } > > @@ -185,10 +131,8 @@ void integrity_inode_free(struct inode *inode) > if (!IS_IMA(inode)) > return; > > - write_lock(&integrity_iint_lock); > - iint = __integrity_iint_find(inode); > - rb_erase(&iint->rb_node, &integrity_iint_tree); > - write_unlock(&integrity_iint_lock); > + iint = integrity_iint_find(inode); > + integrity_inode_set_iint(inode, NULL); > > iint_free(iint); > } > @@ -212,6 +156,10 @@ int __init integrity_iintcache_init(void) > return 0; > } > > +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = { > + .lbs_inode = sizeof(struct integrity_iint_cache *), > +}; > + > /* > * integrity_kernel_read - read data from the file > * > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 3f59cce3fa02..52b4a3bba45a 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -1162,6 +1162,7 @@ DEFINE_LSM(ima) = { > .name = "ima", > .init = init_ima_lsm, > .order = LSM_ORDER_LAST, > + .blobs = &integrity_blob_sizes, > }; > > late_initcall(init_ima); /* Start IMA after the TPM is available */ > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index 26d3b08dca1c..2fb35c67d64d 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -158,7 +158,6 @@ struct ima_file_id { > > /* integrity data associated with an inode */ > struct integrity_iint_cache { > - struct rb_node rb_node; /* rooted in integrity_iint_tree */ > struct mutex mutex; /* protects: version, flags, digest */ > struct inode *inode; /* back pointer to inode in question */ > u64 version; /* track inode changes */ > @@ -194,6 +193,25 @@ int integrity_kernel_read(struct file *file, loff_t offset, > #define INTEGRITY_KEYRING_MAX 4 > > extern struct dentry *integrity_dir; > +extern struct lsm_blob_sizes integrity_blob_sizes; > + > +static inline struct integrity_iint_cache * > +integrity_inode_get_iint(const struct inode *inode) > +{ > + struct integrity_iint_cache **iint_sec; > + > + iint_sec = inode->i_security + integrity_blob_sizes.lbs_inode; > + return *iint_sec; > +} > + > +static inline void integrity_inode_set_iint(const struct inode *inode, > + struct integrity_iint_cache *iint) > +{ > + struct integrity_iint_cache **iint_sec; > + > + iint_sec = inode->i_security + integrity_blob_sizes.lbs_inode; > + *iint_sec = iint; > +} > > struct modsig; >
diff --git a/security/integrity/iint.c b/security/integrity/iint.c index c36054041b84..8fc9455dda11 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -14,56 +14,25 @@ #include <linux/slab.h> #include <linux/init.h> #include <linux/spinlock.h> -#include <linux/rbtree.h> #include <linux/file.h> #include <linux/uaccess.h> #include <linux/security.h> #include <linux/lsm_hooks.h> #include "integrity.h" -static struct rb_root integrity_iint_tree = RB_ROOT; -static DEFINE_RWLOCK(integrity_iint_lock); static struct kmem_cache *iint_cache __ro_after_init; struct dentry *integrity_dir; -/* - * __integrity_iint_find - return the iint associated with an inode - */ -static struct integrity_iint_cache *__integrity_iint_find(struct inode *inode) -{ - struct integrity_iint_cache *iint; - struct rb_node *n = integrity_iint_tree.rb_node; - - while (n) { - iint = rb_entry(n, struct integrity_iint_cache, rb_node); - - if (inode < iint->inode) - n = n->rb_left; - else if (inode > iint->inode) - n = n->rb_right; - else - return iint; - } - - return NULL; -} - /* * integrity_iint_find - return the iint associated with an inode */ struct integrity_iint_cache *integrity_iint_find(struct inode *inode) { - struct integrity_iint_cache *iint; - if (!IS_IMA(inode)) return NULL; - read_lock(&integrity_iint_lock); - iint = __integrity_iint_find(inode); - read_unlock(&integrity_iint_lock); - - return iint; + return integrity_inode_get_iint(inode); } #define IMA_MAX_NESTING (FILESYSTEM_MAX_STACK_DEPTH+1) @@ -123,9 +92,7 @@ static void iint_free(struct integrity_iint_cache *iint) */ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) { - struct rb_node **p; - struct rb_node *node, *parent = NULL; - struct integrity_iint_cache *iint, *test_iint; + struct integrity_iint_cache *iint; /* * After removing the 'integrity' LSM, the 'ima' LSM calls @@ -144,31 +111,10 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) iint_init_always(iint, inode); - write_lock(&integrity_iint_lock); - - p = &integrity_iint_tree.rb_node; - while (*p) { - parent = *p; - test_iint = rb_entry(parent, struct integrity_iint_cache, - rb_node); - if (inode < test_iint->inode) { - p = &(*p)->rb_left; - } else if (inode > test_iint->inode) { - p = &(*p)->rb_right; - } else { - write_unlock(&integrity_iint_lock); - kmem_cache_free(iint_cache, iint); - return test_iint; - } - } - iint->inode = inode; - node = &iint->rb_node; inode->i_flags |= S_IMA; - rb_link_node(node, parent, p); - rb_insert_color(node, &integrity_iint_tree); + integrity_inode_set_iint(inode, iint); - write_unlock(&integrity_iint_lock); return iint; } @@ -185,10 +131,8 @@ void integrity_inode_free(struct inode *inode) if (!IS_IMA(inode)) return; - write_lock(&integrity_iint_lock); - iint = __integrity_iint_find(inode); - rb_erase(&iint->rb_node, &integrity_iint_tree); - write_unlock(&integrity_iint_lock); + iint = integrity_iint_find(inode); + integrity_inode_set_iint(inode, NULL); iint_free(iint); } @@ -212,6 +156,10 @@ int __init integrity_iintcache_init(void) return 0; } +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = { + .lbs_inode = sizeof(struct integrity_iint_cache *), +}; + /* * integrity_kernel_read - read data from the file * diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 3f59cce3fa02..52b4a3bba45a 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -1162,6 +1162,7 @@ DEFINE_LSM(ima) = { .name = "ima", .init = init_ima_lsm, .order = LSM_ORDER_LAST, + .blobs = &integrity_blob_sizes, }; late_initcall(init_ima); /* Start IMA after the TPM is available */ diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 26d3b08dca1c..2fb35c67d64d 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -158,7 +158,6 @@ struct ima_file_id { /* integrity data associated with an inode */ struct integrity_iint_cache { - struct rb_node rb_node; /* rooted in integrity_iint_tree */ struct mutex mutex; /* protects: version, flags, digest */ struct inode *inode; /* back pointer to inode in question */ u64 version; /* track inode changes */ @@ -194,6 +193,25 @@ int integrity_kernel_read(struct file *file, loff_t offset, #define INTEGRITY_KEYRING_MAX 4 extern struct dentry *integrity_dir; +extern struct lsm_blob_sizes integrity_blob_sizes; + +static inline struct integrity_iint_cache * +integrity_inode_get_iint(const struct inode *inode) +{ + struct integrity_iint_cache **iint_sec; + + iint_sec = inode->i_security + integrity_blob_sizes.lbs_inode; + return *iint_sec; +} + +static inline void integrity_inode_set_iint(const struct inode *inode, + struct integrity_iint_cache *iint) +{ + struct integrity_iint_cache **iint_sec; + + iint_sec = inode->i_security + integrity_blob_sizes.lbs_inode; + *iint_sec = iint; +} struct modsig;