On 17.11.23 21:57, Paul Moore wrote:
On Nov 7, 2023 Roberto Sassu <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
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.
Because of this alternative storage mechanism, there was no need to use
disjoint inode metadata, so IMA and EVM today still share them.
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. However, since IMA and EVM share the
inode metadata, they cannot directly reserve the space for them.
Instead, request from the 'integrity' LSM a space in the security blob for
the pointer of inode metadata (integrity_iint_cache structure). The other
reason for keeping the 'integrity' LSM is to preserve the original ordering
of IMA and EVM functions as when they were hardcoded.
Prefer reserving space for a pointer to allocating the integrity_iint_cache
structure directly, as IMA would require it only for a subset of inodes.
Always allocating it would cause a waste of memory.
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.
Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
---
security/integrity/iint.c | 71 +++++-----------------------------
security/integrity/integrity.h | 20 +++++++++-
2 files changed, 29 insertions(+), 62 deletions(-)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 882fde2a2607..a5edd3c70784 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -231,6 +175,10 @@ static int __init integrity_lsm_init(void)
return 0;
}
+struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
+ .lbs_inode = sizeof(struct integrity_iint_cache *),
+};
I'll admit that I'm likely missing an important detail, but is there
a reason why you couldn't stash the integrity_iint_cache struct
directly in the inode's security blob instead of the pointer? For
example:
struct lsm_blob_sizes ... = {
.lbs_inode = sizeof(struct integrity_iint_cache),
};
struct integrity_iint_cache *integrity_inode_get(inode)
{
if (unlikely(!inode->isecurity))
return NULL;
Ok, this caught my attention...
I see that selinux_inode() has it, but smack_inode() doesn't.
Some Smack code assumes that the inode security blob is always non-NULL:
static void init_inode_smack(struct inode *inode, struct smack_known *skp)
{
struct inode_smack *isp = smack_inode(inode);
isp->smk_inode = skp;
isp->smk_flags = 0;
}
Is that intended? Should I add the check?
Thanks
Roberto
return inode->i_security + integrity_blob_sizes.lbs_inode;
}
--
paul-moore.com
