From: Roberto Sassu <[email protected]>
IMA and EVM are not effectively LSMs, especially due the fact that in the
past they could not provide a security blob while there is another LSM
active.
That changed in the recent years, the LSM stacking feature now makes it
possible to stack together multiple LSMs, and allows them to provide a
security blob for most kernel objects. While the LSM stacking feature has
some limitations being worked out, it is already suitable to make IMA and
EVM as LSMs.
In short, while this patch set is big, it does not make any functional
change to IMA and EVM. IMA and EVM functions are called by the LSM
infrastructure in the same places as before (except ima_post_path_mknod()),
rather being hardcoded calls, and the inode metadata pointer is directly
stored in the inode security blob rather than in a separate rbtree.
More specifically, patches 1-11 make IMA and EVM functions suitable to
be registered to the LSM infrastructure, by aligning function parameters.
Patches 12-20 add new LSM hooks in the same places where IMA and EVM
functions are called, if there is no LSM hook already.
Patches 21-24 do the bulk of the work, remove hardcoded calls to IMA, EVM
and integrity functions, register those functions in the LSM
infrastructure, and let the latter call them. In addition, they also
reserve one slot for EVM to supply an xattr with the inode_init_security
hook.
Finally, patch 25 removes the rbtree used to bind metadata to the inodes,
and instead reserves a space in the inode security blob to store the
pointer to metadata. This also brings performance improvements due to
retrieving metadata in constant time, as opposed to logarithmic.
The patch set applies on top of lsm/next, commit 8e4672d6f902 ("lsm:
constify the 'file' parameter in security_binder_transfer_file()")
Changelog:
v2:
- Add description for newly introduced LSM hooks (suggested by Casey)
- Clarify in the description of security_file_pre_free() that actions can
be performed while the file is still open
v1:
- Drop 'evm: Complete description of evm_inode_setattr()', 'fs: Fix
description of vfs_tmpfile()' and 'security: Introduce LSM_ORDER_LAST',
they were sent separately (suggested by Christian Brauner)
- Replace dentry with file descriptor parameter for
security_inode_post_create_tmpfile()
- Introduce mode_stripped and pass it as mode argument to
security_path_mknod() and security_path_post_mknod()
- Use goto in do_mknodat() and __vfs_removexattr_locked() (suggested by
Mimi)
- Replace __lsm_ro_after_init with __ro_after_init
- Modify short description of security_inode_post_create_tmpfile() and
security_inode_post_set_acl() (suggested by Stefan)
- Move security_inode_post_setattr() just after security_inode_setattr()
(suggested by Mimi)
- Modify short description of security_key_post_create_or_update()
(suggested by Mimi)
- Add back exported functions ima_file_check() and
evm_inode_init_security() respectively to ima.h and evm.h (reported by
kernel robot)
- Remove extern from prototype declarations and fix style issues
- Remove unnecessary include of linux/lsm_hooks.h in ima_main.c and
ima_appraise.c
Roberto Sassu (25):
ima: Align ima_inode_post_setattr() definition with LSM infrastructure
ima: Align ima_post_path_mknod() definition with LSM infrastructure
ima: Align ima_post_create_tmpfile() definition with LSM
infrastructure
ima: Align ima_file_mprotect() definition with LSM infrastructure
ima: Align ima_inode_setxattr() definition with LSM infrastructure
ima: Align ima_inode_removexattr() definition with LSM infrastructure
ima: Align ima_post_read_file() definition with LSM infrastructure
evm: Align evm_inode_post_setattr() definition with LSM infrastructure
evm: Align evm_inode_setxattr() definition with LSM infrastructure
evm: Align evm_inode_post_setxattr() definition with LSM
infrastructure
security: Align inode_setattr hook definition with EVM
security: Introduce inode_post_setattr hook
security: Introduce inode_post_removexattr hook
security: Introduce file_post_open hook
security: Introduce file_pre_free_security hook
security: Introduce path_post_mknod hook
security: Introduce inode_post_create_tmpfile hook
security: Introduce inode_post_set_acl hook
security: Introduce inode_post_remove_acl hook
security: Introduce key_post_create_or_update hook
ima: Move to LSM infrastructure
ima: Move IMA-Appraisal to LSM infrastructure
evm: Move to LSM infrastructure
integrity: Move integrity functions to the LSM infrastructure
integrity: Switch from rbtree to LSM-managed blob for
integrity_iint_cache
fs/attr.c | 5 +-
fs/file_table.c | 3 +-
fs/namei.c | 18 +-
fs/nfsd/vfs.c | 3 +-
fs/open.c | 1 -
fs/posix_acl.c | 5 +-
fs/xattr.c | 9 +-
include/linux/evm.h | 103 ----------
include/linux/ima.h | 136 -------------
include/linux/integrity.h | 26 ---
include/linux/lsm_hook_defs.h | 21 +-
include/linux/security.h | 65 +++++++
security/integrity/evm/evm_main.c | 104 ++++++++--
security/integrity/iint.c | 92 +++------
security/integrity/ima/ima.h | 11 ++
security/integrity/ima/ima_appraise.c | 37 +++-
security/integrity/ima/ima_main.c | 76 ++++++--
security/integrity/integrity.h | 44 ++++-
security/keys/key.c | 10 +-
security/security.c | 265 ++++++++++++++++----------
security/selinux/hooks.c | 3 +-
security/smack/smack_lsm.c | 4 +-
22 files changed, 540 insertions(+), 501 deletions(-)
--
2.34.1
From: Roberto Sassu <[email protected]>
Change evm_inode_post_setattr() definition, so that it can be registered as
implementation of the inode_post_setattr hook.
Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
fs/attr.c | 2 +-
include/linux/evm.h | 6 ++++--
security/integrity/evm/evm_main.c | 4 +++-
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index 7d4553c1208d..431f667726c7 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -487,7 +487,7 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
if (!error) {
fsnotify_change(dentry, ia_valid);
ima_inode_post_setattr(idmap, dentry, ia_valid);
- evm_inode_post_setattr(dentry, ia_valid);
+ evm_inode_post_setattr(idmap, dentry, ia_valid);
}
return error;
diff --git a/include/linux/evm.h b/include/linux/evm.h
index 01fc495a83e2..aebaae181fd9 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -23,7 +23,8 @@ extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
struct integrity_iint_cache *iint);
extern int evm_inode_setattr(struct mnt_idmap *idmap,
struct dentry *dentry, struct iattr *attr);
-extern void evm_inode_post_setattr(struct dentry *dentry, int ia_valid);
+void evm_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ int ia_valid);
extern int evm_inode_setxattr(struct mnt_idmap *idmap,
struct dentry *dentry, const char *name,
const void *value, size_t size);
@@ -97,7 +98,8 @@ static inline int evm_inode_setattr(struct mnt_idmap *idmap,
return 0;
}
-static inline void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
+static inline void evm_inode_post_setattr(struct mnt_idmap *idmap,
+ struct dentry *dentry, int ia_valid)
{
return;
}
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index ff9a939dad8e..d2f986a55d38 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -841,6 +841,7 @@ int evm_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
/**
* evm_inode_post_setattr - update 'security.evm' after modifying metadata
+ * @idmap: idmap of the idmapped mount
* @dentry: pointer to the affected dentry
* @ia_valid: for the UID and GID status
*
@@ -850,7 +851,8 @@ int evm_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
* This function is called from notify_change(), which expects the caller
* to lock the inode's i_mutex.
*/
-void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
+void evm_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ int ia_valid)
{
if (!evm_revalidate_status(NULL))
return;
--
2.34.1
From: Roberto Sassu <[email protected]>
Change ima_file_mprotect() definition, so that it can be registered
as implementation of the file_mprotect hook.
Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 5 +++--
security/integrity/ima/ima_main.c | 6 ++++--
security/security.c | 2 +-
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 893c3b98b4d0..56e72c0beb96 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -24,7 +24,8 @@ extern void ima_post_create_tmpfile(struct mnt_idmap *idmap,
extern void ima_file_free(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long reqprot,
unsigned long prot, unsigned long flags);
-extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot);
+int ima_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
+ unsigned long prot);
extern int ima_load_data(enum kernel_load_data_id id, bool contents);
extern int ima_post_load_data(char *buf, loff_t size,
enum kernel_load_data_id id, char *description);
@@ -88,7 +89,7 @@ static inline int ima_file_mmap(struct file *file, unsigned long reqprot,
}
static inline int ima_file_mprotect(struct vm_area_struct *vma,
- unsigned long prot)
+ unsigned long reqprot, unsigned long prot)
{
return 0;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 52e742d32f4b..e9e2a3ad25a1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -441,7 +441,8 @@ int ima_file_mmap(struct file *file, unsigned long reqprot,
/**
* ima_file_mprotect - based on policy, limit mprotect change
* @vma: vm_area_struct protection is set to
- * @prot: contains the protection that will be applied by the kernel.
+ * @reqprot: protection requested by the application
+ * @prot: protection that will be applied by the kernel
*
* Files can be mmap'ed read/write and later changed to execute to circumvent
* IMA's mmap appraisal policy rules. Due to locking issues (mmap semaphore
@@ -451,7 +452,8 @@ int ima_file_mmap(struct file *file, unsigned long reqprot,
*
* On mprotect change success, return 0. On failure, return -EACESS.
*/
-int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
+int ima_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
+ unsigned long prot)
{
struct ima_template_desc *template = NULL;
struct file *file;
diff --git a/security/security.c b/security/security.c
index 96f2c68a1571..dffb67e6e119 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2721,7 +2721,7 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
ret = call_int_hook(file_mprotect, 0, vma, reqprot, prot);
if (ret)
return ret;
- return ima_file_mprotect(vma, prot);
+ return ima_file_mprotect(vma, reqprot, prot);
}
/**
--
2.34.1
From: Roberto Sassu <[email protected]>
Add the idmap parameter to the definition, so that evm_inode_setattr() can
be registered as this hook implementation.
Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
Acked-by: Casey Schaufler <[email protected]>
---
include/linux/lsm_hook_defs.h | 3 ++-
security/security.c | 2 +-
security/selinux/hooks.c | 3 ++-
security/smack/smack_lsm.c | 4 +++-
4 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 4bdddb52a8fe..fdf075a6b1bb 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -134,7 +134,8 @@ LSM_HOOK(int, 0, inode_readlink, struct dentry *dentry)
LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
bool rcu)
LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
-LSM_HOOK(int, 0, inode_setattr, struct dentry *dentry, struct iattr *attr)
+LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry,
+ struct iattr *attr)
LSM_HOOK(int, 0, inode_getattr, const struct path *path)
LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
struct dentry *dentry, const char *name, const void *value,
diff --git a/security/security.c b/security/security.c
index cb6242feb968..2b24d01cf181 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2117,7 +2117,7 @@ int security_inode_setattr(struct mnt_idmap *idmap,
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
- ret = call_int_hook(inode_setattr, 0, dentry, attr);
+ ret = call_int_hook(inode_setattr, 0, idmap, dentry, attr);
if (ret)
return ret;
return evm_inode_setattr(idmap, dentry, attr);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ee7c49c2cfd3..bfcc4d9aa5ab 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3075,7 +3075,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
return rc;
}
-static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
+static int selinux_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ struct iattr *iattr)
{
const struct cred *cred = current_cred();
struct inode *inode = d_backing_inode(dentry);
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 679156601a10..89f2669d50a9 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1181,12 +1181,14 @@ static int smack_inode_permission(struct inode *inode, int mask)
/**
* smack_inode_setattr - Smack check for setting attributes
+ * @idmap: idmap of the mount
* @dentry: the object
* @iattr: for the force flag
*
* Returns 0 if access is permitted, an error code otherwise
*/
-static int smack_inode_setattr(struct dentry *dentry, struct iattr *iattr)
+static int smack_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ struct iattr *iattr)
{
struct smk_audit_info ad;
int rc;
--
2.34.1
From: Roberto Sassu <[email protected]>
In preparation for moving IMA and EVM to the LSM infrastructure, introduce
the path_post_mknod hook.
It is useful for IMA to let new empty files be subsequently opened for
further modification.
LSMs can benefit from this hook to update the inode state after a file has
been successfully created. The new hook cannot return an error and cannot
cause the operation to be canceled.
Signed-off-by: Roberto Sassu <[email protected]>
---
fs/namei.c | 5 +++++
include/linux/lsm_hook_defs.h | 3 +++
include/linux/security.h | 9 +++++++++
security/security.c | 19 +++++++++++++++++++
4 files changed, 36 insertions(+)
diff --git a/fs/namei.c b/fs/namei.c
index 7dc4626859f0..c8c4ab26b52a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4061,6 +4061,11 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
dentry, mode, 0);
break;
}
+
+ if (error)
+ goto out2;
+
+ security_path_post_mknod(idmap, &path, dentry, mode_stripped, dev);
out2:
done_path_create(&path, dentry);
if (retry_estale(error, lookup_flags)) {
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 797f51da3f7d..b1634b5de98c 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -93,6 +93,9 @@ LSM_HOOK(int, 0, path_mkdir, const struct path *dir, struct dentry *dentry,
LSM_HOOK(int, 0, path_rmdir, const struct path *dir, struct dentry *dentry)
LSM_HOOK(int, 0, path_mknod, const struct path *dir, struct dentry *dentry,
umode_t mode, unsigned int dev)
+LSM_HOOK(void, LSM_RET_VOID, path_post_mknod, struct mnt_idmap *idmap,
+ const struct path *dir, struct dentry *dentry, umode_t mode,
+ unsigned int dev)
LSM_HOOK(int, 0, path_truncate, const struct path *path)
LSM_HOOK(int, 0, path_symlink, const struct path *dir, struct dentry *dentry,
const char *old_name)
diff --git a/include/linux/security.h b/include/linux/security.h
index 7871009d59ae..f210bd66e939 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1842,6 +1842,9 @@ int security_path_mkdir(const struct path *dir, struct dentry *dentry, umode_t m
int security_path_rmdir(const struct path *dir, struct dentry *dentry);
int security_path_mknod(const struct path *dir, struct dentry *dentry, umode_t mode,
unsigned int dev);
+void security_path_post_mknod(struct mnt_idmap *idmap, const struct path *dir,
+ struct dentry *dentry, umode_t mode,
+ unsigned int dev);
int security_path_truncate(const struct path *path);
int security_path_symlink(const struct path *dir, struct dentry *dentry,
const char *old_name);
@@ -1876,6 +1879,12 @@ static inline int security_path_mknod(const struct path *dir, struct dentry *den
return 0;
}
+static inline void security_path_post_mknod(struct mnt_idmap *idmap,
+ const struct path *dir,
+ struct dentry *dentry, umode_t mode,
+ unsigned int dev)
+{ }
+
static inline int security_path_truncate(const struct path *path)
{
return 0;
diff --git a/security/security.c b/security/security.c
index fbb58eeeea02..78aeb24efedb 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1702,6 +1702,25 @@ int security_path_mknod(const struct path *dir, struct dentry *dentry,
}
EXPORT_SYMBOL(security_path_mknod);
+/**
+ * security_path_post_mknod() - Update inode security field after file creation
+ * @idmap: idmap of the mount
+ * @dir: parent directory
+ * @dentry: new file
+ * @mode: new file mode
+ * @dev: device number
+ *
+ * Update inode security field after a file has been created.
+ */
+void security_path_post_mknod(struct mnt_idmap *idmap, const struct path *dir,
+ struct dentry *dentry, umode_t mode,
+ unsigned int dev)
+{
+ if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
+ return;
+ call_void_hook(path_post_mknod, idmap, dir, dentry, mode, dev);
+}
+
/**
* security_path_mkdir() - Check if creating a new directory is allowed
* @dir: parent directory
--
2.34.1
From: Roberto Sassu <[email protected]>
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. Thus, request from the 'integrity' LSM a
space in the security blob for the pointer of inode metadata
(integrity_iint_cache structure).
Prefer this 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 <[email protected]>
Reviewed-by: Casey Schaufler <[email protected]>
---
security/integrity/iint.c | 67 +++-------------------------------
security/integrity/integrity.h | 19 +++++++++-
2 files changed, 24 insertions(+), 62 deletions(-)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 70ee803a33ea..c2fba8afbbdb 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 __read_mostly;
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);
}
static void iint_free(struct integrity_iint_cache *iint)
@@ -92,9 +61,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;
iint = integrity_iint_find(inode);
if (iint)
@@ -104,31 +71,10 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
if (!iint)
return NULL;
- 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;
}
@@ -145,10 +91,8 @@ static 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);
}
@@ -188,6 +132,7 @@ static int __init integrity_lsm_init(void)
}
struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
+ .lbs_inode = sizeof(struct integrity_iint_cache *),
.lbs_xattr_count = 1,
};
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index e020c365997b..24de4ad4a37e 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 */
@@ -192,6 +191,24 @@ int integrity_kernel_read(struct file *file, loff_t offset,
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;
#ifdef CONFIG_IMA
--
2.34.1
From: Roberto Sassu <[email protected]>
In preparation for moving IMA and EVM to the LSM infrastructure, introduce
the inode_post_setattr hook.
It is useful for EVM to recalculate the HMAC on modified file attributes
and other file metadata, after it verified the HMAC of current file
metadata with the inode_setattr hook.
LSMs should use the new hook instead of inode_setattr, when they need to
know that the operation was done successfully (not known in inode_setattr).
The new hook cannot return an error and cannot cause the operation to be
reverted.
Signed-off-by: Roberto Sassu <[email protected]>
---
fs/attr.c | 1 +
include/linux/lsm_hook_defs.h | 2 ++
include/linux/security.h | 7 +++++++
security/security.c | 16 ++++++++++++++++
4 files changed, 26 insertions(+)
diff --git a/fs/attr.c b/fs/attr.c
index 431f667726c7..3c309eb456c6 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -486,6 +486,7 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
if (!error) {
fsnotify_change(dentry, ia_valid);
+ security_inode_post_setattr(idmap, dentry, ia_valid);
ima_inode_post_setattr(idmap, dentry, ia_valid);
evm_inode_post_setattr(idmap, dentry, ia_valid);
}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index fdf075a6b1bb..995d30336cfa 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -136,6 +136,8 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry,
struct iattr *attr)
+LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap,
+ struct dentry *dentry, int ia_valid)
LSM_HOOK(int, 0, inode_getattr, const struct path *path)
LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
struct dentry *dentry, const char *name, const void *value,
diff --git a/include/linux/security.h b/include/linux/security.h
index dcb3604ffab8..820899db5276 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -355,6 +355,8 @@ int security_inode_follow_link(struct dentry *dentry, struct inode *inode,
int security_inode_permission(struct inode *inode, int mask);
int security_inode_setattr(struct mnt_idmap *idmap,
struct dentry *dentry, struct iattr *attr);
+void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ int ia_valid);
int security_inode_getattr(const struct path *path);
int security_inode_setxattr(struct mnt_idmap *idmap,
struct dentry *dentry, const char *name,
@@ -856,6 +858,11 @@ static inline int security_inode_setattr(struct mnt_idmap *idmap,
return 0;
}
+static inline void
+security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ int ia_valid)
+{ }
+
static inline int security_inode_getattr(const struct path *path)
{
return 0;
diff --git a/security/security.c b/security/security.c
index 2b24d01cf181..764a6f28b3b9 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2124,6 +2124,22 @@ int security_inode_setattr(struct mnt_idmap *idmap,
}
EXPORT_SYMBOL_GPL(security_inode_setattr);
+/**
+ * security_inode_post_setattr() - Update the inode after a setattr operation
+ * @idmap: idmap of the mount
+ * @dentry: file
+ * @ia_valid: file attributes set
+ *
+ * Update inode security field after successful setting file attributes.
+ */
+void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ int ia_valid)
+{
+ if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
+ return;
+ call_void_hook(inode_post_setattr, idmap, dentry, ia_valid);
+}
+
/**
* security_inode_getattr() - Check if getting file attributes is allowed
* @path: file
--
2.34.1
From: Roberto Sassu <[email protected]>
Remove hardcoded calls to integrity functions from the LSM infrastructure.
Also move the global declaration of integrity_inode_get() to
security/integrity/integrity.h, so that the function can be still called by
IMA.
Register integrity functions as hook implementations in
integrity_lsm_init().
Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Casey Schaufler <[email protected]>
---
include/linux/integrity.h | 26 --------------------------
security/integrity/iint.c | 11 ++++++++++-
security/integrity/integrity.h | 7 +++++++
security/security.c | 9 +--------
4 files changed, 18 insertions(+), 35 deletions(-)
diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index 2ea0f2f65ab6..afaae7ad26f4 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -21,38 +21,12 @@ enum integrity_status {
/* List of EVM protected security xattrs */
#ifdef CONFIG_INTEGRITY
-extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
-extern void integrity_inode_free(struct inode *inode);
extern void __init integrity_load_keys(void);
#else
-static inline struct integrity_iint_cache *
- integrity_inode_get(struct inode *inode)
-{
- return NULL;
-}
-
-static inline void integrity_inode_free(struct inode *inode)
-{
- return;
-}
-
static inline void integrity_load_keys(void)
{
}
#endif /* CONFIG_INTEGRITY */
-#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
-
-extern int integrity_kernel_module_request(char *kmod_name);
-
-#else
-
-static inline int integrity_kernel_module_request(char *kmod_name)
-{
- return 0;
-}
-
-#endif /* CONFIG_INTEGRITY_ASYMMETRIC_KEYS */
-
#endif /* _LINUX_INTEGRITY_H */
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index dd03f978b45c..70ee803a33ea 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -138,7 +138,7 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
*
* Free the integrity information(iint) associated with an inode.
*/
-void integrity_inode_free(struct inode *inode)
+static void integrity_inode_free(struct inode *inode)
{
struct integrity_iint_cache *iint;
@@ -167,12 +167,21 @@ static void init_once(void *foo)
mutex_init(&iint->mutex);
}
+static struct security_hook_list integrity_hooks[] __ro_after_init = {
+ LSM_HOOK_INIT(inode_free_security, integrity_inode_free),
+#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
+ LSM_HOOK_INIT(kernel_module_request, integrity_kernel_module_request),
+#endif
+};
+
static int __init integrity_lsm_init(void)
{
iint_cache =
kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
0, SLAB_PANIC, init_once);
+ security_add_hooks(integrity_hooks, ARRAY_SIZE(integrity_hooks),
+ "integrity");
init_ima_lsm();
init_evm_lsm();
return 0;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 83a465ac9013..e020c365997b 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -178,6 +178,7 @@ struct integrity_iint_cache {
* integrity data associated with an inode.
*/
struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
+struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
int integrity_kernel_read(struct file *file, loff_t offset,
void *addr, unsigned long count);
@@ -251,12 +252,18 @@ static inline int __init integrity_load_cert(const unsigned int id,
#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
int asymmetric_verify(struct key *keyring, const char *sig,
int siglen, const char *data, int datalen);
+int integrity_kernel_module_request(char *kmod_name);
#else
static inline int asymmetric_verify(struct key *keyring, const char *sig,
int siglen, const char *data, int datalen)
{
return -EOPNOTSUPP;
}
+
+static inline int integrity_kernel_module_request(char *kmod_name)
+{
+ return 0;
+}
#endif
#ifdef CONFIG_IMA_APPRAISE_MODSIG
diff --git a/security/security.c b/security/security.c
index 5e700557e434..5f26a0c16d48 100644
--- a/security/security.c
+++ b/security/security.c
@@ -19,7 +19,6 @@
#include <linux/kernel.h>
#include <linux/kernel_read_file.h>
#include <linux/lsm_hooks.h>
-#include <linux/integrity.h>
#include <linux/fsnotify.h>
#include <linux/mman.h>
#include <linux/mount.h>
@@ -1497,7 +1496,6 @@ static void inode_free_by_rcu(struct rcu_head *head)
*/
void security_inode_free(struct inode *inode)
{
- integrity_inode_free(inode);
call_void_hook(inode_free_security, inode);
/*
* The inode may still be referenced in a path walk and
@@ -3090,12 +3088,7 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
*/
int security_kernel_module_request(char *kmod_name)
{
- int ret;
-
- ret = call_int_hook(kernel_module_request, 0, kmod_name);
- if (ret)
- return ret;
- return integrity_kernel_module_request(kmod_name);
+ return call_int_hook(kernel_module_request, 0, kmod_name);
}
/**
--
2.34.1
From: Roberto Sassu <[email protected]>
Change ima_inode_setxattr() definition, so that it can be registered as
implementation of the inode_setxattr hook.
Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 11 +++++++----
security/integrity/ima/ima_appraise.c | 5 +++--
security/security.c | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 56e72c0beb96..3dedff8659d2 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -194,8 +194,9 @@ static inline void ima_post_key_create_or_update(struct key *keyring,
extern bool is_ima_appraise_enabled(void);
extern void ima_inode_post_setattr(struct mnt_idmap *idmap,
struct dentry *dentry, int ia_valid);
-extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
- const void *xattr_value, size_t xattr_value_len);
+int ima_inode_setxattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ const char *xattr_name, const void *xattr_value,
+ size_t xattr_value_len, int flags);
extern int ima_inode_set_acl(struct mnt_idmap *idmap,
struct dentry *dentry, const char *acl_name,
struct posix_acl *kacl);
@@ -218,10 +219,12 @@ static inline void ima_inode_post_setattr(struct mnt_idmap *idmap,
return;
}
-static inline int ima_inode_setxattr(struct dentry *dentry,
+static inline int ima_inode_setxattr(struct mnt_idmap *idmap,
+ struct dentry *dentry,
const char *xattr_name,
const void *xattr_value,
- size_t xattr_value_len)
+ size_t xattr_value_len,
+ int flags)
{
return 0;
}
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 6b032bce4fe7..88c5a0b2992f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -748,8 +748,9 @@ static int validate_hash_algo(struct dentry *dentry,
return -EACCES;
}
-int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
- const void *xattr_value, size_t xattr_value_len)
+int ima_inode_setxattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ const char *xattr_name, const void *xattr_value,
+ size_t xattr_value_len, int flags)
{
const struct evm_ima_xattr_data *xvalue = xattr_value;
int digsig = 0;
diff --git a/security/security.c b/security/security.c
index dffb67e6e119..00826269481e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2171,7 +2171,7 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
ret = cap_inode_setxattr(dentry, name, value, size, flags);
if (ret)
return ret;
- ret = ima_inode_setxattr(dentry, name, value, size);
+ ret = ima_inode_setxattr(idmap, dentry, name, value, size, flags);
if (ret)
return ret;
return evm_inode_setxattr(idmap, dentry, name, value, size);
--
2.34.1
From: Roberto Sassu <[email protected]>
Change ima_post_path_mknod() definition, so that it can be registered as
implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
well.
Also, make sure that ima_post_path_mknod() is executed only if
(mode & S_IFMT) is equal to zero or S_IFREG.
Add this check to take into account the different placement of the
path_post_mknod hook (to be introduced) in do_mknodat(). Since the new hook
will be placed after the switch(), the check ensures that
ima_post_path_mknod() is invoked as originally intended when it is
registered as implementation of path_post_mknod.
Signed-off-by: Roberto Sassu <[email protected]>
---
fs/namei.c | 9 ++++++---
include/linux/ima.h | 7 +++++--
security/integrity/ima/ima_main.c | 10 +++++++++-
3 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index e56ff39a79bc..c5e96f716f98 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
struct path path;
int error;
unsigned int lookup_flags = 0;
+ umode_t mode_stripped;
error = may_mknod(mode);
if (error)
@@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
if (IS_ERR(dentry))
goto out1;
- error = security_path_mknod(&path, dentry,
- mode_strip_umask(path.dentry->d_inode, mode), dev);
+ mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
+
+ error = security_path_mknod(&path, dentry, mode_stripped, dev);
if (error)
goto out2;
@@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
error = vfs_create(idmap, path.dentry->d_inode,
dentry, mode, true);
if (!error)
- ima_post_path_mknod(idmap, dentry);
+ ima_post_path_mknod(idmap, &path, dentry,
+ mode_stripped, dev);
break;
case S_IFCHR: case S_IFBLK:
error = vfs_mknod(idmap, path.dentry->d_inode,
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 910a2f11a906..179ce52013b2 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct mnt_idmap *idmap,
- struct dentry *dentry);
+ const struct path *dir, struct dentry *dentry,
+ umode_t mode, unsigned int dev);
extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
@@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
}
static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
- struct dentry *dentry)
+ const struct path *dir,
+ struct dentry *dentry,
+ umode_t mode, unsigned int dev)
{
return;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 365db0e43d7c..76eba92d7f10 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
/**
* ima_post_path_mknod - mark as a new inode
* @idmap: idmap of the mount the inode was found from
+ * @dir: path structure of parent of the new file
* @dentry: newly created dentry
+ * @mode: mode of the new file
+ * @dev: undecoded device number
*
* Mark files created via the mknodat syscall as new, so that the
* file data can be written later.
*/
void ima_post_path_mknod(struct mnt_idmap *idmap,
- struct dentry *dentry)
+ const struct path *dir, struct dentry *dentry,
+ umode_t mode, unsigned int dev)
{
struct integrity_iint_cache *iint;
struct inode *inode = dentry->d_inode;
int must_appraise;
+ /* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
+ if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
+ return;
+
if (!ima_policy_flag || !S_ISREG(inode->i_mode))
return;
--
2.34.1
From: Roberto Sassu <[email protected]>
Change ima_post_read_file() definition, so that it can be registered as
implementation of the post_read_file hook.
Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 6 +++---
security/integrity/ima/ima_main.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 93e3c6cdf1f8..6e4d060ff378 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -31,8 +31,8 @@ extern int ima_post_load_data(char *buf, loff_t size,
enum kernel_load_data_id id, char *description);
extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
bool contents);
-extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
- enum kernel_read_file_id id);
+int ima_post_read_file(struct file *file, char *buf, loff_t size,
+ enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct mnt_idmap *idmap,
const struct path *dir, struct dentry *dentry,
umode_t mode, unsigned int dev);
@@ -112,7 +112,7 @@ static inline int ima_read_file(struct file *file, enum kernel_read_file_id id,
return 0;
}
-static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
+static inline int ima_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id)
{
return 0;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e9e2a3ad25a1..f8581032e62c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -801,7 +801,7 @@ const int read_idmap[READING_MAX_ID] = {
* On success return 0. On integrity appraisal error, assuming the file
* is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
*/
-int ima_post_read_file(struct file *file, void *buf, loff_t size,
+int ima_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id read_id)
{
enum ima_hooks func;
--
2.34.1
From: Roberto Sassu <[email protected]>
In preparation for moving IMA and EVM to the LSM infrastructure, introduce
the inode_post_set_acl hook.
It is useful for EVM to recalculate the HMAC on the modified POSIX ACL and
other file metadata, after it verified the HMAC of current file metadata
with the inode_set_acl hook.
LSMs should use the new hook instead of inode_set_acl, when they need to
know that the operation was done successfully (not known in inode_set_acl).
The new hook cannot return an error and cannot cause the operation to be
reverted.
Signed-off-by: Roberto Sassu <[email protected]>
---
fs/posix_acl.c | 1 +
include/linux/lsm_hook_defs.h | 2 ++
include/linux/security.h | 7 +++++++
security/security.c | 17 +++++++++++++++++
4 files changed, 27 insertions(+)
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 7fa1b738bbab..3b7dbea5c3ff 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -1137,6 +1137,7 @@ int vfs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
error = -EIO;
if (!error) {
fsnotify_xattr(dentry);
+ security_inode_post_set_acl(dentry, acl_name, kacl);
evm_inode_post_set_acl(dentry, acl_name, kacl);
}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 9ae573b83737..bba1fbd97207 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -157,6 +157,8 @@ LSM_HOOK(void, LSM_RET_VOID, inode_post_removexattr, struct dentry *dentry,
const char *name)
LSM_HOOK(int, 0, inode_set_acl, struct mnt_idmap *idmap,
struct dentry *dentry, const char *acl_name, struct posix_acl *kacl)
+LSM_HOOK(void, LSM_RET_VOID, inode_post_set_acl, struct dentry *dentry,
+ const char *acl_name, struct posix_acl *kacl)
LSM_HOOK(int, 0, inode_get_acl, struct mnt_idmap *idmap,
struct dentry *dentry, const char *acl_name)
LSM_HOOK(int, 0, inode_remove_acl, struct mnt_idmap *idmap,
diff --git a/include/linux/security.h b/include/linux/security.h
index 5f296761883f..556d019ebe5c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -367,6 +367,8 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
int security_inode_set_acl(struct mnt_idmap *idmap,
struct dentry *dentry, const char *acl_name,
struct posix_acl *kacl);
+void security_inode_post_set_acl(struct dentry *dentry, const char *acl_name,
+ struct posix_acl *kacl);
int security_inode_get_acl(struct mnt_idmap *idmap,
struct dentry *dentry, const char *acl_name);
int security_inode_remove_acl(struct mnt_idmap *idmap,
@@ -894,6 +896,11 @@ static inline int security_inode_set_acl(struct mnt_idmap *idmap,
return 0;
}
+static inline void security_inode_post_set_acl(struct dentry *dentry,
+ const char *acl_name,
+ struct posix_acl *kacl)
+{ }
+
static inline int security_inode_get_acl(struct mnt_idmap *idmap,
struct dentry *dentry,
const char *acl_name)
diff --git a/security/security.c b/security/security.c
index aa6274c90147..aabace9e24d9 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2260,6 +2260,23 @@ int security_inode_set_acl(struct mnt_idmap *idmap,
return evm_inode_set_acl(idmap, dentry, acl_name, kacl);
}
+/**
+ * security_inode_post_set_acl() - Update inode security after set_acl()
+ * @dentry: file
+ * @acl_name: acl name
+ * @kacl: acl struct
+ *
+ * Update inode security field after successful set_acl operation on @dentry.
+ * The posix acls in @kacl are identified by @acl_name.
+ */
+void security_inode_post_set_acl(struct dentry *dentry, const char *acl_name,
+ struct posix_acl *kacl)
+{
+ if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
+ return;
+ call_void_hook(inode_post_set_acl, dentry, acl_name, kacl);
+}
+
/**
* security_inode_get_acl() - Check if reading posix acls is allowed
* @idmap: idmap of the mount
--
2.34.1
From: Roberto Sassu <[email protected]>
In preparation for moving IMA and EVM to the LSM infrastructure, introduce
the file_pre_free_security hook.
It is useful for IMA to calculate the digest of the file content, just
before a file descriptor is closed, and update the security.ima xattr with
the new value.
LSMs should use this hook instead of file_free_security, if they still need
to access the opened file, before it is closed. The new hook cannot return
an error and cannot cause the operation to be canceled.
Signed-off-by: Roberto Sassu <[email protected]>
---
fs/file_table.c | 1 +
include/linux/lsm_hook_defs.h | 1 +
include/linux/security.h | 4 ++++
security/security.c | 11 +++++++++++
4 files changed, 17 insertions(+)
diff --git a/fs/file_table.c b/fs/file_table.c
index fc7d677ff5ad..964e24120684 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -375,6 +375,7 @@ static void __fput(struct file *file)
eventpoll_release(file);
locks_remove_file(file);
+ security_file_pre_free(file);
ima_file_free(file);
if (unlikely(file->f_flags & FASYNC)) {
if (file->f_op->fasync)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 60ed33f0c80d..797f51da3f7d 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -172,6 +172,7 @@ LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir,
struct kernfs_node *kn)
LSM_HOOK(int, 0, file_permission, struct file *file, int mask)
LSM_HOOK(int, 0, file_alloc_security, struct file *file)
+LSM_HOOK(void, LSM_RET_VOID, file_pre_free_security, struct file *file)
LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
unsigned long arg)
diff --git a/include/linux/security.h b/include/linux/security.h
index a0f16511c059..7871009d59ae 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -389,6 +389,7 @@ int security_kernfs_init_security(struct kernfs_node *kn_dir,
struct kernfs_node *kn);
int security_file_permission(struct file *file, int mask);
int security_file_alloc(struct file *file);
+void security_file_pre_free(struct file *file);
void security_file_free(struct file *file);
int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
int security_mmap_file(struct file *file, unsigned long prot,
@@ -985,6 +986,9 @@ static inline int security_file_alloc(struct file *file)
return 0;
}
+static inline void security_file_pre_free(struct file *file)
+{ }
+
static inline void security_file_free(struct file *file)
{ }
diff --git a/security/security.c b/security/security.c
index 3e0078b51e46..fbb58eeeea02 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2626,6 +2626,17 @@ int security_file_alloc(struct file *file)
return rc;
}
+/**
+ * security_file_pre_free() - Perform actions before closing a file descriptor
+ * @file: the file
+ *
+ * Perform actions before the file descriptor is closed and freed.
+ */
+void security_file_pre_free(struct file *file)
+{
+ call_void_hook(file_pre_free_security, file);
+}
+
/**
* security_file_free() - Free a file's LSM blob
* @file: the file
--
2.34.1
From: Roberto Sassu <[email protected]>
As for IMA, remove hardcoded EVM function calls from the LSM infrastructure
and the VFS. Make EVM functions as static (except for
evm_inode_init_security(), which is exported), and register them as hook
implementations in init_evm_lsm(), called from integrity_lsm_init().
Finally, switch to the LSM reservation mechanism for the EVM xattr, and
consequently decrement by one the number of xattrs to allocate in
security_inode_init_security().
Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Casey Schaufler <[email protected]>
---
fs/attr.c | 2 -
fs/posix_acl.c | 3 -
fs/xattr.c | 2 -
include/linux/evm.h | 107 ------------------------------
security/integrity/evm/evm_main.c | 103 +++++++++++++++++++++++-----
security/integrity/iint.c | 7 ++
security/integrity/integrity.h | 9 +++
security/security.c | 42 +++---------
8 files changed, 113 insertions(+), 162 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index 63fb60195409..4153f83a4a1f 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -16,7 +16,6 @@
#include <linux/fcntl.h>
#include <linux/filelock.h>
#include <linux/security.h>
-#include <linux/evm.h>
#include "internal.h"
@@ -486,7 +485,6 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
if (!error) {
fsnotify_change(dentry, ia_valid);
security_inode_post_setattr(idmap, dentry, ia_valid);
- evm_inode_post_setattr(idmap, dentry, ia_valid);
}
return error;
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 2a2a2750b3e9..5cea0df45d3b 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -26,7 +26,6 @@
#include <linux/mnt_idmapping.h>
#include <linux/iversion.h>
#include <linux/security.h>
-#include <linux/evm.h>
#include <linux/fsnotify.h>
#include <linux/filelock.h>
@@ -1138,7 +1137,6 @@ int vfs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
if (!error) {
fsnotify_xattr(dentry);
security_inode_post_set_acl(dentry, acl_name, kacl);
- evm_inode_post_set_acl(dentry, acl_name, kacl);
}
out_inode_unlock:
@@ -1247,7 +1245,6 @@ int vfs_remove_acl(struct mnt_idmap *idmap, struct dentry *dentry,
if (!error) {
fsnotify_xattr(dentry);
security_inode_post_remove_acl(idmap, dentry, acl_name);
- evm_inode_post_remove_acl(idmap, dentry, acl_name);
}
out_inode_unlock:
diff --git a/fs/xattr.c b/fs/xattr.c
index 4a0280295686..4495e0b4d003 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -16,7 +16,6 @@
#include <linux/mount.h>
#include <linux/namei.h>
#include <linux/security.h>
-#include <linux/evm.h>
#include <linux/syscalls.h>
#include <linux/export.h>
#include <linux/fsnotify.h>
@@ -557,7 +556,6 @@ __vfs_removexattr_locked(struct mnt_idmap *idmap,
fsnotify_xattr(dentry);
security_inode_post_removexattr(dentry, name);
- evm_inode_post_removexattr(dentry, name);
out:
return error;
diff --git a/include/linux/evm.h b/include/linux/evm.h
index 642e52483adc..cb481eccc967 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -21,44 +21,6 @@ extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
void *xattr_value,
size_t xattr_value_len,
struct integrity_iint_cache *iint);
-extern int evm_inode_setattr(struct mnt_idmap *idmap,
- struct dentry *dentry, struct iattr *attr);
-void evm_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
- int ia_valid);
-extern int evm_inode_setxattr(struct mnt_idmap *idmap,
- struct dentry *dentry, const char *name,
- const void *value, size_t size, int flags);
-extern void evm_inode_post_setxattr(struct dentry *dentry,
- const char *xattr_name,
- const void *xattr_value,
- size_t xattr_value_len,
- int flags);
-extern int evm_inode_removexattr(struct mnt_idmap *idmap,
- struct dentry *dentry, const char *xattr_name);
-extern void evm_inode_post_removexattr(struct dentry *dentry,
- const char *xattr_name);
-static inline void evm_inode_post_remove_acl(struct mnt_idmap *idmap,
- struct dentry *dentry,
- const char *acl_name)
-{
- evm_inode_post_removexattr(dentry, acl_name);
-}
-extern int evm_inode_set_acl(struct mnt_idmap *idmap,
- struct dentry *dentry, const char *acl_name,
- struct posix_acl *kacl);
-static inline int evm_inode_remove_acl(struct mnt_idmap *idmap,
- struct dentry *dentry,
- const char *acl_name)
-{
- return evm_inode_set_acl(idmap, dentry, acl_name, NULL);
-}
-static inline void evm_inode_post_set_acl(struct dentry *dentry,
- const char *acl_name,
- struct posix_acl *kacl)
-{
- return evm_inode_post_setxattr(dentry, acl_name, NULL, 0, 0);
-}
-
int evm_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, struct xattr *xattrs,
int *xattr_count);
@@ -93,75 +55,6 @@ static inline enum integrity_status evm_verifyxattr(struct dentry *dentry,
}
#endif
-static inline int evm_inode_setattr(struct mnt_idmap *idmap,
- struct dentry *dentry, struct iattr *attr)
-{
- return 0;
-}
-
-static inline void evm_inode_post_setattr(struct mnt_idmap *idmap,
- struct dentry *dentry, int ia_valid)
-{
- return;
-}
-
-static inline int evm_inode_setxattr(struct mnt_idmap *idmap,
- struct dentry *dentry, const char *name,
- const void *value, size_t size, int flags)
-{
- return 0;
-}
-
-static inline void evm_inode_post_setxattr(struct dentry *dentry,
- const char *xattr_name,
- const void *xattr_value,
- size_t xattr_value_len,
- int flags)
-{
- return;
-}
-
-static inline int evm_inode_removexattr(struct mnt_idmap *idmap,
- struct dentry *dentry,
- const char *xattr_name)
-{
- return 0;
-}
-
-static inline void evm_inode_post_removexattr(struct dentry *dentry,
- const char *xattr_name)
-{
- return;
-}
-
-static inline void evm_inode_post_remove_acl(struct mnt_idmap *idmap,
- struct dentry *dentry,
- const char *acl_name)
-{
- return;
-}
-
-static inline int evm_inode_set_acl(struct mnt_idmap *idmap,
- struct dentry *dentry, const char *acl_name,
- struct posix_acl *kacl)
-{
- return 0;
-}
-
-static inline int evm_inode_remove_acl(struct mnt_idmap *idmap,
- struct dentry *dentry,
- const char *acl_name)
-{
- return 0;
-}
-
-static inline void evm_inode_post_set_acl(struct dentry *dentry,
- const char *acl_name,
- struct posix_acl *kacl)
-{
- return;
-}
-
static inline int evm_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
struct xattr *xattrs,
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 2e8f6d1c9984..adbb996e681d 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -567,9 +567,9 @@ static int evm_protect_xattr(struct mnt_idmap *idmap,
* userspace from writing HMAC value. Writing 'security.evm' requires
* requires CAP_SYS_ADMIN privileges.
*/
-int evm_inode_setxattr(struct mnt_idmap *idmap, struct dentry *dentry,
- const char *xattr_name, const void *xattr_value,
- size_t xattr_value_len, int flags)
+static int evm_inode_setxattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ const char *xattr_name, const void *xattr_value,
+ size_t xattr_value_len, int flags)
{
const struct evm_ima_xattr_data *xattr_data = xattr_value;
@@ -599,8 +599,8 @@ int evm_inode_setxattr(struct mnt_idmap *idmap, struct dentry *dentry,
* Removing 'security.evm' requires CAP_SYS_ADMIN privileges and that
* the current value is valid.
*/
-int evm_inode_removexattr(struct mnt_idmap *idmap,
- struct dentry *dentry, const char *xattr_name)
+static int evm_inode_removexattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ const char *xattr_name)
{
/* Policy permits modification of the protected xattrs even though
* there's no HMAC key loaded
@@ -650,9 +650,11 @@ static inline int evm_inode_set_acl_change(struct mnt_idmap *idmap,
* Prevent modifying posix acls causing the EVM HMAC to be re-calculated
* and 'security.evm' xattr updated, unless the existing 'security.evm' is
* valid.
+ *
+ * Return: zero on success, -EPERM on failure.
*/
-int evm_inode_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
- const char *acl_name, struct posix_acl *kacl)
+static int evm_inode_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
+ const char *acl_name, struct posix_acl *kacl)
{
enum integrity_status evm_status;
@@ -691,6 +693,24 @@ int evm_inode_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
return -EPERM;
}
+/**
+ * evm_inode_remove_acl - Protect the EVM extended attribute from posix acls
+ * @idmap: idmap of the mount
+ * @dentry: pointer to the affected dentry
+ * @acl_name: name of the posix acl
+ *
+ * Prevent removing posix acls causing the EVM HMAC to be re-calculated
+ * and 'security.evm' xattr updated, unless the existing 'security.evm' is
+ * valid.
+ *
+ * Return: zero on success, -EPERM on failure.
+ */
+static int evm_inode_remove_acl(struct mnt_idmap *idmap, struct dentry *dentry,
+ const char *acl_name)
+{
+ return evm_inode_set_acl(idmap, dentry, acl_name, NULL);
+}
+
static void evm_reset_status(struct inode *inode)
{
struct integrity_iint_cache *iint;
@@ -739,9 +759,11 @@ bool evm_revalidate_status(const char *xattr_name)
* __vfs_setxattr_noperm(). The caller of which has taken the inode's
* i_mutex lock.
*/
-void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
- const void *xattr_value, size_t xattr_value_len,
- int flags)
+static void evm_inode_post_setxattr(struct dentry *dentry,
+ const char *xattr_name,
+ const void *xattr_value,
+ size_t xattr_value_len,
+ int flags)
{
if (!evm_revalidate_status(xattr_name))
return;
@@ -757,6 +779,21 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
}
+/**
+ * evm_inode_post_set_acl - Update the EVM extended attribute from posix acls
+ * @dentry: pointer to the affected dentry
+ * @acl_name: name of the posix acl
+ * @kacl: pointer to the posix acls
+ *
+ * Update the 'security.evm' xattr with the EVM HMAC re-calculated after setting
+ * posix acls.
+ */
+static void evm_inode_post_set_acl(struct dentry *dentry, const char *acl_name,
+ struct posix_acl *kacl)
+{
+ return evm_inode_post_setxattr(dentry, acl_name, NULL, 0, 0);
+}
+
/**
* evm_inode_post_removexattr - update 'security.evm' after removing the xattr
* @dentry: pointer to the affected dentry
@@ -767,7 +804,8 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
* No need to take the i_mutex lock here, as this function is called from
* vfs_removexattr() which takes the i_mutex.
*/
-void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
+static void evm_inode_post_removexattr(struct dentry *dentry,
+ const char *xattr_name)
{
if (!evm_revalidate_status(xattr_name))
return;
@@ -783,6 +821,22 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
evm_update_evmxattr(dentry, xattr_name, NULL, 0);
}
+/**
+ * evm_inode_post_remove_acl - Update the EVM extended attribute from posix acls
+ * @idmap: idmap of the mount
+ * @dentry: pointer to the affected dentry
+ * @acl_name: name of the posix acl
+ *
+ * Update the 'security.evm' xattr with the EVM HMAC re-calculated after
+ * removing posix acls.
+ */
+static inline void evm_inode_post_remove_acl(struct mnt_idmap *idmap,
+ struct dentry *dentry,
+ const char *acl_name)
+{
+ evm_inode_post_removexattr(dentry, acl_name);
+}
+
static int evm_attr_change(struct mnt_idmap *idmap,
struct dentry *dentry, struct iattr *attr)
{
@@ -806,8 +860,8 @@ static int evm_attr_change(struct mnt_idmap *idmap,
* Permit update of file attributes when files have a valid EVM signature,
* except in the case of them having an immutable portable signature.
*/
-int evm_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
- struct iattr *attr)
+static int evm_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ struct iattr *attr)
{
unsigned int ia_valid = attr->ia_valid;
enum integrity_status evm_status;
@@ -854,8 +908,8 @@ int evm_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
* This function is called from notify_change(), which expects the caller
* to lock the inode's i_mutex.
*/
-void evm_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
- int ia_valid)
+static void evm_inode_post_setattr(struct mnt_idmap *idmap,
+ struct dentry *dentry, int ia_valid)
{
if (!evm_revalidate_status(NULL))
return;
@@ -965,4 +1019,23 @@ static int __init init_evm(void)
return error;
}
+static struct security_hook_list evm_hooks[] __ro_after_init = {
+ LSM_HOOK_INIT(inode_setattr, evm_inode_setattr),
+ LSM_HOOK_INIT(inode_post_setattr, evm_inode_post_setattr),
+ LSM_HOOK_INIT(inode_setxattr, evm_inode_setxattr),
+ LSM_HOOK_INIT(inode_set_acl, evm_inode_set_acl),
+ LSM_HOOK_INIT(inode_post_set_acl, evm_inode_post_set_acl),
+ LSM_HOOK_INIT(inode_remove_acl, evm_inode_remove_acl),
+ LSM_HOOK_INIT(inode_post_remove_acl, evm_inode_post_remove_acl),
+ LSM_HOOK_INIT(inode_post_setxattr, evm_inode_post_setxattr),
+ LSM_HOOK_INIT(inode_removexattr, evm_inode_removexattr),
+ LSM_HOOK_INIT(inode_post_removexattr, evm_inode_post_removexattr),
+ LSM_HOOK_INIT(inode_init_security, evm_inode_init_security),
+};
+
+void __init init_evm_lsm(void)
+{
+ security_add_hooks(evm_hooks, ARRAY_SIZE(evm_hooks), "integrity");
+}
+
late_initcall(init_evm);
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 32f0f3c5c4dd..dd03f978b45c 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -174,12 +174,19 @@ static int __init integrity_lsm_init(void)
0, SLAB_PANIC, init_once);
init_ima_lsm();
+ init_evm_lsm();
return 0;
}
+
+struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
+ .lbs_xattr_count = 1,
+};
+
DEFINE_LSM(integrity) = {
.name = "integrity",
.init = integrity_lsm_init,
.order = LSM_ORDER_LAST,
+ .blobs = &integrity_blob_sizes,
};
/*
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 7adc7d6c4f9f..83a465ac9013 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -189,6 +189,7 @@ 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;
struct modsig;
@@ -200,6 +201,14 @@ static inline void __init init_ima_lsm(void)
}
#endif
+#ifdef CONFIG_EVM
+void __init init_evm_lsm(void);
+#else
+static inline void __init init_evm_lsm(void)
+{
+}
+#endif
+
#ifdef CONFIG_INTEGRITY_SIGNATURE
int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
diff --git a/security/security.c b/security/security.c
index eb686f9471ea..5e700557e434 100644
--- a/security/security.c
+++ b/security/security.c
@@ -20,13 +20,13 @@
#include <linux/kernel_read_file.h>
#include <linux/lsm_hooks.h>
#include <linux/integrity.h>
-#include <linux/evm.h>
#include <linux/fsnotify.h>
#include <linux/mman.h>
#include <linux/mount.h>
#include <linux/personality.h>
#include <linux/backing-dev.h>
#include <linux/string.h>
+#include <linux/xattr.h>
#include <linux/msg.h>
#include <net/flow.h>
@@ -1616,8 +1616,8 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
return 0;
if (initxattrs) {
- /* Allocate +1 for EVM and +1 as terminator. */
- new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 2,
+ /* Allocate +1 as terminator. */
+ new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1,
sizeof(*new_xattrs), GFP_NOFS);
if (!new_xattrs)
return -ENOMEM;
@@ -1641,10 +1641,6 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
if (!xattr_count)
goto out;
- ret = evm_inode_init_security(inode, dir, qstr, new_xattrs,
- &xattr_count);
- if (ret)
- goto out;
ret = initxattrs(inode, new_xattrs, fs_data);
out:
for (; xattr_count > 0; xattr_count--)
@@ -2144,14 +2140,9 @@ int security_inode_permission(struct inode *inode, int mask)
int security_inode_setattr(struct mnt_idmap *idmap,
struct dentry *dentry, struct iattr *attr)
{
- int ret;
-
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
- ret = call_int_hook(inode_setattr, 0, idmap, dentry, attr);
- if (ret)
- return ret;
- return evm_inode_setattr(idmap, dentry, attr);
+ return call_int_hook(inode_setattr, 0, idmap, dentry, attr);
}
EXPORT_SYMBOL_GPL(security_inode_setattr);
@@ -2216,9 +2207,7 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
if (ret == 1)
ret = cap_inode_setxattr(dentry, name, value, size, flags);
- if (ret)
- return ret;
- return evm_inode_setxattr(idmap, dentry, name, value, size, flags);
+ return ret;
}
/**
@@ -2237,15 +2226,10 @@ int security_inode_set_acl(struct mnt_idmap *idmap,
struct dentry *dentry, const char *acl_name,
struct posix_acl *kacl)
{
- int ret;
-
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
- ret = call_int_hook(inode_set_acl, 0, idmap, dentry, acl_name,
- kacl);
- if (ret)
- return ret;
- return evm_inode_set_acl(idmap, dentry, acl_name, kacl);
+ return call_int_hook(inode_set_acl, 0, idmap, dentry, acl_name,
+ kacl);
}
/**
@@ -2298,14 +2282,9 @@ int security_inode_get_acl(struct mnt_idmap *idmap,
int security_inode_remove_acl(struct mnt_idmap *idmap,
struct dentry *dentry, const char *acl_name)
{
- int ret;
-
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
- ret = call_int_hook(inode_remove_acl, 0, idmap, dentry, acl_name);
- if (ret)
- return ret;
- return evm_inode_remove_acl(idmap, dentry, acl_name);
+ return call_int_hook(inode_remove_acl, 0, idmap, dentry, acl_name);
}
/**
@@ -2341,7 +2320,6 @@ void security_inode_post_setxattr(struct dentry *dentry, const char *name,
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return;
call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
- evm_inode_post_setxattr(dentry, name, value, size, flags);
}
/**
@@ -2402,9 +2380,7 @@ int security_inode_removexattr(struct mnt_idmap *idmap,
ret = call_int_hook(inode_removexattr, 1, idmap, dentry, name);
if (ret == 1)
ret = cap_inode_removexattr(idmap, dentry, name);
- if (ret)
- return ret;
- return evm_inode_removexattr(idmap, dentry, name);
+ return ret;
}
/**
--
2.34.1
On 9/4/23 09:40, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> 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. Thus, request from the 'integrity' LSM a
> space in the security blob for the pointer of inode metadata
> (integrity_iint_cache structure).
>
> Prefer this 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 <[email protected]>
> Reviewed-by: Casey Schaufler <[email protected]>
> ---
>
> @@ -145,10 +91,8 @@ static void integrity_inode_free(struct inode *inode)
> if (!IS_IMA(inode))
> return;
I think you can remove this check !IS_IMA()Â as well since the next
function called here integrity_iint_find() already has this check:
struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
{
       if (!IS_IMA(inode))
               return NULL;
       return integrity_inode_get_iint(inode);
}
>
> - 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);
> }
On Tue, 2023-09-12 at 12:19 -0400, Stefan Berger wrote:
> On 9/4/23 09:40, Roberto Sassu wrote:
> > From: Roberto Sassu <[email protected]>
> >
> > 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. Thus, request from the 'integrity' LSM a
> > space in the security blob for the pointer of inode metadata
> > (integrity_iint_cache structure).
> >
> > Prefer this 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 <[email protected]>
> > Reviewed-by: Casey Schaufler <[email protected]>
> > ---
> >
> > @@ -145,10 +91,8 @@ static void integrity_inode_free(struct inode *inode)
> > if (!IS_IMA(inode))
> > return;
>
> I think you can remove this check !IS_IMA()Â as well since the next
> function called here integrity_iint_find() already has this check:
>
> struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
> {
> Â Â Â Â Â Â Â if (!IS_IMA(inode))
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return NULL;
>
> Â Â Â Â Â Â Â return integrity_inode_get_iint(inode);
> }
I agree, thanks!
Roberto
> >
> > - 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);
> > }
On Mon, 2023-09-04 at 15:34 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> the inode_post_setattr hook.
>
> It is useful for EVM to recalculate the HMAC on modified file attributes
> and other file metadata, after it verified the HMAC of current file
> metadata with the inode_setattr hook.
>
> LSMs should use the new hook instead of inode_setattr, when they need to
> know that the operation was done successfully (not known in inode_setattr).
> The new hook cannot return an error and cannot cause the operation to be
> reverted.
>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> fs/attr.c | 1 +
Hi Christian, Al
could you please review and ack patches 12-19, which touch the VFS?
Thanks a lot!
Roberto
> include/linux/lsm_hook_defs.h | 2 ++
> include/linux/security.h | 7 +++++++
> security/security.c | 16 ++++++++++++++++
> 4 files changed, 26 insertions(+)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 431f667726c7..3c309eb456c6 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -486,6 +486,7 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
>
> if (!error) {
> fsnotify_change(dentry, ia_valid);
> + security_inode_post_setattr(idmap, dentry, ia_valid);
> ima_inode_post_setattr(idmap, dentry, ia_valid);
> evm_inode_post_setattr(idmap, dentry, ia_valid);
> }
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index fdf075a6b1bb..995d30336cfa 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -136,6 +136,8 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
> LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
> LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry,
> struct iattr *attr)
> +LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap,
> + struct dentry *dentry, int ia_valid)
> LSM_HOOK(int, 0, inode_getattr, const struct path *path)
> LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
> struct dentry *dentry, const char *name, const void *value,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index dcb3604ffab8..820899db5276 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -355,6 +355,8 @@ int security_inode_follow_link(struct dentry *dentry, struct inode *inode,
> int security_inode_permission(struct inode *inode, int mask);
> int security_inode_setattr(struct mnt_idmap *idmap,
> struct dentry *dentry, struct iattr *attr);
> +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> + int ia_valid);
> int security_inode_getattr(const struct path *path);
> int security_inode_setxattr(struct mnt_idmap *idmap,
> struct dentry *dentry, const char *name,
> @@ -856,6 +858,11 @@ static inline int security_inode_setattr(struct mnt_idmap *idmap,
> return 0;
> }
>
> +static inline void
> +security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> + int ia_valid)
> +{ }
> +
> static inline int security_inode_getattr(const struct path *path)
> {
> return 0;
> diff --git a/security/security.c b/security/security.c
> index 2b24d01cf181..764a6f28b3b9 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2124,6 +2124,22 @@ int security_inode_setattr(struct mnt_idmap *idmap,
> }
> EXPORT_SYMBOL_GPL(security_inode_setattr);
>
> +/**
> + * security_inode_post_setattr() - Update the inode after a setattr operation
> + * @idmap: idmap of the mount
> + * @dentry: file
> + * @ia_valid: file attributes set
> + *
> + * Update inode security field after successful setting file attributes.
> + */
> +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> + int ia_valid)
> +{
> + if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> + return;
> + call_void_hook(inode_post_setattr, idmap, dentry, ia_valid);
> +}
> +
> /**
> * security_inode_getattr() - Check if getting file attributes is allowed
> * @path: file
On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> Change ima_post_path_mknod() definition, so that it can be registered as
> implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> well.
> Also, make sure that ima_post_path_mknod() is executed only if
> (mode & S_IFMT) is equal to zero or S_IFREG.
>
> Add this check to take into account the different placement of the
> path_post_mknod hook (to be introduced) in do_mknodat().
Move "(to be introduced)" to when it is first mentioned.
> Since the new hook
> will be placed after the switch(), the check ensures that
> ima_post_path_mknod() is invoked as originally intended when it is
> registered as implementation of path_post_mknod.
>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> fs/namei.c | 9 ++++++---
> include/linux/ima.h | 7 +++++--
> security/integrity/ima/ima_main.c | 10 +++++++++-
> 3 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index e56ff39a79bc..c5e96f716f98 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> struct path path;
> int error;
> unsigned int lookup_flags = 0;
> + umode_t mode_stripped;
>
> error = may_mknod(mode);
> if (error)
> @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> if (IS_ERR(dentry))
> goto out1;
>
> - error = security_path_mknod(&path, dentry,
> - mode_strip_umask(path.dentry->d_inode, mode), dev);
> + mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> +
> + error = security_path_mknod(&path, dentry, mode_stripped, dev);
> if (error)
> goto out2;
>
> @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> error = vfs_create(idmap, path.dentry->d_inode,
> dentry, mode, true);
> if (!error)
> - ima_post_path_mknod(idmap, dentry);
> + ima_post_path_mknod(idmap, &path, dentry,
> + mode_stripped, dev);
> break;
> case S_IFCHR: case S_IFBLK:
> error = vfs_mknod(idmap, path.dentry->d_inode,
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 910a2f11a906..179ce52013b2 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> enum kernel_read_file_id id);
> extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> - struct dentry *dentry);
> + const struct path *dir, struct dentry *dentry,
> + umode_t mode, unsigned int dev);
> extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> }
>
> static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> - struct dentry *dentry)
> + const struct path *dir,
> + struct dentry *dentry,
> + umode_t mode, unsigned int dev)
> {
> return;
> }
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 365db0e43d7c..76eba92d7f10 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> /**
> * ima_post_path_mknod - mark as a new inode
> * @idmap: idmap of the mount the inode was found from
> + * @dir: path structure of parent of the new file
> * @dentry: newly created dentry
> + * @mode: mode of the new file
> + * @dev: undecoded device number
> *
> * Mark files created via the mknodat syscall as new, so that the
> * file data can be written later.
> */
> void ima_post_path_mknod(struct mnt_idmap *idmap,
> - struct dentry *dentry)
> + const struct path *dir, struct dentry *dentry,
> + umode_t mode, unsigned int dev)
> {
> struct integrity_iint_cache *iint;
> struct inode *inode = dentry->d_inode;
> int must_appraise;
>
> + /* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> + if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> + return;
> +
There's already a check below to make sure that this is a regular file.
Are both needed?
> if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> return;
>
On Wed, 2023-10-11 at 10:51 -0400, Mimi Zohar wrote:
> On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > From: Roberto Sassu <[email protected]>
> >
> > Change ima_file_mprotect() definition, so that it can be registered
> > as implementation of the file_mprotect hook.
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
> > Reviewed-by: Stefan Berger <[email protected]>
> > ---
> > include/linux/ima.h | 5 +++--
> > security/integrity/ima/ima_main.c | 6 ++++--
> > security/security.c | 2 +-
> > 3 files changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 893c3b98b4d0..56e72c0beb96 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -24,7 +24,8 @@ extern void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > extern void ima_file_free(struct file *file);
> > extern int ima_file_mmap(struct file *file, unsigned long reqprot,
> > unsigned long prot, unsigned long flags);
> > -extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot);
> > +int ima_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> > + unsigned long prot);
>
> "extern" is needed here and similarly in 5/25.
I removed because of a complain from checkpatch.pl --strict.
Roberto
> Mimi
>
> > extern int ima_load_data(enum kernel_load_data_id id, bool contents);
> > extern int ima_post_load_data(char *buf, loff_t size,
> > enum kernel_load_data_id id, char *description);
> > @@ -88,7 +89,7 @@ static inline int ima_file_mmap(struct file *file, unsigned long reqprot,
> > }
> >
> > static inline int ima_file_mprotect(struct vm_area_struct *vma,
> > - unsigned long prot)
> > + unsigned long reqprot, unsigned long prot)
> > {
> > return 0;
> > }
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 52e742d32f4b..e9e2a3ad25a1 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -441,7 +441,8 @@ int ima_file_mmap(struct file *file, unsigned long reqprot,
> > /**
> > * ima_file_mprotect - based on policy, limit mprotect change
> > * @vma: vm_area_struct protection is set to
> > - * @prot: contains the protection that will be applied by the kernel.
> > + * @reqprot: protection requested by the application
> > + * @prot: protection that will be applied by the kernel
> > *
> > * Files can be mmap'ed read/write and later changed to execute to circumvent
> > * IMA's mmap appraisal policy rules. Due to locking issues (mmap semaphore
> > @@ -451,7 +452,8 @@ int ima_file_mmap(struct file *file, unsigned long reqprot,
> > *
> > * On mprotect change success, return 0. On failure, return -EACESS.
> > */
> > -int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
> > +int ima_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> > + unsigned long prot)
> > {
> > struct ima_template_desc *template = NULL;
> > struct file *file;
> > diff --git a/security/security.c b/security/security.c
> > index 96f2c68a1571..dffb67e6e119 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -2721,7 +2721,7 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> > ret = call_int_hook(file_mprotect, 0, vma, reqprot, prot);
> > if (ret)
> > return ret;
> > - return ima_file_mprotect(vma, prot);
> > + return ima_file_mprotect(vma, reqprot, prot);
> > }
> >
> > /**
>
On Wed, 2023-10-11 at 10:38 -0400, Mimi Zohar wrote:
> On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > From: Roberto Sassu <[email protected]>
> >
> > Change ima_post_path_mknod() definition, so that it can be registered as
> > implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> > mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> > well.
> > Also, make sure that ima_post_path_mknod() is executed only if
> > (mode & S_IFMT) is equal to zero or S_IFREG.
> >
> > Add this check to take into account the different placement of the
> > path_post_mknod hook (to be introduced) in do_mknodat().
>
> Move "(to be introduced)" to when it is first mentioned.
>
> > Since the new hook
> > will be placed after the switch(), the check ensures that
> > ima_post_path_mknod() is invoked as originally intended when it is
> > registered as implementation of path_post_mknod.
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
> > ---
> > fs/namei.c | 9 ++++++---
> > include/linux/ima.h | 7 +++++--
> > security/integrity/ima/ima_main.c | 10 +++++++++-
> > 3 files changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index e56ff39a79bc..c5e96f716f98 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > struct path path;
> > int error;
> > unsigned int lookup_flags = 0;
> > + umode_t mode_stripped;
> >
> > error = may_mknod(mode);
> > if (error)
> > @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > if (IS_ERR(dentry))
> > goto out1;
> >
> > - error = security_path_mknod(&path, dentry,
> > - mode_strip_umask(path.dentry->d_inode, mode), dev);
> > + mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> > +
> > + error = security_path_mknod(&path, dentry, mode_stripped, dev);
> > if (error)
> > goto out2;
> >
> > @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > error = vfs_create(idmap, path.dentry->d_inode,
> > dentry, mode, true);
> > if (!error)
> > - ima_post_path_mknod(idmap, dentry);
> > + ima_post_path_mknod(idmap, &path, dentry,
> > + mode_stripped, dev);
> > break;
> > case S_IFCHR: case S_IFBLK:
> > error = vfs_mknod(idmap, path.dentry->d_inode,
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 910a2f11a906..179ce52013b2 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> > extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > enum kernel_read_file_id id);
> > extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> > - struct dentry *dentry);
> > + const struct path *dir, struct dentry *dentry,
> > + umode_t mode, unsigned int dev);
> > extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> > extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > }
> >
> > static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> > - struct dentry *dentry)
> > + const struct path *dir,
> > + struct dentry *dentry,
> > + umode_t mode, unsigned int dev)
> > {
> > return;
> > }
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 365db0e43d7c..76eba92d7f10 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > /**
> > * ima_post_path_mknod - mark as a new inode
> > * @idmap: idmap of the mount the inode was found from
> > + * @dir: path structure of parent of the new file
> > * @dentry: newly created dentry
> > + * @mode: mode of the new file
> > + * @dev: undecoded device number
> > *
> > * Mark files created via the mknodat syscall as new, so that the
> > * file data can be written later.
> > */
> > void ima_post_path_mknod(struct mnt_idmap *idmap,
> > - struct dentry *dentry)
> > + const struct path *dir, struct dentry *dentry,
> > + umode_t mode, unsigned int dev)
> > {
> > struct integrity_iint_cache *iint;
> > struct inode *inode = dentry->d_inode;
> > int must_appraise;
> >
> > + /* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> > + if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> > + return;
> > +
>
> There's already a check below to make sure that this is a regular file.
> Are both needed?
You are right, I can remove the first check.
Thanks
Roberto
> > if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> > return;
> >
On Wed, 2023-10-11 at 17:43 +0200, Roberto Sassu wrote:
> On Wed, 2023-10-11 at 10:51 -0400, Mimi Zohar wrote:
> > On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > > From: Roberto Sassu <[email protected]>
> > >
> > > Change ima_file_mprotect() definition, so that it can be registered
> > > as implementation of the file_mprotect hook.
> > >
> > > Signed-off-by: Roberto Sassu <[email protected]>
> > > Reviewed-by: Stefan Berger <[email protected]>
> > > ---
> > > include/linux/ima.h | 5 +++--
> > > security/integrity/ima/ima_main.c | 6 ++++--
> > > security/security.c | 2 +-
> > > 3 files changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > index 893c3b98b4d0..56e72c0beb96 100644
> > > --- a/include/linux/ima.h
> > > +++ b/include/linux/ima.h
> > > @@ -24,7 +24,8 @@ extern void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > > extern void ima_file_free(struct file *file);
> > > extern int ima_file_mmap(struct file *file, unsigned long reqprot,
> > > unsigned long prot, unsigned long flags);
> > > -extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot);
> > > +int ima_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> > > + unsigned long prot);
> >
> > "extern" is needed here and similarly in 5/25.
>
> I removed because of a complain from checkpatch.pl --strict.
Intermixing with/without "extern" looks weird. I would suggest
removing all the externs as a separate patch, but they're being removed
in "[PATCH v3 21/25] ima: Move to LSM infrastructure" anyway. For now
I would include the "extern".
--
thanks,
Mimi
On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> Change ima_post_read_file() definition, so that it can be registered as
> implementation of the post_read_file hook.
The only change here is making "void *buf" a "char *buf".
thanks,
Mimi
> Signed-off-by: Roberto Sassu <[email protected]>
> Reviewed-by: Stefan Berger <[email protected]>
> ---
> include/linux/ima.h | 6 +++---
> security/integrity/ima/ima_main.c | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 93e3c6cdf1f8..6e4d060ff378 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -31,8 +31,8 @@ extern int ima_post_load_data(char *buf, loff_t size,
> enum kernel_load_data_id id, char *description);
> extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> bool contents);
> -extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> - enum kernel_read_file_id id);
> +int ima_post_read_file(struct file *file, char *buf, loff_t size,
> + enum kernel_read_file_id id);
> extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> const struct path *dir, struct dentry *dentry,
> umode_t mode, unsigned int dev);
> @@ -112,7 +112,7 @@ static inline int ima_read_file(struct file *file, enum kernel_read_file_id id,
> return 0;
> }
>
> -static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> +static inline int ima_post_read_file(struct file *file, char *buf, loff_t size,
> enum kernel_read_file_id id)
> {
> return 0;
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index e9e2a3ad25a1..f8581032e62c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -801,7 +801,7 @@ const int read_idmap[READING_MAX_ID] = {
> * On success return 0. On integrity appraisal error, assuming the file
> * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
> */
> -int ima_post_read_file(struct file *file, void *buf, loff_t size,
> +int ima_post_read_file(struct file *file, char *buf, loff_t size,
> enum kernel_read_file_id read_id)
> {
> enum ima_hooks func;
On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> Change evm_inode_post_setattr() definition, so that it can be registered as
> implementation of the inode_post_setattr hook.
Refer to comments on 1/25.
>
> Signed-off-by: Roberto Sassu <[email protected]>
> Reviewed-by: Stefan Berger <[email protected]>
--
thanks,
Mimi
gOn Mon, 2023-09-04 at 15:34 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> the inode_post_setattr hook.
>
> It is useful for EVM to recalculate the HMAC on modified file attributes
> and other file metadata, after it verified the HMAC of current file
> metadata with the inode_setattr hook.
"useful"?
At inode_setattr hook, EVM verifies the file's existing HMAC value. At
inode_post_setattr, EVM re-calculates the file's HMAC based on the
modified file attributes and other file metadata.
>
> LSMs should use the new hook instead of inode_setattr, when they need to
> know that the operation was done successfully (not known in inode_setattr).
> The new hook cannot return an error and cannot cause the operation to be
> reverted.
Other LSMs could similarly update security xattrs or ...
>
> Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Mimi Zohar <[email protected]>
On Thu, 2023-10-12 at 09:29 +0200, Roberto Sassu wrote:
> On Wed, 2023-10-11 at 15:01 -0400, Mimi Zohar wrote:
> > On Wed, 2023-10-11 at 18:02 +0200, Roberto Sassu wrote:
> > > On Wed, 2023-10-11 at 10:38 -0400, Mimi Zohar wrote:
> > > > On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > > > > From: Roberto Sassu <[email protected]>
> > > > >
> > > > > Change ima_post_path_mknod() definition, so that it can be registered as
> > > > > implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> > > > > mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> > > > > well.
> > > > > Also, make sure that ima_post_path_mknod() is executed only if
> > > > > (mode & S_IFMT) is equal to zero or S_IFREG.
> > > > >
> > > > > Add this check to take into account the different placement of the
> > > > > path_post_mknod hook (to be introduced) in do_mknodat().
> > > >
> > > > Move "(to be introduced)" to when it is first mentioned.
> > > >
> > > > > Since the new hook
> > > > > will be placed after the switch(), the check ensures that
> > > > > ima_post_path_mknod() is invoked as originally intended when it is
> > > > > registered as implementation of path_post_mknod.
> > > > >
> > > > > Signed-off-by: Roberto Sassu <[email protected]>
> > > > > ---
> > > > > fs/namei.c | 9 ++++++---
> > > > > include/linux/ima.h | 7 +++++--
> > > > > security/integrity/ima/ima_main.c | 10 +++++++++-
> > > > > 3 files changed, 20 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > > index e56ff39a79bc..c5e96f716f98 100644
> > > > > --- a/fs/namei.c
> > > > > +++ b/fs/namei.c
> > > > > @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > struct path path;
> > > > > int error;
> > > > > unsigned int lookup_flags = 0;
> > > > > + umode_t mode_stripped;
> > > > >
> > > > > error = may_mknod(mode);
> > > > > if (error)
> > > > > @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > if (IS_ERR(dentry))
> > > > > goto out1;
> > > > >
> > > > > - error = security_path_mknod(&path, dentry,
> > > > > - mode_strip_umask(path.dentry->d_inode, mode), dev);
> > > > > + mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> > > > > +
> > > > > + error = security_path_mknod(&path, dentry, mode_stripped, dev);
> > > > > if (error)
> > > > > goto out2;
> > > > >
> > > > > @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > error = vfs_create(idmap, path.dentry->d_inode,
> > > > > dentry, mode, true);
> > > > > if (!error)
> > > > > - ima_post_path_mknod(idmap, dentry);
> > > > > + ima_post_path_mknod(idmap, &path, dentry,
> > > > > + mode_stripped, dev);
> > > > > break;
> > > > > case S_IFCHR: case S_IFBLK:
> > > > > error = vfs_mknod(idmap, path.dentry->d_inode,
> > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > > > index 910a2f11a906..179ce52013b2 100644
> > > > > --- a/include/linux/ima.h
> > > > > +++ b/include/linux/ima.h
> > > > > @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> > > > > extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > > enum kernel_read_file_id id);
> > > > > extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > - struct dentry *dentry);
> > > > > + const struct path *dir, struct dentry *dentry,
> > > > > + umode_t mode, unsigned int dev);
> > > > > extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > > > > extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> > > > > extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > > > > @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > > }
> > > > >
> > > > > static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > - struct dentry *dentry)
> > > > > + const struct path *dir,
> > > > > + struct dentry *dentry,
> > > > > + umode_t mode, unsigned int dev)
> > > > > {
> > > > > return;
> > > > > }
> > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > > index 365db0e43d7c..76eba92d7f10 100644
> > > > > --- a/security/integrity/ima/ima_main.c
> > > > > +++ b/security/integrity/ima/ima_main.c
> > > > > @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > > > > /**
> > > > > * ima_post_path_mknod - mark as a new inode
> > > > > * @idmap: idmap of the mount the inode was found from
> > > > > + * @dir: path structure of parent of the new file
> > > > > * @dentry: newly created dentry
> > > > > + * @mode: mode of the new file
> > > > > + * @dev: undecoded device number
> > > > > *
> > > > > * Mark files created via the mknodat syscall as new, so that the
> > > > > * file data can be written later.
> > > > > */
> > > > > void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > - struct dentry *dentry)
> > > > > + const struct path *dir, struct dentry *dentry,
> > > > > + umode_t mode, unsigned int dev)
> > > > > {
> > > > > struct integrity_iint_cache *iint;
> > > > > struct inode *inode = dentry->d_inode;
> > > > > int must_appraise;
> > > > >
> > > > > + /* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> > > > > + if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> > > > > + return;
> > > > > +
> > > >
> > > > There's already a check below to make sure that this is a regular file.
> > > > Are both needed?
> > >
> > > You are right, I can remove the first check.
> >
> > The question then becomes why modify hook the arguments?
>
> We need to make sure that ima_post_path_mknod() has the same parameters
> as the LSM hook at the time we register it to the LSM infrastructure.
I'm trying to understand why the pre hook parameters and the missing
IMA parameter are used, as opposed to just defining the new
post_path_mknod hook like IMA.
thanks,
Mimi
>
> > >
> > > > > if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> > > > > return;
> > > > >
> > >
> >
>
On Thu, 2023-10-12 at 07:43 -0400, Mimi Zohar wrote:
> On Thu, 2023-10-12 at 09:42 +0200, Roberto Sassu wrote:
> > On Wed, 2023-10-11 at 20:08 -0400, Mimi Zohar wrote:
> > > gOn Mon, 2023-09-04 at 15:34 +0200, Roberto Sassu wrote:
> > > > From: Roberto Sassu <[email protected]>
> > > >
> > > > In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> > > > the inode_post_setattr hook.
> > > >
> > > > It is useful for EVM to recalculate the HMAC on modified file attributes
> > > > and other file metadata, after it verified the HMAC of current file
> > > > metadata with the inode_setattr hook.
> > >
> > > "useful"?
> > >
> > > At inode_setattr hook, EVM verifies the file's existing HMAC value. At
> > > inode_post_setattr, EVM re-calculates the file's HMAC based on the
> > > modified file attributes and other file metadata.
> > >
> > > >
> > > > LSMs should use the new hook instead of inode_setattr, when they need to
> > > > know that the operation was done successfully (not known in inode_setattr).
> > > > The new hook cannot return an error and cannot cause the operation to be
> > > > reverted.
> > >
> > > Other LSMs could similarly update security xattrs or ...
> >
> > I added your sentence. The one above is to satisfy Casey's request to
> > justify the addition of the new hook, and to explain why inode_setattr
> > is not sufficient.
>
> I was suggesting simplifying the wording. Perhaps something like:
>
> Other LSMs could similarly take some action after successful file attri
> bute change.
Ok, will use that.
Thanks
Roberto
On Thu, 2023-10-12 at 09:25 -0400, Mimi Zohar wrote:
> On Thu, 2023-10-12 at 14:19 +0200, Roberto Sassu wrote:
> > On Thu, 2023-10-12 at 07:42 -0400, Mimi Zohar wrote:
> > > On Thu, 2023-10-12 at 09:29 +0200, Roberto Sassu wrote:
> > > > On Wed, 2023-10-11 at 15:01 -0400, Mimi Zohar wrote:
> > > > > On Wed, 2023-10-11 at 18:02 +0200, Roberto Sassu wrote:
> > > > > > On Wed, 2023-10-11 at 10:38 -0400, Mimi Zohar wrote:
> > > > > > > On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> > > > > > > > From: Roberto Sassu <[email protected]>
> > > > > > > >
> > > > > > > > Change ima_post_path_mknod() definition, so that it can be registered as
> > > > > > > > implementation of the path_post_mknod hook. Since LSMs see a umask-stripped
> > > > > > > > mode from security_path_mknod(), pass the same to ima_post_path_mknod() as
> > > > > > > > well.
> > > > > > > > Also, make sure that ima_post_path_mknod() is executed only if
> > > > > > > > (mode & S_IFMT) is equal to zero or S_IFREG.
> > > > > > > >
> > > > > > > > Add this check to take into account the different placement of the
> > > > > > > > path_post_mknod hook (to be introduced) in do_mknodat().
> > > > > > >
> > > > > > > Move "(to be introduced)" to when it is first mentioned.
> > > > > > >
> > > > > > > > Since the new hook
> > > > > > > > will be placed after the switch(), the check ensures that
> > > > > > > > ima_post_path_mknod() is invoked as originally intended when it is
> > > > > > > > registered as implementation of path_post_mknod.
> > > > > > > >
> > > > > > > > Signed-off-by: Roberto Sassu <[email protected]>
> > > > > > > > ---
> > > > > > > > fs/namei.c | 9 ++++++---
> > > > > > > > include/linux/ima.h | 7 +++++--
> > > > > > > > security/integrity/ima/ima_main.c | 10 +++++++++-
> > > > > > > > 3 files changed, 20 insertions(+), 6 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > > > > > index e56ff39a79bc..c5e96f716f98 100644
> > > > > > > > --- a/fs/namei.c
> > > > > > > > +++ b/fs/namei.c
> > > > > > > > @@ -4024,6 +4024,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > > > > struct path path;
> > > > > > > > int error;
> > > > > > > > unsigned int lookup_flags = 0;
> > > > > > > > + umode_t mode_stripped;
> > > > > > > >
> > > > > > > > error = may_mknod(mode);
> > > > > > > > if (error)
> > > > > > > > @@ -4034,8 +4035,9 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > > > > if (IS_ERR(dentry))
> > > > > > > > goto out1;
> > > > > > > >
> > > > > > > > - error = security_path_mknod(&path, dentry,
> > > > > > > > - mode_strip_umask(path.dentry->d_inode, mode), dev);
> > > > > > > > + mode_stripped = mode_strip_umask(path.dentry->d_inode, mode);
> > > > > > > > +
> > > > > > > > + error = security_path_mknod(&path, dentry, mode_stripped, dev);
> > > > > > > > if (error)
> > > > > > > > goto out2;
> > > > > > > >
> > > > > > > > @@ -4045,7 +4047,8 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
> > > > > > > > error = vfs_create(idmap, path.dentry->d_inode,
> > > > > > > > dentry, mode, true);
> > > > > > > > if (!error)
> > > > > > > > - ima_post_path_mknod(idmap, dentry);
> > > > > > > > + ima_post_path_mknod(idmap, &path, dentry,
> > > > > > > > + mode_stripped, dev);
> > > > > > > > break;
> > > > > > > > case S_IFCHR: case S_IFBLK:
> > > > > > > > error = vfs_mknod(idmap, path.dentry->d_inode,
> > > > > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > > > > > > index 910a2f11a906..179ce52013b2 100644
> > > > > > > > --- a/include/linux/ima.h
> > > > > > > > +++ b/include/linux/ima.h
> > > > > > > > @@ -32,7 +32,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
> > > > > > > > extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > > > > > enum kernel_read_file_id id);
> > > > > > > > extern void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > > > - struct dentry *dentry);
> > > > > > > > + const struct path *dir, struct dentry *dentry,
> > > > > > > > + umode_t mode, unsigned int dev);
> > > > > > > > extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > > > > > > > extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> > > > > > > > extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > > > > > > > @@ -114,7 +115,9 @@ static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > > > > > }
> > > > > > > >
> > > > > > > > static inline void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > > > - struct dentry *dentry)
> > > > > > > > + const struct path *dir,
> > > > > > > > + struct dentry *dentry,
> > > > > > > > + umode_t mode, unsigned int dev)
> > > > > > > > {
> > > > > > > > return;
> > > > > > > > }
> > > > > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > > > > > index 365db0e43d7c..76eba92d7f10 100644
> > > > > > > > --- a/security/integrity/ima/ima_main.c
> > > > > > > > +++ b/security/integrity/ima/ima_main.c
> > > > > > > > @@ -696,18 +696,26 @@ void ima_post_create_tmpfile(struct mnt_idmap *idmap,
> > > > > > > > /**
> > > > > > > > * ima_post_path_mknod - mark as a new inode
> > > > > > > > * @idmap: idmap of the mount the inode was found from
> > > > > > > > + * @dir: path structure of parent of the new file
> > > > > > > > * @dentry: newly created dentry
> > > > > > > > + * @mode: mode of the new file
> > > > > > > > + * @dev: undecoded device number
> > > > > > > > *
> > > > > > > > * Mark files created via the mknodat syscall as new, so that the
> > > > > > > > * file data can be written later.
> > > > > > > > */
> > > > > > > > void ima_post_path_mknod(struct mnt_idmap *idmap,
> > > > > > > > - struct dentry *dentry)
> > > > > > > > + const struct path *dir, struct dentry *dentry,
> > > > > > > > + umode_t mode, unsigned int dev)
> > > > > > > > {
> > > > > > > > struct integrity_iint_cache *iint;
> > > > > > > > struct inode *inode = dentry->d_inode;
> > > > > > > > int must_appraise;
> > > > > > > >
> > > > > > > > + /* See do_mknodat(), IMA is executed for case 0: and case S_IFREG: */
> > > > > > > > + if ((mode & S_IFMT) != 0 && (mode & S_IFMT) != S_IFREG)
> > > > > > > > + return;
> > > > > > > > +
> > > > > > >
> > > > > > > There's already a check below to make sure that this is a regular file.
> > > > > > > Are both needed?
> > > > > >
> > > > > > You are right, I can remove the first check.
> > > > >
> > > > > The question then becomes why modify hook the arguments?
> > > >
> > > > We need to make sure that ima_post_path_mknod() has the same parameters
> > > > as the LSM hook at the time we register it to the LSM infrastructure.
> > >
> > > I'm trying to understand why the pre hook parameters and the missing
> > > IMA parameter are used, as opposed to just defining the new
> > > post_path_mknod hook like IMA.
> >
> > As an empyrical rule, I pass the same parameters as the corresponding
> > pre hook (plus idmap, in this case). This is similar to the
> > inode_setxattr hook. But I can be wrong, if desired I can reduce.
>
> The inode_setxattr hook change example is legitimate, as EVM includes
> idmap, while IMA doesn't.
>
> Unless there is a good reason for the additional parameters, I'm not
> sure that adding them makes sense. Not modifying the parameter list
> will reduce the size of this patch set.
The hook is going to be used by any LSM. Without knowing all the
possible use cases, maybe it is better to include more information now,
than modifying the hook and respective implementations later.
(again, no problem to reduce)
Thanks
Roberto
> > > > > We need to make sure that ima_post_path_mknod() has the same parameters
> > > > > as the LSM hook at the time we register it to the LSM infrastructure.
> > > >
> > > > I'm trying to understand why the pre hook parameters and the missing
> > > > IMA parameter are used, as opposed to just defining the new
> > > > post_path_mknod hook like IMA.
> > >
> > > As an empyrical rule, I pass the same parameters as the corresponding
> > > pre hook (plus idmap, in this case). This is similar to the
> > > inode_setxattr hook. But I can be wrong, if desired I can reduce.
> >
> > The inode_setxattr hook change example is legitimate, as EVM includes
> > idmap, while IMA doesn't.
> >
> > Unless there is a good reason for the additional parameters, I'm not
> > sure that adding them makes sense. Not modifying the parameter list
> > will reduce the size of this patch set.
>
> The hook is going to be used by any LSM. Without knowing all the
> possible use cases, maybe it is better to include more information now,
> than modifying the hook and respective implementations later.
>
> (again, no problem to reduce)
Unless there is a known use case for a specific parameter, please
minimize them. Additional parameters can be added later as needed.
--
thanks,
Mimi
On Thu, 2023-10-12 at 13:10 -0400, Mimi Zohar wrote:
> > > > > > We need to make sure that ima_post_path_mknod() has the
> > > > > > same parameters
> > > > > > as the LSM hook at the time we register it to the LSM
> > > > > > infrastructure.
> > > > >
> > > > > I'm trying to understand why the pre hook parameters and the
> > > > > missing
> > > > > IMA parameter are used, as opposed to just defining the new
> > > > > post_path_mknod hook like IMA.
> > > >
> > > > As an empyrical rule, I pass the same parameters as the
> > > > corresponding
> > > > pre hook (plus idmap, in this case). This is similar to the
> > > > inode_setxattr hook. But I can be wrong, if desired I can
> > > > reduce.
> > >
> > > The inode_setxattr hook change example is legitimate, as EVM
> > > includes
> > > idmap, while IMA doesn't.
> > >
> > > Unless there is a good reason for the additional parameters, I'm
> > > not
> > > sure that adding them makes sense. Not modifying the parameter
> > > list
> > > will reduce the size of this patch set.
> >
> > The hook is going to be used by any LSM. Without knowing all the
> > possible use cases, maybe it is better to include more information
> > now,
> > than modifying the hook and respective implementations later.
> >
> > (again, no problem to reduce)
>
> Unless there is a known use case for a specific parameter, please
> minimize them. Additional parameters can be added later as needed.
Ok. I did the same for inode_post_create_tmpfile.
Thanks
Roberto
On Mon, 2023-09-04 at 15:34 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> the path_post_mknod hook.
>
> It is useful for IMA to let new empty files be subsequently opened for
> further modification.
(Please remove "It is useful for" here and in other patch
descriptions. Will not repeat this again.)
Possible wording:
IMA-appraisal requires all existing files in policy to have a file
hash/signature stored in security.ima. An exception is made for empty
files created by mknod, by tagging them as new files.
>
> LSMs can benefit from this hook to update the inode state after a file has
> been successfully created.
(Please remove "LSMs can benefit from" here and in other patch
descriptions.)
Please make sure that the patch description is in sync with the LSM
hook kernel doc.
> The new hook cannot return an error and cannot
> cause the operation to be canceled.
(Separate paragaraph)
>
> Signed-off-by: Roberto Sassu <[email protected]>
--
thanks,
Mimi
On Mon, 2023-09-04 at 15:33 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> IMA and EVM are not effectively LSMs, especially due the fact that in the
> past they could not provide a security blob while there is another LSM
> active.
>
> That changed in the recent years, the LSM stacking feature now makes it
> possible to stack together multiple LSMs, and allows them to provide a
> security blob for most kernel objects. While the LSM stacking feature has
> some limitations being worked out, it is already suitable to make IMA and
> EVM as LSMs.
>
> In short, while this patch set is big, it does not make any functional
> change to IMA and EVM. IMA and EVM functions are called by the LSM
> infrastructure in the same places as before (except ima_post_path_mknod()),
> rather being hardcoded calls, and the inode metadata pointer is directly
> stored in the inode security blob rather than in a separate rbtree.
>
> More specifically, patches 1-11 make IMA and EVM functions suitable to
> be registered to the LSM infrastructure, by aligning function parameters.
>
> Patches 12-20 add new LSM hooks in the same places where IMA and EVM
> functions are called, if there is no LSM hook already.
>
> Patches 21-24 do the bulk of the work, remove hardcoded calls to IMA, EVM
> and integrity functions, register those functions in the LSM
> infrastructure, and let the latter call them. In addition, they also
> reserve one slot for EVM to supply an xattr with the inode_init_security
> hook.
>
> Finally, patch 25 removes the rbtree used to bind metadata to the inodes,
> and instead reserves a space in the inode security blob to store the
> pointer to metadata. This also brings performance improvements due to
> retrieving metadata in constant time, as opposed to logarithmic.
>
> The patch set applies on top of lsm/next, commit 8e4672d6f902 ("lsm:
> constify the 'file' parameter in security_binder_transfer_file()")
Thanks, Roberto! There were just a few suggestions/changes, which
though minor, will result in some patch churn. Other than that, there
were some suggestions patch description suggestions.
--
thanks,
Mimi