[PATCH v4 0/5] evm: Prepare for moving to the LSM infrastructure

[PATCH v4 0/5] evm: Prepare for moving to the LSM infrastructure

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

One of the challenges that must be tackled to move IMA and EVM to the LSM
infrastructure is to ensure that EVM is capable to correctly handle
multiple stacked LSMs providing an xattr at file creation. At the moment,
there are few issues that would prevent a correct integration. This patch
set aims at solving them.

From the LSM infrastructure side, the LSM stacking feature added the
possibility of registering multiple implementations of the security hooks,
that are called sequentially whenever someone calls the corresponding
security hook. However, security_inode_init_security() is currently limited
to support one xattr provided by LSM and one by EVM.
security_old_inode_init_security() can only support one xattr due to its
API.

In addition, using the call_int_hook() macro causes some issues. According
to the documentation in include/linux/lsm_hooks.h, it is a legitimate case
that an LSM returns -EOPNOTSUPP when it does not want to provide an xattr.
However, the loop defined in the macro would stop calling subsequent LSMs
if that happens. In the case of security_old_inode_init_security(), using
the macro would also cause a memory leak due to replacing the *value
pointer, if multiple LSMs provide an xattr.

From EVM side, the first operation to be done is to change the definition
of evm_inode_init_security() to be compatible with the security hook
definition. Unfortunately, the current definition does not provide enough
information for EVM, as it must have visibility of all xattrs provided by
LSMs to correctly calculate the HMAC. This patch set changes the security
hook definition by replacing the name, value and len triple with the xattr
array allocated by security_inode_init_security().

Secondly, given that the place where EVM can fill an xattr is not provided
anymore with the changed definition, EVM must know how many elements are in
the xattr array. EVM can rely on the fact that the xattr array must be
terminated with an element with name field set to NULL. If EVM is moved to
the LSM infrastructure, the infrastructure will provide additional
information.

Casey suggested to use the reservation mechanism currently implemented for
other security blobs, for xattrs. In this way,
security_inode_init_security() can know after LSM initialization how many
slots for xattrs should be allocated, and LSMs know the offset in the
array from where they can start writing xattrs.

One of the problem was that LSMs can decide at run-time, although they
reserved a slot, to not use it (for example because they were not
initialized). Given that the initxattrs() method implemented by filesystems
expect that the array elements are contiguous, they would miss the slots
after the one not being initialized. security_check_compact_xattrs() has
been introduced to overcome this problem and also to check the correctness
of the xattrs provided by the LSMs.

This patch set has been tested by introducing several instances of a
TestLSM (some providing an xattr, some not, one with a wrong implementation
to see how the LSM infrastructure handles it, one providing multiple xattrs
and another providing an xattr but in a disabled state). The patch is not
included in this set but it is available here:

https://github.com/robertosassu/linux/commit/e0eed5b271e44ded36b23713f9a5998810954843

The test, added to ima-evm-utils, is available here:

https://github.com/robertosassu/ima-evm-utils/blob/evm-multiple-lsms-v4-devel-v10/tests/evm_multiple_lsms.test

The test takes a UML kernel built by Github Actions and launches it several
times, each time with a different combination of LSMs. After boot, it first
checks that there is an xattr for each LSM providing it, and then calculates
the HMAC in user space and compares it with the HMAC calculated by EVM in
kernel space.

A test report can be obtained here:

https://github.com/robertosassu/ima-evm-utils/actions/runs/3435348442/jobs/5727609718

The patch set has been tested with both the SElinux and Smack test suites.
Below, there is the summary of the test results:

SELinux Test Suite result (without patches):
Files=73, Tests=1346, 225 wallclock secs ( 0.43 usr  0.23 sys +  6.11 cusr 58.70 csys = 65.47 CPU)

SELinux Test Suite result (with patches):
Files=73, Tests=1346, 225 wallclock secs ( 0.44 usr  0.22 sys +  6.15 cusr 59.94 csys = 66.75 CPU)

Smack Test Suite result (without patches):
95 Passed, 0 Failed, 100% Success rate

Smack Test Suite result (with patches):
95 Passed, 0 Failed, 100% Success rate

The patch set has also been successfully tested with a WIP branch where
IMA/EVM have been moved to the LSM infrastructure. It is available here:

https://github.com/robertosassu/linux/commits/ima-evm-lsms-v1-devel-v8

This is the patch that moves EVM to the LSM infrastructure:

https://github.com/robertosassu/linux/commit/08ceb14a2ddfd334cb9d8703a4e1a86ee721b580

The only trivial changes, after this patch set, would be to allocate one
element less in the xattr array (because EVM will reserve its own xattr),
and to simply remove the call to evm_inode_init_security().

The test report when IMA and EVM are moved to the LSM infrastructure is
available here:

https://github.com/robertosassu/ima-evm-utils/actions/runs/3435500712/jobs/5727933607

Changelog

v3:
- Don't free the xattr name in reiserfs_security_free()
- Don't include fs_data parameter in inode_init_security hook
- Don't change evm_inode_init_security(), as it will be removed if EVM is
  stacked
- Fix inode_init_security hook documentation
- Drop lsm_find_xattr_slot(), use simple xattr reservation mechanism and
  introduce security_check_compact_xattrs() to compact the xattr array
- Don't allocate xattr array if LSMs didn't reserve any xattr
- Return zero if initxattrs() is not provided to
  security_inode_init_security(), -EOPNOTSUPP if value is not provided to
  security_old_inode_init_security()
- Request LSMs to fill xattrs if only value (not the triple) is provided to
  security_old_inode_init_security(), to avoid unnecessary memory
  allocation

v2:
- rewrite selinux_old_inode_init_security() to use
  security_inode_init_security()
- add lbs_xattr field to lsm_blob_sizes structure, to give the ability to
  LSMs to reserve slots in the xattr array (suggested by Casey)
- add new parameter base_slot to inode_init_security hook definition

v1:
- add calls to reiserfs_security_free() and initialize sec->value to NULL
  (suggested by Tetsuo and Mimi)
- change definition of inode_init_security hook, replace the name, value
  and len triple with the xattr array (suggested by Casey)
- introduce lsm_find_xattr_slot() helper for LSMs to find an unused slot in
  the passed xattr array

Roberto Sassu (5):
  reiserfs: Add missing calls to reiserfs_security_free()
  security: Rewrite security_old_inode_init_security()
  security: Allow all LSMs to provide xattrs for inode_init_security
    hook
  evm: Align evm_inode_init_security() definition with LSM
    infrastructure
  evm: Support multiple LSMs providing an xattr

 fs/reiserfs/namei.c                 |   4 +
 fs/reiserfs/xattr_security.c        |   2 +-
 include/linux/evm.h                 |  12 +--
 include/linux/lsm_hook_defs.h       |   3 +-
 include/linux/lsm_hooks.h           |  17 ++--
 security/integrity/evm/evm.h        |   2 +
 security/integrity/evm/evm_crypto.c |   9 +-
 security/integrity/evm/evm_main.c   |  28 ++++--
 security/security.c                 | 132 ++++++++++++++++++++++------
 security/selinux/hooks.c            |  19 ++--
 security/smack/smack_lsm.c          |  26 +++---
 11 files changed, 187 insertions(+), 67 deletions(-)

-- 
2.25.1

[PATCH v4 1/5] reiserfs: Add missing calls to reiserfs_security_free()

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Commit 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes
during inode creation") defined reiserfs_security_free() to free the name
and value of a security xattr allocated by the active LSM through
security_old_inode_init_security(). However, this function is not called
in the reiserfs code.

Thus, add a call to reiserfs_security_free() whenever
reiserfs_security_init() is called, and initialize value to NULL, to avoid
to call kfree() on an uninitialized pointer.

Finally, remove the kfree() for the xattr name, as it is not allocated
anymore.

Fixes: 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes during inode creation")
Cc: stable@vger.kernel.org
Cc: Jeff Mahoney <jeffm@suse.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: Mimi Zohar <zohar@linux.ibm.com>
Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/reiserfs/namei.c          | 4 ++++
 fs/reiserfs/xattr_security.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c
index 3d7a35d6a18b..b916859992ec 100644
--- a/fs/reiserfs/namei.c
+++ b/fs/reiserfs/namei.c
@@ -696,6 +696,7 @@ static int reiserfs_create(struct user_namespace *mnt_userns, struct inode *dir,
 
 out_failed:
 	reiserfs_write_unlock(dir->i_sb);
+	reiserfs_security_free(&security);
 	return retval;
 }
 
@@ -779,6 +780,7 @@ static int reiserfs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
 
 out_failed:
 	reiserfs_write_unlock(dir->i_sb);
+	reiserfs_security_free(&security);
 	return retval;
 }
 
@@ -878,6 +880,7 @@ static int reiserfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 	retval = journal_end(&th);
 out_failed:
 	reiserfs_write_unlock(dir->i_sb);
+	reiserfs_security_free(&security);
 	return retval;
 }
 
@@ -1194,6 +1197,7 @@ static int reiserfs_symlink(struct user_namespace *mnt_userns,
 	retval = journal_end(&th);
 out_failed:
 	reiserfs_write_unlock(parent_dir->i_sb);
+	reiserfs_security_free(&security);
 	return retval;
 }
 
diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c
index 8965c8e5e172..857a65b05726 100644
--- a/fs/reiserfs/xattr_security.c
+++ b/fs/reiserfs/xattr_security.c
@@ -50,6 +50,7 @@ int reiserfs_security_init(struct inode *dir, struct inode *inode,
 	int error;
 
 	sec->name = NULL;
+	sec->value = NULL;
 
 	/* Don't add selinux attributes on xattrs - they'll never get used */
 	if (IS_PRIVATE(dir))
@@ -95,7 +96,6 @@ int reiserfs_security_write(struct reiserfs_transaction_handle *th,
 
 void reiserfs_security_free(struct reiserfs_security_handle *sec)
 {
-	kfree(sec->name);
 	kfree(sec->value);
 	sec->name = NULL;
 	sec->value = NULL;
-- 
2.25.1

[PATCH v4 2/5] security: Rewrite security_old_inode_init_security()

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Rewrite security_old_inode_init_security() to call
security_inode_init_security() before making changes to support multiple
LSMs providing xattrs. Do it so that the required changes are done only in
one place.

Define the security_initxattrs() callback and pass it to
security_inode_init_security() as argument, to obtain the first xattr
provided by LSMs.

This behavior is a bit different from the current one. Before this patch
calling call_int_hook() could cause multiple LSMs to provide an xattr,
since call_int_hook() does not stop when an LSM returns zero. The caller of
security_old_inode_init_security() receives the last xattr set. The pointer
of the xattr value of previous LSMs is lost, causing memory leaks.

However, in practice, this scenario does not happen as the only in-tree
LSMs providing an xattr at inode creation time are SELinux and Smack, which
are mutually exclusive.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/security.c | 58 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/security/security.c b/security/security.c
index 79d82cb6e469..a0e9b4ce2341 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1089,20 +1089,34 @@ int security_dentry_create_files_as(struct dentry *dentry, int mode,
 }
 EXPORT_SYMBOL(security_dentry_create_files_as);
 
+static int security_initxattrs(struct inode *inode, const struct xattr *xattrs,
+			       void *fs_info)
+{
+	struct xattr *dest = (struct xattr *)fs_info;
+
+	dest->name = xattrs->name;
+	dest->value = xattrs->value;
+	dest->value_len = xattrs->value_len;
+	return 0;
+}
+
 int security_inode_init_security(struct inode *inode, struct inode *dir,
 				 const struct qstr *qstr,
 				 const initxattrs initxattrs, void *fs_data)
 {
 	struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
 	struct xattr *lsm_xattr, *evm_xattr, *xattr;
-	int ret;
+	int ret = -EOPNOTSUPP;
 
 	if (unlikely(IS_PRIVATE(inode)))
-		return 0;
+		goto out_exit;
 
-	if (!initxattrs)
-		return call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
-				     dir, qstr, NULL, NULL, NULL);
+	if (!initxattrs ||
+	    (initxattrs == &security_initxattrs && !fs_data)) {
+		ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
+				    dir, qstr, NULL, NULL, NULL);
+		goto out_exit;
+	}
 	memset(new_xattrs, 0, sizeof(new_xattrs));
 	lsm_xattr = new_xattrs;
 	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
@@ -1118,8 +1132,19 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
 		goto out;
 	ret = initxattrs(inode, new_xattrs, fs_data);
 out:
-	for (xattr = new_xattrs; xattr->value != NULL; xattr++)
+	for (xattr = new_xattrs; xattr->value != NULL; xattr++) {
+		/*
+		 * Xattr value freed by the caller of
+		 * security_old_inode_init_security().
+		 */
+		if (xattr == new_xattrs && initxattrs == &security_initxattrs &&
+		    !ret)
+			continue;
 		kfree(xattr->value);
+	}
+out_exit:
+	if (initxattrs == &security_initxattrs)
+		return ret;
 	return (ret == -EOPNOTSUPP) ? 0 : ret;
 }
 EXPORT_SYMBOL(security_inode_init_security);
@@ -1136,10 +1161,23 @@ int security_old_inode_init_security(struct inode *inode, struct inode *dir,
 				     const struct qstr *qstr, const char **name,
 				     void **value, size_t *len)
 {
-	if (unlikely(IS_PRIVATE(inode)))
-		return -EOPNOTSUPP;
-	return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
-			     qstr, name, value, len);
+	struct xattr xattr = {};
+	struct xattr *lsm_xattr = (value) ? &xattr : NULL;
+	int ret;
+
+	ret = security_inode_init_security(inode, dir, qstr,
+					   security_initxattrs, lsm_xattr);
+	if (ret)
+		return ret;
+
+	if (name)
+		*name = lsm_xattr->name;
+	if (value)
+		*value = lsm_xattr->value;
+	if (len)
+		*len = lsm_xattr->value_len;
+
+	return 0;
 }
 EXPORT_SYMBOL(security_old_inode_init_security);
 
-- 
2.25.1

[PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Currently, security_inode_init_security() supports only one LSM providing
an xattr and EVM calculating the HMAC on that xattr, plus other inode
metadata.

Allow all LSMs to provide one or multiple xattrs, by extending the security
blob reservation mechanism. Introduce the new lbs_xattr field of the
lsm_blob_sizes structure, so that each LSM can specify how many xattrs it
needs, and the LSM infrastructure knows how many xattr slots it should
allocate.

Dynamically allocate the xattrs array to be populated by LSMs with the
inode_init_security hook, and pass it to the latter instead of the
name/value/len triple.

Since the LSM infrastructure, at initialization time, updates the number of
the requested xattrs provided by each LSM with a corresponding offset in
the security blob (in this case the xattr array), it makes straightforward
for an LSM to access the right position in the xattr array.

There is still the issue that an LSM might not fill the xattr, even if it
requests it (legitimate case, for example it might have been loaded but not
initialized with a policy). Since users of the xattr array (e.g. the
initxattrs() callbacks) detect the end of the xattr array by checking if
the xattr name is NULL, not filling an xattr would cause those users to
stop scanning xattrs prematurely.

Solve that issue by introducing security_check_compact_xattrs(), which does
a basic check of the xattr array (if the xattr name is filled, the xattr
value should be too, and viceversa), and compacts the xattr array by
removing the holes.

An alternative solution would be to let users of the xattr array know the
number of elements of the xattr array, so that they don't have to check the
termination. However, this seems more invasive, compared to a simple move
of few array elements.

Finally, adapt both SELinux and Smack to use the new definition of the
inode_init_security hook, and to correctly fill the designated slots in the
xattr array.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/lsm_hook_defs.h |  3 +-
 include/linux/lsm_hooks.h     | 17 +++++---
 security/security.c           | 77 +++++++++++++++++++++++++++--------
 security/selinux/hooks.c      | 19 +++++----
 security/smack/smack_lsm.c    | 26 +++++++-----
 5 files changed, 100 insertions(+), 42 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ec119da1d89b..be344d0211f8 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -112,8 +112,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
 LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
 LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
 LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
-	 struct inode *dir, const struct qstr *qstr, const char **name,
-	 void **value, size_t *len)
+	 struct inode *dir, const struct qstr *qstr, struct xattr *xattrs)
 LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
 	 const struct qstr *name, const struct inode *context_inode)
 LSM_HOOK(int, 0, inode_create, struct inode *dir, struct dentry *dentry,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 4ec80b96c22e..ba1655370643 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -229,18 +229,22 @@
  *	This hook is called by the fs code as part of the inode creation
  *	transaction and provides for atomic labeling of the inode, unlike
  *	the post_create/mkdir/... hooks called by the VFS.  The hook function
- *	is expected to allocate the name and value via kmalloc, with the caller
- *	being responsible for calling kfree after using them.
+ *	is expected to allocate the value via kmalloc, with the caller
+ *	being responsible for calling kfree after using it.
  *	If the security module does not use security attributes or does
  *	not wish to put a security attribute on this particular inode,
  *	then it should return -EOPNOTSUPP to skip this processing.
  *	@inode contains the inode structure of the newly created inode.
  *	@dir contains the inode structure of the parent directory.
  *	@qstr contains the last path component of the new object
- *	@name will be set to the allocated name suffix (e.g. selinux).
- *	@value will be set to the allocated attribute value.
- *	@len will be set to the length of the value.
- *	Returns 0 if @name and @value have been successfully set,
+ *	@xattrs contains the full array of xattrs provided by LSMs where
+ *	->name will be set to the name suffix (e.g. selinux).
+ *	->value will be set to the allocated attribute value.
+ *	->value_len will be set to the length of the value.
+ *	Slots in @xattrs need to be reserved by LSMs by providing the number of
+ *	the desired xattrs in the lbs_xattr field of the lsm_blob_sizes
+ *	structure.
+ *	Returns 0 if the requested slots in @xattrs have been successfully set,
  *	-EOPNOTSUPP if no security attribute is needed, or
  *	-ENOMEM on memory allocation failure.
  * @inode_init_security_anon:
@@ -1624,6 +1628,7 @@ struct lsm_blob_sizes {
 	int	lbs_ipc;
 	int	lbs_msg_msg;
 	int	lbs_task;
+	int	lbs_xattr;
 };
 
 /*
diff --git a/security/security.c b/security/security.c
index a0e9b4ce2341..b62f192de6da 100644
--- a/security/security.c
+++ b/security/security.c
@@ -30,8 +30,6 @@
 #include <linux/msg.h>
 #include <net/flow.h>
 
-#define MAX_LSM_EVM_XATTR	2
-
 /* How many LSMs were built into the kernel? */
 #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
 
@@ -210,6 +208,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
 	lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
 	lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
 	lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
+	lsm_set_blob_size(&needed->lbs_xattr, &blob_sizes.lbs_xattr);
 }
 
 /* Prepare LSM for initialization. */
@@ -346,6 +345,7 @@ static void __init ordered_lsm_init(void)
 	init_debug("msg_msg blob size    = %d\n", blob_sizes.lbs_msg_msg);
 	init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
 	init_debug("task blob size       = %d\n", blob_sizes.lbs_task);
+	init_debug("xattr slots          = %d\n", blob_sizes.lbs_xattr);
 
 	/*
 	 * Create any kmem_caches needed for blobs
@@ -1100,34 +1100,78 @@ static int security_initxattrs(struct inode *inode, const struct xattr *xattrs,
 	return 0;
 }
 
+static int security_check_compact_xattrs(struct xattr *xattrs,
+					 int num_xattrs, int *checked_xattrs)
+{
+	int i;
+
+	for (i = *checked_xattrs; i < num_xattrs; i++) {
+		if ((!xattrs[i].name && xattrs[i].value) ||
+		    (xattrs[i].name && !xattrs[i].value))
+			return -EINVAL;
+
+		if (!xattrs[i].name)
+			continue;
+
+		if (i == *checked_xattrs) {
+			(*checked_xattrs)++;
+			continue;
+		}
+
+		memcpy(xattrs + (*checked_xattrs)++, xattrs + i,
+		       sizeof(*xattrs));
+		memset(xattrs + i, 0, sizeof(*xattrs));
+	}
+
+	return 0;
+}
+
 int security_inode_init_security(struct inode *inode, struct inode *dir,
 				 const struct qstr *qstr,
 				 const initxattrs initxattrs, void *fs_data)
 {
-	struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
-	struct xattr *lsm_xattr, *evm_xattr, *xattr;
-	int ret = -EOPNOTSUPP;
+	struct security_hook_list *P;
+	struct xattr *new_xattrs;
+	struct xattr *xattr;
+	int ret = -EOPNOTSUPP, cur_xattrs = 0;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		goto out_exit;
 
+	if (!blob_sizes.lbs_xattr)
+		goto out_exit;
+
 	if (!initxattrs ||
 	    (initxattrs == &security_initxattrs && !fs_data)) {
 		ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
-				    dir, qstr, NULL, NULL, NULL);
+				    dir, qstr, NULL);
 		goto out_exit;
 	}
-	memset(new_xattrs, 0, sizeof(new_xattrs));
-	lsm_xattr = new_xattrs;
-	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
-						&lsm_xattr->name,
-						&lsm_xattr->value,
-						&lsm_xattr->value_len);
-	if (ret)
-		goto out;
+	/* Allocate +1 for EVM and +1 as terminator. */
+	new_xattrs = kcalloc(blob_sizes.lbs_xattr + 2, sizeof(*new_xattrs),
+			     GFP_NOFS);
+	if (!new_xattrs) {
+		ret = -ENOMEM;
+		goto out_exit;
+	}
+	hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
+			     list) {
+		ret = P->hook.inode_init_security(inode, dir, qstr, new_xattrs);
+		if (ret && ret != -EOPNOTSUPP)
+			goto out;
+		if (ret == -EOPNOTSUPP)
+			continue;
+		ret = security_check_compact_xattrs(new_xattrs,
+						    blob_sizes.lbs_xattr,
+						    &cur_xattrs);
+		if (ret < 0) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	}
 
-	evm_xattr = lsm_xattr + 1;
-	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
+	ret = evm_inode_init_security(inode, new_xattrs,
+				      new_xattrs + cur_xattrs);
 	if (ret)
 		goto out;
 	ret = initxattrs(inode, new_xattrs, fs_data);
@@ -1142,6 +1186,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
 			continue;
 		kfree(xattr->value);
 	}
+	kfree(new_xattrs);
 out_exit:
 	if (initxattrs == &security_initxattrs)
 		return ret;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f553c370397e..57e5bc7c9ed8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -104,6 +104,8 @@
 #include "audit.h"
 #include "avc_ss.h"
 
+#define SELINUX_INODE_INIT_XATTRS 1
+
 struct selinux_state selinux_state;
 
 /* SECMARK reference count */
@@ -2868,11 +2870,11 @@ static int selinux_dentry_create_files_as(struct dentry *dentry, int mode,
 
 static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 				       const struct qstr *qstr,
-				       const char **name,
-				       void **value, size_t *len)
+				       struct xattr *xattrs)
 {
 	const struct task_security_struct *tsec = selinux_cred(current_cred());
 	struct superblock_security_struct *sbsec;
+	struct xattr *xattr = NULL;
 	u32 newsid, clen;
 	int rc;
 	char *context;
@@ -2899,16 +2901,18 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 	    !(sbsec->flags & SBLABEL_MNT))
 		return -EOPNOTSUPP;
 
-	if (name)
-		*name = XATTR_SELINUX_SUFFIX;
+	if (xattrs)
+		xattr = xattrs + selinux_blob_sizes.lbs_xattr;
+
+	if (xattr) {
+		xattr->name = XATTR_SELINUX_SUFFIX;
 
-	if (value && len) {
 		rc = security_sid_to_context_force(&selinux_state, newsid,
 						   &context, &clen);
 		if (rc)
 			return rc;
-		*value = context;
-		*len = clen;
+		xattr->value = context;
+		xattr->value_len = clen;
 	}
 
 	return 0;
@@ -6900,6 +6904,7 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
 	.lbs_ipc = sizeof(struct ipc_security_struct),
 	.lbs_msg_msg = sizeof(struct msg_security_struct),
 	.lbs_superblock = sizeof(struct superblock_security_struct),
+	.lbs_xattr = SELINUX_INODE_INIT_XATTRS,
 };
 
 #ifdef CONFIG_PERF_EVENTS
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index b6306d71c908..e770cfbc82b5 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -52,6 +52,8 @@
 #define SMK_RECEIVING	1
 #define SMK_SENDING	2
 
+#define SMACK_INODE_INIT_XATTRS 1
+
 #ifdef SMACK_IPV6_PORT_LABELING
 static DEFINE_MUTEX(smack_ipv6_lock);
 static LIST_HEAD(smk_ipv6_port_list);
@@ -939,26 +941,27 @@ static int smack_inode_alloc_security(struct inode *inode)
  * @inode: the newly created inode
  * @dir: containing directory object
  * @qstr: unused
- * @name: where to put the attribute name
- * @value: where to put the attribute value
- * @len: where to put the length of the attribute
+ * @xattrs: where to put the attribute
  *
  * Returns 0 if it all works out, -ENOMEM if there's no memory
  */
 static int smack_inode_init_security(struct inode *inode, struct inode *dir,
-				     const struct qstr *qstr, const char **name,
-				     void **value, size_t *len)
+				     const struct qstr *qstr,
+				     struct xattr *xattrs)
 {
 	struct inode_smack *issp = smack_inode(inode);
 	struct smack_known *skp = smk_of_current();
 	struct smack_known *isp = smk_of_inode(inode);
 	struct smack_known *dsp = smk_of_inode(dir);
+	struct xattr *xattr = NULL;
 	int may;
 
-	if (name)
-		*name = XATTR_SMACK_SUFFIX;
+	if (xattrs)
+		xattr = xattrs + smack_blob_sizes.lbs_xattr;
+
+	if (xattr) {
+		xattr->name = XATTR_SMACK_SUFFIX;
 
-	if (value && len) {
 		rcu_read_lock();
 		may = smk_access_entry(skp->smk_known, dsp->smk_known,
 				       &skp->smk_rules);
@@ -976,11 +979,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 			issp->smk_flags |= SMK_INODE_CHANGED;
 		}
 
-		*value = kstrdup(isp->smk_known, GFP_NOFS);
-		if (*value == NULL)
+		xattr->value = kstrdup(isp->smk_known, GFP_NOFS);
+		if (xattr->value == NULL)
 			return -ENOMEM;
 
-		*len = strlen(isp->smk_known);
+		xattr->value_len = strlen(isp->smk_known);
 	}
 
 	return 0;
@@ -4785,6 +4788,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
 	.lbs_ipc = sizeof(struct smack_known *),
 	.lbs_msg_msg = sizeof(struct smack_known *),
 	.lbs_superblock = sizeof(struct superblock_smack),
+	.lbs_xattr = SMACK_INODE_INIT_XATTRS,
 };
 
 static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
-- 
2.25.1

[PATCH v4 4/5] evm: Align evm_inode_init_security() definition with LSM infrastructure

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Change the evm_inode_init_security() definition to align with the LSM
infrastructure, in preparation for moving IMA and EVM to that
infrastructure.

This requires passing only the xattr array allocated by
security_inode_init_security(), instead of the first LSM xattr and the
place where the EVM xattr should be filled.

It also requires positioning after the last filled xattr (by checking the
xattr name), since the beginning of the xattr array is given.

If EVM is moved to the LSM infrastructure, it will use the xattr
reservation mechanism too, i.e. it positions itself in the xattr array with
the offset given by the LSM infrastructure.

Finally, make evm_inode_init_security() return value compatible with the
inode_init_security hook conventions, i.e. return -EOPNOTSUPP if it is not
setting an xattr.

EVM is a bit tricky, because xattrs is both an input and an output. If it
was just output, EVM should have returned zero if xattrs is NULL. But,
since xattrs is also input, EVM is unable to do its calculations, so return
-EOPNOTSUPP and handle this error in security_inode_init_security().

Don't change the return value in the inline function
evm_inode_init_security() in include/linux/evm.h, as the function will be
removed if EVM is moved to the LSM infrastructure.

Last note, this patch does not fix a possible crash if the xattr array is
empty (due to calling evm_protected_xattr() with a NULL argument). It will
be fixed with 'evm: Support multiple LSMs providing an xattr', as it will
first ensure that the xattr name is not NULL before calling
evm_protected_xattr().

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/evm.h               | 12 ++++++------
 security/integrity/evm/evm_main.c | 20 +++++++++++++-------
 security/security.c               |  5 ++---
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/include/linux/evm.h b/include/linux/evm.h
index aa63e0b3c0a2..3bb2ae9fe098 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -35,9 +35,9 @@ extern int evm_inode_removexattr(struct user_namespace *mnt_userns,
 				 struct dentry *dentry, const char *xattr_name);
 extern void evm_inode_post_removexattr(struct dentry *dentry,
 				       const char *xattr_name);
-extern int evm_inode_init_security(struct inode *inode,
-				   const struct xattr *xattr_array,
-				   struct xattr *evm);
+extern int evm_inode_init_security(struct inode *inode, struct inode *dir,
+				   const struct qstr *qstr,
+				   struct xattr *xattrs);
 extern bool evm_revalidate_status(const char *xattr_name);
 extern int evm_protected_xattr_if_enabled(const char *req_xattr_name);
 extern int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
@@ -108,9 +108,9 @@ static inline void evm_inode_post_removexattr(struct dentry *dentry,
 	return;
 }
 
-static inline int evm_inode_init_security(struct inode *inode,
-					  const struct xattr *xattr_array,
-					  struct xattr *evm)
+static inline int evm_inode_init_security(struct inode *inode, struct inode *dir,
+					  const struct qstr *qstr,
+					  struct xattr *xattrs)
 {
 	return 0;
 }
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 23d484e05e6f..0a312cafb7de 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -845,23 +845,29 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
 /*
  * evm_inode_init_security - initializes security.evm HMAC value
  */
-int evm_inode_init_security(struct inode *inode,
-				 const struct xattr *lsm_xattr,
-				 struct xattr *evm_xattr)
+int evm_inode_init_security(struct inode *inode, struct inode *dir,
+			    const struct qstr *qstr,
+			    struct xattr *xattrs)
 {
 	struct evm_xattr *xattr_data;
+	struct xattr *xattr, *evm_xattr;
 	int rc;
 
-	if (!(evm_initialized & EVM_INIT_HMAC) ||
-	    !evm_protected_xattr(lsm_xattr->name))
-		return 0;
+	if (!(evm_initialized & EVM_INIT_HMAC) || !xattrs ||
+	    !evm_protected_xattr(xattrs->name))
+		return -EOPNOTSUPP;
+
+	for (xattr = xattrs; xattr->value != NULL; xattr++)
+		;
+
+	evm_xattr = xattr;
 
 	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
 	if (!xattr_data)
 		return -ENOMEM;
 
 	xattr_data->data.type = EVM_XATTR_HMAC;
-	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
+	rc = evm_init_hmac(inode, xattrs, xattr_data->digest);
 	if (rc < 0)
 		goto out;
 
diff --git a/security/security.c b/security/security.c
index b62f192de6da..999102101a75 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1170,9 +1170,8 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
 		}
 	}
 
-	ret = evm_inode_init_security(inode, new_xattrs,
-				      new_xattrs + cur_xattrs);
-	if (ret)
+	ret = evm_inode_init_security(inode, dir, qstr, new_xattrs);
+	if (ret && ret != -EOPNOTSUPP)
 		goto out;
 	ret = initxattrs(inode, new_xattrs, fs_data);
 out:
-- 
2.25.1

[PATCH v4 5/5] evm: Support multiple LSMs providing an xattr

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Currently, evm_inode_init_security() processes a single LSM xattr from
the array passed by security_inode_init_security(), and calculates the
HMAC on it and other inode metadata.

Given that initxattrs() callbacks, called by
security_inode_init_security(), expect that this array is terminated when
the xattr name is set to NULL, reuse the same assumption to scan all xattrs
and to calculate the HMAC on all of them.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm.h        |  2 ++
 security/integrity/evm/evm_crypto.c |  9 ++++++++-
 security/integrity/evm/evm_main.c   | 16 +++++++++++-----
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index f8b8c5004fc7..f799d72a59fa 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -46,6 +46,8 @@ struct evm_digest {
 	char digest[IMA_MAX_DIGEST_SIZE];
 } __packed;
 
+int evm_protected_xattr(const char *req_xattr_name);
+
 int evm_init_key(void);
 int evm_update_evmxattr(struct dentry *dentry,
 			const char *req_xattr_name,
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 708de9656bbd..68f99faac316 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -389,6 +389,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
 		  char *hmac_val)
 {
 	struct shash_desc *desc;
+	const struct xattr *xattr;
 
 	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
 	if (IS_ERR(desc)) {
@@ -396,7 +397,13 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
 		return PTR_ERR(desc);
 	}
 
-	crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len);
+	for (xattr = lsm_xattr; xattr->name != NULL; xattr++) {
+		if (!evm_protected_xattr(xattr->name))
+			continue;
+
+		crypto_shash_update(desc, xattr->value, xattr->value_len);
+	}
+
 	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
 	kfree(desc);
 	return 0;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 0a312cafb7de..1cf6871a0019 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -305,7 +305,7 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
 	return found;
 }
 
-static int evm_protected_xattr(const char *req_xattr_name)
+int evm_protected_xattr(const char *req_xattr_name)
 {
 	return evm_protected_xattr_common(req_xattr_name, false);
 }
@@ -851,14 +851,20 @@ int evm_inode_init_security(struct inode *inode, struct inode *dir,
 {
 	struct evm_xattr *xattr_data;
 	struct xattr *xattr, *evm_xattr;
+	bool evm_protected_xattrs = false;
 	int rc;
 
-	if (!(evm_initialized & EVM_INIT_HMAC) || !xattrs ||
-	    !evm_protected_xattr(xattrs->name))
+	if (!(evm_initialized & EVM_INIT_HMAC) || !xattrs)
 		return -EOPNOTSUPP;
 
-	for (xattr = xattrs; xattr->value != NULL; xattr++)
-		;
+	for (xattr = xattrs; xattr->value != NULL; xattr++) {
+		if (evm_protected_xattr(xattr->name))
+			evm_protected_xattrs = true;
+	}
+
+	/* EVM xattr not needed. */
+	if (!evm_protected_xattrs)
+		return -EOPNOTSUPP;
 
 	evm_xattr = xattr;
 
-- 
2.25.1

Re: [PATCH v4 1/5] reiserfs: Add missing calls to reiserfs_security_free()

[Mimi Zohar]

On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Commit 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes
> during inode creation") defined reiserfs_security_free() to free the name
> and value of a security xattr allocated by the active LSM through
> security_old_inode_init_security(). However, this function is not called
> in the reiserfs code.
> 
> Thus, add a call to reiserfs_security_free() whenever
> reiserfs_security_init() is called, and initialize value to NULL, to avoid
> to call kfree() on an uninitialized pointer.
> 
> Finally, remove the kfree() for the xattr name, as it is not allocated
> anymore.
> 
> Fixes: 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes during inode creation")
> Cc: stable@vger.kernel.org
> Cc: Jeff Mahoney <jeffm@suse.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: Mimi Zohar <zohar@linux.ibm.com>
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

Re: [PATCH v4 2/5] security: Rewrite security_old_inode_init_security()

[Mimi Zohar]

Hi Roberto,

On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Rewrite security_old_inode_init_security() to call
> security_inode_init_security() before making changes to support multiple
> LSMs providing xattrs. Do it so that the required changes are done only in
> one place.

Only security_inode_init_security() has support for EVM.   Making
security_old_inode_init_security() a wrapper for
security_inode_init_security() could result in security.evm extended
attributes being created that previously weren't created.

In fact ocfs2 defines ocfs2_init_security_get() as a wrapper for both
the old and new inode_init_security calls based on the caller's
preference.   Only mknod and symlink seem to use the old function. 
Wondering why do they differentiate between callers?  (Cc'ing the ocfs2
mailing list as they're affected by this change.)

"[PATCH v4 1/5] reiserfs: Add missing calls to
reiserfs_security_free()"  fixed a memory leak.  I couldn't tell if
there was a similar memory leak in ocfs2, the only other user of
security_old_inode_init_security().

As ocfs2 already defines initxattrs, that leaves only reiserfs missing
initxattrs().  A better, cleaner solution would be to define one.

thanks,

Mimi

> 
> Define the security_initxattrs() callback and pass it to
> security_inode_init_security() as argument, to obtain the first xattr
> provided by LSMs.
> 
> This behavior is a bit different from the current one. Before this patch
> calling call_int_hook() could cause multiple LSMs to provide an xattr,
> since call_int_hook() does not stop when an LSM returns zero. The caller of
> security_old_inode_init_security() receives the last xattr set. The pointer
> of the xattr value of previous LSMs is lost, causing memory leaks.
> 
> However, in practice, this scenario does not happen as the only in-tree
> LSMs providing an xattr at inode creation time are SELinux and Smack, which
> are mutually exclusive.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>b

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Mimi Zohar]

hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Currently, security_inode_init_security() supports only one LSM providing
> an xattr and EVM calculating the HMAC on that xattr, plus other inode
> metadata.
> 
> Allow all LSMs to provide one or multiple xattrs, by extending the security
> blob reservation mechanism. Introduce the new lbs_xattr field of the
> lsm_blob_sizes structure, so that each LSM can specify how many xattrs it
> needs, and the LSM infrastructure knows how many xattr slots it should
> allocate.

Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
doesn't currently support it.  The LSM xattrs are hard coded in
evm_config_default_xattrnames[],  based on whether the LSM is
configured.  Additional security xattrs may be included in the
security.evm calculation, by extending the list via
security/integrity/evm/evm_xattrs.

> 
> Dynamically allocate the xattrs array to be populated by LSMs with the
> inode_init_security hook, and pass it to the latter instead of the
> name/value/len triple.
> 
> Since the LSM infrastructure, at initialization time, updates the number of
> the requested xattrs provided by each LSM with a corresponding offset in
> the security blob (in this case the xattr array), it makes straightforward
> for an LSM to access the right position in the xattr array.
> 
> There is still the issue that an LSM might not fill the xattr, even if it
> requests it (legitimate case, for example it might have been loaded but not
> initialized with a policy). Since users of the xattr array (e.g. the
> initxattrs() callbacks) detect the end of the xattr array by checking if
> the xattr name is NULL, not filling an xattr would cause those users to
> stop scanning xattrs prematurely.
> 
> Solve that issue by introducing security_check_compact_xattrs(), which does
> a basic check of the xattr array (if the xattr name is filled, the xattr
> value should be too, and viceversa), and compacts the xattr array by
> removing the holes.
> 
> An alternative solution would be to let users of the xattr array know the
> number of elements of the xattr array, so that they don't have to check the
> termination. However, this seems more invasive, compared to a simple move
> of few array elements.
> 
> Finally, adapt both SELinux and Smack to use the new definition of the
> inode_init_security hook, and to correctly fill the designated slots in the
> xattr array.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---

> diff --git a/security/security.c b/security/security.c
> index a0e9b4ce2341..b62f192de6da 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -30,8 +30,6 @@
>  #include <linux/msg.h>
>  #include <net/flow.h>
>  
> -#define MAX_LSM_EVM_XATTR	2
> -
>  /* How many LSMs were built into the kernel? */
>  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>  
> @@ -210,6 +208,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
>  	lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
>  	lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
>  	lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
> +	lsm_set_blob_size(&needed->lbs_xattr, &blob_sizes.lbs_xattr);
>  }
>  
>  /* Prepare LSM for initialization. */
> @@ -346,6 +345,7 @@ static void __init ordered_lsm_init(void)
>  	init_debug("msg_msg blob size    = %d\n", blob_sizes.lbs_msg_msg);
>  	init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
>  	init_debug("task blob size       = %d\n", blob_sizes.lbs_task);
> +	init_debug("xattr slots          = %d\n", blob_sizes.lbs_xattr);
>  
>  	/*
>  	 * Create any kmem_caches needed for blobs
> @@ -1100,34 +1100,78 @@ static int security_initxattrs(struct inode *inode, const struct xattr *xattrs,
>  	return 0;
>  }

> +static int security_check_compact_xattrs(struct xattr *xattrs,
> +					 int num_xattrs, int *checked_xattrs)

Perhaps the variable naming is off, making it difficult to read.   So
although this is a static function, which normally doesn't require a
comment, it's definitely needs one.

> +{
> +	int i;
> +
> +	for (i = *checked_xattrs; i < num_xattrs; i++) {

If the number of "checked" xattrs was kept up to date, removing the
empty xattr gaps wouldn't require a loop.  Is the purpose of this loop
to support multiple per LSM xattrs?

> +		if ((!xattrs[i].name && xattrs[i].value) ||
> +		    (xattrs[i].name && !xattrs[i].value))
> +			return -EINVAL;
> +
> +		if (!xattrs[i].name)
> +			continue;
> +
> +		if (i == *checked_xattrs) {
> +			(*checked_xattrs)++;
> +			continue;
> +		}
> +
> +		memcpy(xattrs + (*checked_xattrs)++, xattrs + i,
> +		       sizeof(*xattrs));
> +		memset(xattrs + i, 0, sizeof(*xattrs));
> +	}
> +
> +	return 0;
> +}
> +
>  int security_inode_init_security(struct inode *inode, struct inode *dir,
>  				 const struct qstr *qstr,
>  				 const initxattrs initxattrs, void *fs_data)
>  {
> -	struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
> -	struct xattr *lsm_xattr, *evm_xattr, *xattr;
> -	int ret = -EOPNOTSUPP;
> +	struct security_hook_list *P;
> +	struct xattr *new_xattrs;
> +	struct xattr *xattr;
> +	int ret = -EOPNOTSUPP, cur_xattrs = 0;
>  
>  	if (unlikely(IS_PRIVATE(inode)))
>  		goto out_exit;
>  
> +	if (!blob_sizes.lbs_xattr)
> +		goto out_exit;
> +
>  	if (!initxattrs ||
>  	    (initxattrs == &security_initxattrs && !fs_data)) {
>  		ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
> -				    dir, qstr, NULL, NULL, NULL);
> +				    dir, qstr, NULL);
>  		goto out_exit;
>  	}
> -	memset(new_xattrs, 0, sizeof(new_xattrs));
> -	lsm_xattr = new_xattrs;
> -	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> -						&lsm_xattr->name,
> -						&lsm_xattr->value,
> -						&lsm_xattr->value_len);
> -	if (ret)
> -		goto out;
> +	/* Allocate +1 for EVM and +1 as terminator. */
> +	new_xattrs = kcalloc(blob_sizes.lbs_xattr + 2, sizeof(*new_xattrs),
> +			     GFP_NOFS);
> +	if (!new_xattrs) {
> +		ret = -ENOMEM;
> +		goto out_exit;
> +	}
> +	hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
> +			     list) {
> +		ret = P->hook.inode_init_security(inode, dir, qstr, new_xattrs);
> +		if (ret && ret != -EOPNOTSUPP)
> +			goto out;
> +		if (ret == -EOPNOTSUPP)
> +			continue;
> +		ret = security_check_compact_xattrs(new_xattrs,
> +						    blob_sizes.lbs_xattr,
> +						    &cur_xattrs);

Defining a variable named "cur_xattrs" to indicate the number of xattrs
compressed is off.  Perhaps use cur_num_xattrs?   Similarly,
"checked_xattrs" should be num_checked_xattrs.  Or change the existing
num_xattrs to max_num_xattrs and rename checked_xattrs to num_xattrs.

thanks,

Mimi

> +		if (ret < 0) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +	}
>  
> -	evm_xattr = lsm_xattr + 1;
> -	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
> +	ret = evm_inode_init_security(inode, new_xattrs,
> +				      new_xattrs + cur_xattrs);
>  	if (ret)
>  		goto out;
>  	ret = initxattrs(inode, new_xattrs, fs_data);
> @@ -1142,6 +1186,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  			continue;
>  		kfree(xattr->value);
>  	}
> +	kfree(new_xattrs);
>  out_exit:
>  	if (initxattrs == &security_initxattrs)
>  		return ret;

Re: [PATCH v4 4/5] evm: Align evm_inode_init_security() definition with LSM infrastructure

[Mimi Zohar]

On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Change the evm_inode_init_security() definition to align with the LSM
> infrastructure, in preparation for moving IMA and EVM to that
> infrastructure.
> 
> This requires passing only the xattr array allocated by
> security_inode_init_security(), instead of the first LSM xattr and the
> place where the EVM xattr should be filled.
> 
> It also requires positioning after the last filled xattr (by checking the
> xattr name), since the beginning of the xattr array is given.

Perhaps combine this sentence to the previous paragraph and start the
sentence with
"In lieu of passing the EVM xattr, ..." 

> If EVM is moved to the LSM infrastructure, it will use the xattr
> reservation mechanism too, i.e. it positions itself in the xattr array with
> the offset given by the LSM infrastructure.

The LSM infrastructure will need to support EVM as the last LSM.  Is
there a reason for including this comment in this patch description.

> Finally, make evm_inode_init_security() return value compatible with the
> inode_init_security hook conventions, i.e. return -EOPNOTSUPP if it is not
> setting an xattr.

> EVM is a bit tricky, because xattrs is both an input and an output. If it
> was just output, EVM should have returned zero if xattrs is NULL. But,
> since xattrs is also input, EVM is unable to do its calculations, so return
> -EOPNOTSUPP and handle this error in security_inode_init_security().
> 
> Don't change the return value in the inline function
> evm_inode_init_security() in include/linux/evm.h, as the function will be
> removed if EVM is moved to the LSM infrastructure.
> 
> Last note, this patch does not fix a possible crash if the xattr array is
> empty (due to calling evm_protected_xattr() with a NULL argument). It will
> be fixed with 'evm: Support multiple LSMs providing an xattr', as it will
> first ensure that the xattr name is not NULL before calling
> evm_protected_xattr().

From my reading of the code, although there might be multiple LSM
xattrs, this patch only includes the first LSM xattr in the security
EVM calculation.  So it only checks the first xattr's name.  Support
for including multiple LSM xattrs in the EVM hmac calculation is added
in the subsequent patch.

thanks,

Mimi

> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Re: [PATCH v4 5/5] evm: Support multiple LSMs providing an xattr

[Mimi Zohar]

On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Currently, evm_inode_init_security() processes a single LSM xattr from
> the array passed by security_inode_init_security(), and calculates the
> HMAC on it and other inode metadata.
> 
> Given that initxattrs() callbacks, called by
> security_inode_init_security(), expect that this array is terminated when
> the xattr name is set to NULL, reuse the same assumption to scan all xattrs
> and to calculate the HMAC on all of them.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Thanks, Roberto.  This looks good.

Mimi

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Casey Schaufler]

On 11/17/2022 8:05 AM, Mimi Zohar wrote:
> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>
>> Currently, security_inode_init_security() supports only one LSM providing
>> an xattr and EVM calculating the HMAC on that xattr, plus other inode
>> metadata.
>>
>> Allow all LSMs to provide one or multiple xattrs, by extending the security
>> blob reservation mechanism. Introduce the new lbs_xattr field of the
>> lsm_blob_sizes structure, so that each LSM can specify how many xattrs it
>> needs, and the LSM infrastructure knows how many xattr slots it should
>> allocate.
> Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
> doesn't currently support it.  The LSM xattrs are hard coded in
> evm_config_default_xattrnames[],  based on whether the LSM is
> configured.  Additional security xattrs may be included in the
> security.evm calculation, by extending the list via
> security/integrity/evm/evm_xattrs.

Smack uses multiple xattrs. All file system objects have a SMACK64
attribute, which is used for access control. A program file may have
a SMACK64EXEC attribute, which is the label the program will run with.
A library may have a SMACK64MMAP attribute to restrict loading. A
directory may have a SMACK64TRANSMUTE attribute, which modifies the
new object creation behavior.

The point being that it may be more than a "nice idea" to support
multiple xattrs. It's not a hypothetical situation.

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Mimi Zohar]

On Thu, 2022-11-17 at 09:18 -0800, Casey Schaufler wrote:
> On 11/17/2022 8:05 AM, Mimi Zohar wrote:
> > hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> >> From: Roberto Sassu <roberto.sassu@huawei.com>
> >>
> >> Currently, security_inode_init_security() supports only one LSM providing
> >> an xattr and EVM calculating the HMAC on that xattr, plus other inode
> >> metadata.
> >>
> >> Allow all LSMs to provide one or multiple xattrs, by extending the security
> >> blob reservation mechanism. Introduce the new lbs_xattr field of the
> >> lsm_blob_sizes structure, so that each LSM can specify how many xattrs it
> >> needs, and the LSM infrastructure knows how many xattr slots it should
> >> allocate.
> > Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
> > doesn't currently support it.  The LSM xattrs are hard coded in
> > evm_config_default_xattrnames[],  based on whether the LSM is
> > configured.  Additional security xattrs may be included in the
> > security.evm calculation, by extending the list via
> > security/integrity/evm/evm_xattrs.
> 
> Smack uses multiple xattrs. All file system objects have a SMACK64
> attribute, which is used for access control. A program file may have
> a SMACK64EXEC attribute, which is the label the program will run with.
> A library may have a SMACK64MMAP attribute to restrict loading. A
> directory may have a SMACK64TRANSMUTE attribute, which modifies the
> new object creation behavior.
> 
> The point being that it may be more than a "nice idea" to support
> multiple xattrs. It's not a hypothetical situation.

And each of these addiitonal Smack xattrs are already defined in 
evm_config_default_xattrnames[].

Mimi

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Casey Schaufler]

On 11/17/2022 9:24 AM, Mimi Zohar wrote:
> On Thu, 2022-11-17 at 09:18 -0800, Casey Schaufler wrote:
>> On 11/17/2022 8:05 AM, Mimi Zohar wrote:
>>> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>>>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>>>
>>>> Currently, security_inode_init_security() supports only one LSM providing
>>>> an xattr and EVM calculating the HMAC on that xattr, plus other inode
>>>> metadata.
>>>>
>>>> Allow all LSMs to provide one or multiple xattrs, by extending the security
>>>> blob reservation mechanism. Introduce the new lbs_xattr field of the
>>>> lsm_blob_sizes structure, so that each LSM can specify how many xattrs it
>>>> needs, and the LSM infrastructure knows how many xattr slots it should
>>>> allocate.
>>> Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
>>> doesn't currently support it.  The LSM xattrs are hard coded in
>>> evm_config_default_xattrnames[],  based on whether the LSM is
>>> configured.  Additional security xattrs may be included in the
>>> security.evm calculation, by extending the list via
>>> security/integrity/evm/evm_xattrs.
>> Smack uses multiple xattrs. All file system objects have a SMACK64
>> attribute, which is used for access control. A program file may have
>> a SMACK64EXEC attribute, which is the label the program will run with.
>> A library may have a SMACK64MMAP attribute to restrict loading. A
>> directory may have a SMACK64TRANSMUTE attribute, which modifies the
>> new object creation behavior.
>>
>> The point being that it may be more than a "nice idea" to support
>> multiple xattrs. It's not a hypothetical situation.
> And each of these addiitonal Smack xattrs are already defined in 
> evm_config_default_xattrnames[].

Then I'm confused by the statement that "EVM doesn't currently support it".

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Mimi Zohar]

On Thu, 2022-11-17 at 09:40 -0800, Casey Schaufler wrote:
> On 11/17/2022 9:24 AM, Mimi Zohar wrote:
> > On Thu, 2022-11-17 at 09:18 -0800, Casey Schaufler wrote:
> >> On 11/17/2022 8:05 AM, Mimi Zohar wrote:
> >>> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> >>>> From: Roberto Sassu <roberto.sassu@huawei.com>
> >>>>
> >>>> Currently, security_inode_init_security() supports only one LSM providing
> >>>> an xattr and EVM calculating the HMAC on that xattr, plus other inode
> >>>> metadata.
> >>>>
> >>>> Allow all LSMs to provide one or multiple xattrs, by extending the security
> >>>> blob reservation mechanism. Introduce the new lbs_xattr field of the
> >>>> lsm_blob_sizes structure, so that each LSM can specify how many xattrs it
> >>>> needs, and the LSM infrastructure knows how many xattr slots it should
> >>>> allocate.
> >>> Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
> >>> doesn't currently support it.  The LSM xattrs are hard coded in
> >>> evm_config_default_xattrnames[],  based on whether the LSM is
> >>> configured.  Additional security xattrs may be included in the
> >>> security.evm calculation, by extending the list via
> >>> security/integrity/evm/evm_xattrs.
> >> Smack uses multiple xattrs. All file system objects have a SMACK64
> >> attribute, which is used for access control. A program file may have
> >> a SMACK64EXEC attribute, which is the label the program will run with.
> >> A library may have a SMACK64MMAP attribute to restrict loading. A
> >> directory may have a SMACK64TRANSMUTE attribute, which modifies the
> >> new object creation behavior.
> >>
> >> The point being that it may be more than a "nice idea" to support
> >> multiple xattrs. It's not a hypothetical situation.
> > And each of these addiitonal Smack xattrs are already defined in 
> > evm_config_default_xattrnames[].
> 
> Then I'm confused by the statement that "EVM doesn't currently support it".

My mistake.  As you pointed out, Smack is defining multiple security
xattrs.

Mimi

Re: [PATCH v4 2/5] security: Rewrite security_old_inode_init_security()

[Roberto Sassu]

On 11/17/2022 2:03 PM, Mimi Zohar wrote:
> Hi Roberto,
> 
> On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>
>> Rewrite security_old_inode_init_security() to call
>> security_inode_init_security() before making changes to support multiple
>> LSMs providing xattrs. Do it so that the required changes are done only in
>> one place.
> 
> Only security_inode_init_security() has support for EVM.   Making
> security_old_inode_init_security() a wrapper for
> security_inode_init_security() could result in security.evm extended
> attributes being created that previously weren't created.

Hi Mimi

yes, I thought about this problem. In fact, it should not matter too 
much. Since security_old_inode_init_security() supports setting only one 
xattr: if there is an LSM xattr, that one will be set, and the EVM one 
will be discarded; if there is no LSM xattr, EVM would not add one.

> In fact ocfs2 defines ocfs2_init_security_get() as a wrapper for both
> the old and new inode_init_security calls based on the caller's
> preference.   Only mknod and symlink seem to use the old function.
> Wondering why do they differentiate between callers?  (Cc'ing the ocfs2
> mailing list as they're affected by this change.)
> 
> "[PATCH v4 1/5] reiserfs: Add missing calls to
> reiserfs_security_free()"  fixed a memory leak.  I couldn't tell if
> there was a similar memory leak in ocfs2, the only other user of
> security_old_inode_init_security().

Will look into it.

> As ocfs2 already defines initxattrs, that leaves only reiserfs missing
> initxattrs().  A better, cleaner solution would be to define one.

Yes, great idea!

Thanks

Roberto

> thanks,
> 
> Mimi
> 
>>
>> Define the security_initxattrs() callback and pass it to
>> security_inode_init_security() as argument, to obtain the first xattr
>> provided by LSMs.
>>
>> This behavior is a bit different from the current one. Before this patch
>> calling call_int_hook() could cause multiple LSMs to provide an xattr,
>> since call_int_hook() does not stop when an LSM returns zero. The caller of
>> security_old_inode_init_security() receives the last xattr set. The pointer
>> of the xattr value of previous LSMs is lost, causing memory leaks.
>>
>> However, in practice, this scenario does not happen as the only in-tree
>> LSMs providing an xattr at inode creation time are SELinux and Smack, which
>> are mutually exclusive.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>b

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Roberto Sassu]

On 11/17/2022 5:05 PM, Mimi Zohar wrote:
> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>
>> Currently, security_inode_init_security() supports only one LSM providing
>> an xattr and EVM calculating the HMAC on that xattr, plus other inode
>> metadata.
>>
>> Allow all LSMs to provide one or multiple xattrs, by extending the security
>> blob reservation mechanism. Introduce the new lbs_xattr field of the
>> lsm_blob_sizes structure, so that each LSM can specify how many xattrs it
>> needs, and the LSM infrastructure knows how many xattr slots it should
>> allocate.
> 
> Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
> doesn't currently support it.  The LSM xattrs are hard coded in
> evm_config_default_xattrnames[],  based on whether the LSM is
> configured.  Additional security xattrs may be included in the
> security.evm calculation, by extending the list via
> security/integrity/evm/evm_xattrs.

EVM wouldn't notice whether it is the same LSM that provide multiple 
xattrs or multiple LSMs provided one xattr. As long as the xattr array 
contains consecutive xattrs, that would be fine. In the IMA/EVM test I 
included a test case where an LSM provides two xattrs (seems to work fine).

>> Dynamically allocate the xattrs array to be populated by LSMs with the
>> inode_init_security hook, and pass it to the latter instead of the
>> name/value/len triple.
>>
>> Since the LSM infrastructure, at initialization time, updates the number of
>> the requested xattrs provided by each LSM with a corresponding offset in
>> the security blob (in this case the xattr array), it makes straightforward
>> for an LSM to access the right position in the xattr array.
>>
>> There is still the issue that an LSM might not fill the xattr, even if it
>> requests it (legitimate case, for example it might have been loaded but not
>> initialized with a policy). Since users of the xattr array (e.g. the
>> initxattrs() callbacks) detect the end of the xattr array by checking if
>> the xattr name is NULL, not filling an xattr would cause those users to
>> stop scanning xattrs prematurely.
>>
>> Solve that issue by introducing security_check_compact_xattrs(), which does
>> a basic check of the xattr array (if the xattr name is filled, the xattr
>> value should be too, and viceversa), and compacts the xattr array by
>> removing the holes.
>>
>> An alternative solution would be to let users of the xattr array know the
>> number of elements of the xattr array, so that they don't have to check the
>> termination. However, this seems more invasive, compared to a simple move
>> of few array elements.
>>
>> Finally, adapt both SELinux and Smack to use the new definition of the
>> inode_init_security hook, and to correctly fill the designated slots in the
>> xattr array.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
> 
>> diff --git a/security/security.c b/security/security.c
>> index a0e9b4ce2341..b62f192de6da 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -30,8 +30,6 @@
>>   #include <linux/msg.h>
>>   #include <net/flow.h>
>>   
>> -#define MAX_LSM_EVM_XATTR	2
>> -
>>   /* How many LSMs were built into the kernel? */
>>   #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>>   
>> @@ -210,6 +208,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
>>   	lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
>>   	lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
>>   	lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
>> +	lsm_set_blob_size(&needed->lbs_xattr, &blob_sizes.lbs_xattr);
>>   }
>>   
>>   /* Prepare LSM for initialization. */
>> @@ -346,6 +345,7 @@ static void __init ordered_lsm_init(void)
>>   	init_debug("msg_msg blob size    = %d\n", blob_sizes.lbs_msg_msg);
>>   	init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
>>   	init_debug("task blob size       = %d\n", blob_sizes.lbs_task);
>> +	init_debug("xattr slots          = %d\n", blob_sizes.lbs_xattr);
>>   
>>   	/*
>>   	 * Create any kmem_caches needed for blobs
>> @@ -1100,34 +1100,78 @@ static int security_initxattrs(struct inode *inode, const struct xattr *xattrs,
>>   	return 0;
>>   }
> 
>> +static int security_check_compact_xattrs(struct xattr *xattrs,
>> +					 int num_xattrs, int *checked_xattrs)
> 
> Perhaps the variable naming is off, making it difficult to read.   So
> although this is a static function, which normally doesn't require a
> comment, it's definitely needs one.

Ok, will improve it.

>> +{
>> +	int i;
>> +
>> +	for (i = *checked_xattrs; i < num_xattrs; i++) {
> 
> If the number of "checked" xattrs was kept up to date, removing the
> empty xattr gaps wouldn't require a loop.  Is the purpose of this loop
> to support multiple per LSM xattrs?

An LSM might reserve one or more xattrs, but not set it/them (for 
example because it is not initialized). In this case, removing the gaps 
is needed for all subsequent LSMs.

>> +		if ((!xattrs[i].name && xattrs[i].value) ||
>> +		    (xattrs[i].name && !xattrs[i].value))
>> +			return -EINVAL;
>> +
>> +		if (!xattrs[i].name)
>> +			continue;
>> +
>> +		if (i == *checked_xattrs) {
>> +			(*checked_xattrs)++;
>> +			continue;
>> +		}
>> +
>> +		memcpy(xattrs + (*checked_xattrs)++, xattrs + i,
>> +		       sizeof(*xattrs));
>> +		memset(xattrs + i, 0, sizeof(*xattrs));
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   int security_inode_init_security(struct inode *inode, struct inode *dir,
>>   				 const struct qstr *qstr,
>>   				 const initxattrs initxattrs, void *fs_data)
>>   {
>> -	struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
>> -	struct xattr *lsm_xattr, *evm_xattr, *xattr;
>> -	int ret = -EOPNOTSUPP;
>> +	struct security_hook_list *P;
>> +	struct xattr *new_xattrs;
>> +	struct xattr *xattr;
>> +	int ret = -EOPNOTSUPP, cur_xattrs = 0;
>>   
>>   	if (unlikely(IS_PRIVATE(inode)))
>>   		goto out_exit;
>>   
>> +	if (!blob_sizes.lbs_xattr)
>> +		goto out_exit;
>> +
>>   	if (!initxattrs ||
>>   	    (initxattrs == &security_initxattrs && !fs_data)) {
>>   		ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
>> -				    dir, qstr, NULL, NULL, NULL);
>> +				    dir, qstr, NULL);
>>   		goto out_exit;
>>   	}
>> -	memset(new_xattrs, 0, sizeof(new_xattrs));
>> -	lsm_xattr = new_xattrs;
>> -	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
>> -						&lsm_xattr->name,
>> -						&lsm_xattr->value,
>> -						&lsm_xattr->value_len);
>> -	if (ret)
>> -		goto out;
>> +	/* Allocate +1 for EVM and +1 as terminator. */
>> +	new_xattrs = kcalloc(blob_sizes.lbs_xattr + 2, sizeof(*new_xattrs),
>> +			     GFP_NOFS);
>> +	if (!new_xattrs) {
>> +		ret = -ENOMEM;
>> +		goto out_exit;
>> +	}
>> +	hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
>> +			     list) {
>> +		ret = P->hook.inode_init_security(inode, dir, qstr, new_xattrs);
>> +		if (ret && ret != -EOPNOTSUPP)
>> +			goto out;
>> +		if (ret == -EOPNOTSUPP)
>> +			continue;
>> +		ret = security_check_compact_xattrs(new_xattrs,
>> +						    blob_sizes.lbs_xattr,
>> +						    &cur_xattrs);
> 
> Defining a variable named "cur_xattrs" to indicate the number of xattrs
> compressed is off.  Perhaps use cur_num_xattrs?   Similarly,
> "checked_xattrs" should be num_checked_xattrs.  Or change the existing
> num_xattrs to max_num_xattrs and rename checked_xattrs to num_xattrs.

Ok.

Thanks

Roberto

> thanks,
> 
> Mimi
> 
>> +		if (ret < 0) {
>> +			ret = -ENOMEM;
>> +			goto out;
>> +		}
>> +	}
>>   
>> -	evm_xattr = lsm_xattr + 1;
>> -	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
>> +	ret = evm_inode_init_security(inode, new_xattrs,
>> +				      new_xattrs + cur_xattrs);
>>   	if (ret)
>>   		goto out;
>>   	ret = initxattrs(inode, new_xattrs, fs_data);
>> @@ -1142,6 +1186,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>>   			continue;
>>   		kfree(xattr->value);
>>   	}
>> +	kfree(new_xattrs);
>>   out_exit:
>>   	if (initxattrs == &security_initxattrs)
>>   		return ret;

Re: [PATCH v4 4/5] evm: Align evm_inode_init_security() definition with LSM infrastructure

[Roberto Sassu]

On 11/17/2022 6:07 PM, Mimi Zohar wrote:
> On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>
>> Change the evm_inode_init_security() definition to align with the LSM
>> infrastructure, in preparation for moving IMA and EVM to that
>> infrastructure.
>>
>> This requires passing only the xattr array allocated by
>> security_inode_init_security(), instead of the first LSM xattr and the
>> place where the EVM xattr should be filled.
>>
>> It also requires positioning after the last filled xattr (by checking the
>> xattr name), since the beginning of the xattr array is given.
> 
> Perhaps combine this sentence to the previous paragraph and start the
> sentence with
> "In lieu of passing the EVM xattr, ..."

Ok.

>> If EVM is moved to the LSM infrastructure, it will use the xattr
>> reservation mechanism too, i.e. it positions itself in the xattr array with
>> the offset given by the LSM infrastructure.
> 
> The LSM infrastructure will need to support EVM as the last LSM.  Is
> there a reason for including this comment in this patch description.

The idea is to first make EVM work like other LSMs, and then add 
limitations that are EVM-specific.

As a regular LSM, EVM could be placed anywhere in the list of LSMs. This 
would mean that whenever EVM is called, it will process xattrs that are 
set by previous LSMs, not the subsequent ones.

What we would need to do EVM-specific is that EVM is the last in the 
list of LSMs, to ensure that all xattrs are protected.

>> Finally, make evm_inode_init_security() return value compatible with the
>> inode_init_security hook conventions, i.e. return -EOPNOTSUPP if it is not
>> setting an xattr.
> 
>> EVM is a bit tricky, because xattrs is both an input and an output. If it
>> was just output, EVM should have returned zero if xattrs is NULL. But,
>> since xattrs is also input, EVM is unable to do its calculations, so return
>> -EOPNOTSUPP and handle this error in security_inode_init_security().
>>
>> Don't change the return value in the inline function
>> evm_inode_init_security() in include/linux/evm.h, as the function will be
>> removed if EVM is moved to the LSM infrastructure.
>>
>> Last note, this patch does not fix a possible crash if the xattr array is
>> empty (due to calling evm_protected_xattr() with a NULL argument). It will
>> be fixed with 'evm: Support multiple LSMs providing an xattr', as it will
>> first ensure that the xattr name is not NULL before calling
>> evm_protected_xattr().
> 
>  From my reading of the code, although there might be multiple LSM
> xattrs, this patch only includes the first LSM xattr in the security
> EVM calculation.  So it only checks the first xattr's name.  Support
> for including multiple LSM xattrs in the EVM hmac calculation is added
> in the subsequent patch.

I tried to include in this patch just the function definition change and 
keep the existing behavior.

The problem is trying to access xattr->name at the beginning of 
evm_inode_init_security().

That would disappear in patch 5, where there is a loop checking 
xattr->value first. Patch 3 disallows combination of NULL name - !NULL 
value and !NULL name - NULL value. Not sure if the latter is correct 
(empty xattr?). Will check what callers do.

Roberto

> thanks,
> 
> Mimi
> 
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Roberto Sassu]

On 11/17/2022 6:18 PM, Casey Schaufler wrote:
> On 11/17/2022 8:05 AM, Mimi Zohar wrote:
>> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>>
>>> Currently, security_inode_init_security() supports only one LSM providing
>>> an xattr and EVM calculating the HMAC on that xattr, plus other inode
>>> metadata.
>>>
>>> Allow all LSMs to provide one or multiple xattrs, by extending the security
>>> blob reservation mechanism. Introduce the new lbs_xattr field of the
>>> lsm_blob_sizes structure, so that each LSM can specify how many xattrs it
>>> needs, and the LSM infrastructure knows how many xattr slots it should
>>> allocate.
>> Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
>> doesn't currently support it.  The LSM xattrs are hard coded in
>> evm_config_default_xattrnames[],  based on whether the LSM is
>> configured.  Additional security xattrs may be included in the
>> security.evm calculation, by extending the list via
>> security/integrity/evm/evm_xattrs.
> 
> Smack uses multiple xattrs. All file system objects have a SMACK64
> attribute, which is used for access control. A program file may have
> a SMACK64EXEC attribute, which is the label the program will run with.
> A library may have a SMACK64MMAP attribute to restrict loading. A
> directory may have a SMACK64TRANSMUTE attribute, which modifies the
> new object creation behavior.
> 
> The point being that it may be more than a "nice idea" to support
> multiple xattrs. It's not a hypothetical situation.

Ok, that means that I have to change the number of xattrs reserved by 
Smack in patch 3.

Thanks

Roberto

Re: [PATCH v4 4/5] evm: Align evm_inode_init_security() definition with LSM infrastructure

[Mimi Zohar]

On Fri, 2022-11-18 at 10:30 +0100, Roberto Sassu wrote:
> On 11/17/2022 6:07 PM, Mimi Zohar wrote:
> > On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> >> From: Roberto Sassu <roberto.sassu@huawei.com>
> >>
> >> Change the evm_inode_init_security() definition to align with the LSM
> >> infrastructure, in preparation for moving IMA and EVM to that
> >> infrastructure.
> >>
> >> This requires passing only the xattr array allocated by
> >> security_inode_init_security(), instead of the first LSM xattr and the
> >> place where the EVM xattr should be filled.
> >>
> >> It also requires positioning after the last filled xattr (by checking the
> >> xattr name), since the beginning of the xattr array is given.
> > 
> > Perhaps combine this sentence to the previous paragraph and start the
> > sentence with
> > "In lieu of passing the EVM xattr, ..."
> 
> Ok.
> 
> >> If EVM is moved to the LSM infrastructure, it will use the xattr
> >> reservation mechanism too, i.e. it positions itself in the xattr array with
> >> the offset given by the LSM infrastructure.
> > 
> > The LSM infrastructure will need to support EVM as the last LSM.  Is
> > there a reason for including this comment in this patch description.
> 
> The idea is to first make EVM work like other LSMs, and then add 
> limitations that are EVM-specific.
> 
> As a regular LSM, EVM could be placed anywhere in the list of LSMs. This 
> would mean that whenever EVM is called, it will process xattrs that are 
> set by previous LSMs, not the subsequent ones.
> 
> What we would need to do EVM-specific is that EVM is the last in the 
> list of LSMs, to ensure that all xattrs are protected.
> 
> >> Finally, make evm_inode_init_security() return value compatible with the
> >> inode_init_security hook conventions, i.e. return -EOPNOTSUPP if it is not
> >> setting an xattr.
> > 
> >> EVM is a bit tricky, because xattrs is both an input and an output. If it
> >> was just output, EVM should have returned zero if xattrs is NULL. But,
> >> since xattrs is also input, EVM is unable to do its calculations, so return
> >> -EOPNOTSUPP and handle this error in security_inode_init_security().
> >>
> >> Don't change the return value in the inline function
> >> evm_inode_init_security() in include/linux/evm.h, as the function will be
> >> removed if EVM is moved to the LSM infrastructure.
> >>
> >> Last note, this patch does not fix a possible crash if the xattr array is
> >> empty (due to calling evm_protected_xattr() with a NULL argument). It will
> >> be fixed with 'evm: Support multiple LSMs providing an xattr', as it will
> >> first ensure that the xattr name is not NULL before calling
> >> evm_protected_xattr().
> > 
> >  From my reading of the code, although there might be multiple LSM
> > xattrs, this patch only includes the first LSM xattr in the security
> > EVM calculation.  So it only checks the first xattr's name.  Support
> > for including multiple LSM xattrs in the EVM hmac calculation is added
> > in the subsequent patch.
> 
> I tried to include in this patch just the function definition change and 
> keep the existing behavior.

That's fine.
> 
> The problem is trying to access xattr->name at the beginning of 
> evm_inode_init_security().
> 
> That would disappear in patch 5, where there is a loop checking 
> xattr->value first. Patch 3 disallows combination of NULL name - !NULL 
> value and !NULL name - NULL value. Not sure if the latter is correct 
> (empty xattr?). Will check what callers do.

My comments here and above were for improving the patch description:
- Just say what this patch is doing, not what subsequent changes will
do in the future.  We'll come to that when the time comes.

- Say something only the lines that this patch includes only one LSM
security xattr in the EVM calculation, like previously.

thanks,

Mimi

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Mimi Zohar]

On Fri, 2022-11-18 at 10:14 +0100, Roberto Sassu wrote:
> >> +static int security_check_compact_xattrs(struct xattr *xattrs,
> >> +                                     int num_xattrs, int *checked_xattrs)
> > 
> > Perhaps the variable naming is off, making it difficult to read.   So
> > although this is a static function, which normally doesn't require a
> > comment, it's definitely needs one.
> 
> Ok, will improve it.
> 
> >> +{
> >> +    int i;
> >> +
> >> +    for (i = *checked_xattrs; i < num_xattrs; i++) {
> > 
> > If the number of "checked" xattrs was kept up to date, removing the
> > empty xattr gaps wouldn't require a loop.  Is the purpose of this loop
> > to support multiple per LSM xattrs?
> 
> An LSM might reserve one or more xattrs, but not set it/them (for 
> example because it is not initialized). In this case, removing the gaps 
> is needed for all subsequent LSMs.

Including this sort of info in the function description or as a comment
in the code would definitely simplify review.

security_check_compact_xattrs() is called in the loop after getting
each LSM's xattr(s).  Only the current LSMs xattrs need to be
compressed, yet the loop goes to the maximum number of xattrs each
time. Just wondering if there is a way of improving it.

-- 
thanks,

Mimi

Re: [PATCH v4 4/5] evm: Align evm_inode_init_security() definition with LSM infrastructure

[Mimi Zohar]

On Fri, 2022-11-18 at 10:30 +0100, Roberto Sassu wrote:
> On 11/17/2022 6:07 PM, Mimi Zohar wrote:
> > On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> >> From: Roberto Sassu <roberto.sassu@huawei.com>
> >>
> >> Change the evm_inode_init_security() definition to align with the LSM
> >> infrastructure, in preparation for moving IMA and EVM to that
> >> infrastructure.
> >>
> >> This requires passing only the xattr array allocated by
> >> security_inode_init_security(), instead of the first LSM xattr and the
> >> place where the EVM xattr should be filled.
> >>
> >> It also requires positioning after the last filled xattr (by checking the
> >> xattr name), since the beginning of the xattr array is given.
> > 
> > Perhaps combine this sentence to the previous paragraph and start the
> > sentence with
> > "In lieu of passing the EVM xattr, ..."
> 
> Ok.
> 
> >> If EVM is moved to the LSM infrastructure, it will use the xattr
> >> reservation mechanism too, i.e. it positions itself in the xattr array with
> >> the offset given by the LSM infrastructure.
> > 
> > The LSM infrastructure will need to support EVM as the last LSM.  Is
> > there a reason for including this comment in this patch description.
> 
> The idea is to first make EVM work like other LSMs, and then add 
> limitations that are EVM-specific.
> 
> As a regular LSM, EVM could be placed anywhere in the list of LSMs. This 
> would mean that whenever EVM is called, it will process xattrs that are 
> set by previous LSMs, not the subsequent ones.
> 
> What we would need to do EVM-specific is that EVM is the last in the 
> list of LSMs, to ensure that all xattrs are protected.
> 
> >> Finally, make evm_inode_init_security() return value compatible with the
> >> inode_init_security hook conventions, i.e. return -EOPNOTSUPP if it is not
> >> setting an xattr.
> > 
> >> EVM is a bit tricky, because xattrs is both an input and an output. If it
> >> was just output, EVM should have returned zero if xattrs is NULL. But,
> >> since xattrs is also input, EVM is unable to do its calculations, so return
> >> -EOPNOTSUPP and handle this error in security_inode_init_security().
> >>
> >> Don't change the return value in the inline function
> >> evm_inode_init_security() in include/linux/evm.h, as the function will be
> >> removed if EVM is moved to the LSM infrastructure.
> >>
> >> Last note, this patch does not fix a possible crash if the xattr array is
> >> empty (due to calling evm_protected_xattr() with a NULL argument). It will
> >> be fixed with 'evm: Support multiple LSMs providing an xattr', as it will
> >> first ensure that the xattr name is not NULL before calling
> >> evm_protected_xattr().
> > 
> >  From my reading of the code, although there might be multiple LSM
> > xattrs, this patch only includes the first LSM xattr in the security
> > EVM calculation.  So it only checks the first xattr's name.  Support
> > for including multiple LSM xattrs in the EVM hmac calculation is added
> > in the subsequent patch.
> 
> I tried to include in this patch just the function definition change and 
> keep the existing behavior.

That's fine.
> 
> The problem is trying to access xattr->name at the beginning of 
> evm_inode_init_security().
> 
> That would disappear in patch 5, where there is a loop checking 
> xattr->value first. Patch 3 disallows combination of NULL name - !NULL 
> value and !NULL name - NULL value. Not sure if the latter is correct 
> (empty xattr?). Will check what callers do.

My comments here and above were for improving the patch description:
- Just say what this patch is doing, not what subsequent changes will
do in the future.  We'll come to that when the time comes.

- Say something only the lines that this patch includes only one LSM
security xattr in the EVM calculation, like previously.

thanks,

Mimi

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Mimi Zohar]

On Fri, 2022-11-18 at 10:32 +0100, Roberto Sassu wrote:
> > Smack uses multiple xattrs. All file system objects have a SMACK64
> > attribute, which is used for access control. A program file may have
> > a SMACK64EXEC attribute, which is the label the program will run with.
> > A library may have a SMACK64MMAP attribute to restrict loading. A
> > directory may have a SMACK64TRANSMUTE attribute, which modifies the
> > new object creation behavior.
> > 
> > The point being that it may be more than a "nice idea" to support
> > multiple xattrs. It's not a hypothetical situation.
> 
> Ok, that means that I have to change the number of xattrs reserved by 
> Smack in patch 3.

Based on evm_config_default_xattrnames[], there are 4.  There's the
original SMACK and these 3 additional ones.

Mimi

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Casey Schaufler]

On 11/18/2022 1:14 AM, Roberto Sassu wrote:
> On 11/17/2022 5:05 PM, Mimi Zohar wrote:
>> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>>
>>> Currently, security_inode_init_security() supports only one LSM
>>> providing
>>> an xattr and EVM calculating the HMAC on that xattr, plus other inode
>>> metadata.
>>>
>>> Allow all LSMs to provide one or multiple xattrs, by extending the
>>> security
>>> blob reservation mechanism. Introduce the new lbs_xattr field of the
>>> lsm_blob_sizes structure, so that each LSM can specify how many
>>> xattrs it
>>> needs, and the LSM infrastructure knows how many xattr slots it should
>>> allocate.
>>
>> Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
>> doesn't currently support it.  The LSM xattrs are hard coded in
>> evm_config_default_xattrnames[],  based on whether the LSM is
>> configured.  Additional security xattrs may be included in the
>> security.evm calculation, by extending the list via
>> security/integrity/evm/evm_xattrs.
>
> EVM wouldn't notice whether it is the same LSM that provide multiple
> xattrs or multiple LSMs provided one xattr. As long as the xattr array
> contains consecutive xattrs, that would be fine. In the IMA/EVM test I
> included a test case where an LSM provides two xattrs (seems to work
> fine).
>
>>> Dynamically allocate the xattrs array to be populated by LSMs with the
>>> inode_init_security hook, and pass it to the latter instead of the
>>> name/value/len triple.
>>>
>>> Since the LSM infrastructure, at initialization time, updates the
>>> number of
>>> the requested xattrs provided by each LSM with a corresponding
>>> offset in
>>> the security blob (in this case the xattr array), it makes
>>> straightforward
>>> for an LSM to access the right position in the xattr array.
>>>
>>> There is still the issue that an LSM might not fill the xattr, even
>>> if it
>>> requests it (legitimate case, for example it might have been loaded
>>> but not
>>> initialized with a policy). Since users of the xattr array (e.g. the
>>> initxattrs() callbacks) detect the end of the xattr array by
>>> checking if
>>> the xattr name is NULL, not filling an xattr would cause those users to
>>> stop scanning xattrs prematurely.
>>>
>>> Solve that issue by introducing security_check_compact_xattrs(),
>>> which does
>>> a basic check of the xattr array (if the xattr name is filled, the
>>> xattr
>>> value should be too, and viceversa), and compacts the xattr array by
>>> removing the holes.
>>>
>>> An alternative solution would be to let users of the xattr array
>>> know the
>>> number of elements of the xattr array, so that they don't have to
>>> check the
>>> termination. However, this seems more invasive, compared to a simple
>>> move
>>> of few array elements.
>>>
>>> Finally, adapt both SELinux and Smack to use the new definition of the
>>> inode_init_security hook, and to correctly fill the designated slots
>>> in the
>>> xattr array.
>>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>> ---
>>
>>> diff --git a/security/security.c b/security/security.c
>>> index a0e9b4ce2341..b62f192de6da 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -30,8 +30,6 @@
>>>   #include <linux/msg.h>
>>>   #include <net/flow.h>
>>>   -#define MAX_LSM_EVM_XATTR    2
>>> -
>>>   /* How many LSMs were built into the kernel? */
>>>   #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>>>   @@ -210,6 +208,7 @@ static void __init lsm_set_blob_sizes(struct
>>> lsm_blob_sizes *needed)
>>>       lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
>>>       lsm_set_blob_size(&needed->lbs_superblock,
>>> &blob_sizes.lbs_superblock);
>>>       lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
>>> +    lsm_set_blob_size(&needed->lbs_xattr, &blob_sizes.lbs_xattr);
>>>   }
>>>     /* Prepare LSM for initialization. */
>>> @@ -346,6 +345,7 @@ static void __init ordered_lsm_init(void)
>>>       init_debug("msg_msg blob size    = %d\n",
>>> blob_sizes.lbs_msg_msg);
>>>       init_debug("superblock blob size = %d\n",
>>> blob_sizes.lbs_superblock);
>>>       init_debug("task blob size       = %d\n", blob_sizes.lbs_task);
>>> +    init_debug("xattr slots          = %d\n", blob_sizes.lbs_xattr);
>>>         /*
>>>        * Create any kmem_caches needed for blobs
>>> @@ -1100,34 +1100,78 @@ static int security_initxattrs(struct inode
>>> *inode, const struct xattr *xattrs,
>>>       return 0;
>>>   }
>>
>>> +static int security_check_compact_xattrs(struct xattr *xattrs,
>>> +                     int num_xattrs, int *checked_xattrs)
>>
>> Perhaps the variable naming is off, making it difficult to read.   So
>> although this is a static function, which normally doesn't require a
>> comment, it's definitely needs one.
>
> Ok, will improve it.
>
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = *checked_xattrs; i < num_xattrs; i++) {
>>
>> If the number of "checked" xattrs was kept up to date, removing the
>> empty xattr gaps wouldn't require a loop.  Is the purpose of this loop
>> to support multiple per LSM xattrs?
>
> An LSM might reserve one or more xattrs, but not set it/them (for
> example because it is not initialized). In this case, removing the
> gaps is needed for all subsequent LSMs.

This is in fact the case with Smack. All file system objects have SMACK64 attributes,
some have SMACK64EXEC, others SMACK64MMAP and still others SMACK64TRANSMUTE.

>
>>> +        if ((!xattrs[i].name && xattrs[i].value) ||
>>> +            (xattrs[i].name && !xattrs[i].value))
>>> +            return -EINVAL;
>>> +
>>> +        if (!xattrs[i].name)
>>> +            continue;
>>> +
>>> +        if (i == *checked_xattrs) {
>>> +            (*checked_xattrs)++;
>>> +            continue;
>>> +        }
>>> +
>>> +        memcpy(xattrs + (*checked_xattrs)++, xattrs + i,
>>> +               sizeof(*xattrs));
>>> +        memset(xattrs + i, 0, sizeof(*xattrs));
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   int security_inode_init_security(struct inode *inode, struct inode
>>> *dir,
>>>                    const struct qstr *qstr,
>>>                    const initxattrs initxattrs, void *fs_data)
>>>   {
>>> -    struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
>>> -    struct xattr *lsm_xattr, *evm_xattr, *xattr;
>>> -    int ret = -EOPNOTSUPP;
>>> +    struct security_hook_list *P;
>>> +    struct xattr *new_xattrs;
>>> +    struct xattr *xattr;
>>> +    int ret = -EOPNOTSUPP, cur_xattrs = 0;
>>>         if (unlikely(IS_PRIVATE(inode)))
>>>           goto out_exit;
>>>   +    if (!blob_sizes.lbs_xattr)
>>> +        goto out_exit;
>>> +
>>>       if (!initxattrs ||
>>>           (initxattrs == &security_initxattrs && !fs_data)) {
>>>           ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
>>> -                    dir, qstr, NULL, NULL, NULL);
>>> +                    dir, qstr, NULL);
>>>           goto out_exit;
>>>       }
>>> -    memset(new_xattrs, 0, sizeof(new_xattrs));
>>> -    lsm_xattr = new_xattrs;
>>> -    ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
>>> dir, qstr,
>>> -                        &lsm_xattr->name,
>>> -                        &lsm_xattr->value,
>>> -                        &lsm_xattr->value_len);
>>> -    if (ret)
>>> -        goto out;
>>> +    /* Allocate +1 for EVM and +1 as terminator. */
>>> +    new_xattrs = kcalloc(blob_sizes.lbs_xattr + 2,
>>> sizeof(*new_xattrs),
>>> +                 GFP_NOFS);
>>> +    if (!new_xattrs) {
>>> +        ret = -ENOMEM;
>>> +        goto out_exit;
>>> +    }
>>> +    hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
>>> +                 list) {
>>> +        ret = P->hook.inode_init_security(inode, dir, qstr,
>>> new_xattrs);
>>> +        if (ret && ret != -EOPNOTSUPP)
>>> +            goto out;
>>> +        if (ret == -EOPNOTSUPP)
>>> +            continue;
>>> +        ret = security_check_compact_xattrs(new_xattrs,
>>> +                            blob_sizes.lbs_xattr,
>>> +                            &cur_xattrs);
>>
>> Defining a variable named "cur_xattrs" to indicate the number of xattrs
>> compressed is off.  Perhaps use cur_num_xattrs?   Similarly,
>> "checked_xattrs" should be num_checked_xattrs.  Or change the existing
>> num_xattrs to max_num_xattrs and rename checked_xattrs to num_xattrs.
>
> Ok.
>
> Thanks
>
> Roberto
>
>> thanks,
>>
>> Mimi
>>
>>> +        if (ret < 0) {
>>> +            ret = -ENOMEM;
>>> +            goto out;
>>> +        }
>>> +    }
>>>   -    evm_xattr = lsm_xattr + 1;
>>> -    ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
>>> +    ret = evm_inode_init_security(inode, new_xattrs,
>>> +                      new_xattrs + cur_xattrs);
>>>       if (ret)
>>>           goto out;
>>>       ret = initxattrs(inode, new_xattrs, fs_data);
>>> @@ -1142,6 +1186,7 @@ int security_inode_init_security(struct inode
>>> *inode, struct inode *dir,
>>>               continue;
>>>           kfree(xattr->value);
>>>       }
>>> +    kfree(new_xattrs);
>>>   out_exit:
>>>       if (initxattrs == &security_initxattrs)
>>>           return ret;
>

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Casey Schaufler]

On 11/18/2022 7:10 AM, Mimi Zohar wrote:
> On Fri, 2022-11-18 at 10:14 +0100, Roberto Sassu wrote:
>>>> +static int security_check_compact_xattrs(struct xattr *xattrs,
>>>> +                                     int num_xattrs, int *checked_xattrs)
>>> Perhaps the variable naming is off, making it difficult to read.   So
>>> although this is a static function, which normally doesn't require a
>>> comment, it's definitely needs one.
>> Ok, will improve it.
>>
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = *checked_xattrs; i < num_xattrs; i++) {
>>> If the number of "checked" xattrs was kept up to date, removing the
>>> empty xattr gaps wouldn't require a loop.  Is the purpose of this loop
>>> to support multiple per LSM xattrs?
>> An LSM might reserve one or more xattrs, but not set it/them (for 
>> example because it is not initialized). In this case, removing the gaps 
>> is needed for all subsequent LSMs.
> Including this sort of info in the function description or as a comment
> in the code would definitely simplify review.
>
> security_check_compact_xattrs() is called in the loop after getting
> each LSM's xattr(s).  Only the current LSMs xattrs need to be
> compressed, yet the loop goes to the maximum number of xattrs each
> time. Just wondering if there is a way of improving it.

At security module registration each module could identify how
many xattrs it uses. That number could be used to limit the range
of the loop. I have to do similar things for the forthcoming LSM
syscalls and module stacking beyond that.


>

Re: [PATCH v4 2/5] security: Rewrite security_old_inode_init_security()

[Roberto Sassu]

On Thu, 2022-11-17 at 08:03 -0500, Mimi Zohar wrote:
> Hi Roberto,
> 
> On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Rewrite security_old_inode_init_security() to call
> > security_inode_init_security() before making changes to support multiple
> > LSMs providing xattrs. Do it so that the required changes are done only in
> > one place.
> 
> Only security_inode_init_security() has support for EVM.   Making
> security_old_inode_init_security() a wrapper for
> security_inode_init_security() could result in security.evm extended
> attributes being created that previously weren't created.
> 
> In fact ocfs2 defines ocfs2_init_security_get() as a wrapper for both
> the old and new inode_init_security calls based on the caller's
> preference.   Only mknod and symlink seem to use the old function. 
> Wondering why do they differentiate between callers?  (Cc'ing the ocfs2
> mailing list as they're affected by this change.)
> 
> "[PATCH v4 1/5] reiserfs: Add missing calls to
> reiserfs_security_free()"  fixed a memory leak.  I couldn't tell if
> there was a similar memory leak in ocfs2, the only other user of
> security_old_inode_init_security().

From what I see, there is no memory leak there.

> As ocfs2 already defines initxattrs, that leaves only reiserfs missing
> initxattrs().  A better, cleaner solution would be to define one.

If I understood why security_old_inode_init_security() is called
instead of security_inode_init_security(), the reason seems that the
filesystem code uses the length of the obtained xattr to make some
calculations (e.g. reserve space). The xattr is written at a later
time.

Since for reiserfs there is a plan to deprecate it, it probably
wouldn't be worth to support the creation of multiple xattrs. I would
define a callback to take the first xattr and make a copy, so that
calling security_inode_init_security() + reiserfs_initxattrs() is
equivalent to calling security_old_inode_init_security().

But then, this is what anyway I was doing with the
security_initxattrs() callback, for all callers of security_old_inode_i
nit_security().

Also, security_old_inode_init_security() is exported to kernel modules.
Maybe, it is used somewhere. So, unless we plan to remove it
completely, it should be probably be fixed to avoid multiple LSMs
successfully setting an xattr, and losing the memory of all except the
last (which this patch fixes by calling security_inode_init_security())
.

If there is still the preference, I will implement the reiserfs
callback and make a fix for security_old_inode_init_security().

Thanks

Roberto

> thanks,
> 
> Mimi
> 
> > Define the security_initxattrs() callback and pass it to
> > security_inode_init_security() as argument, to obtain the first xattr
> > provided by LSMs.
> > 
> > This behavior is a bit different from the current one. Before this patch
> > calling call_int_hook() could cause multiple LSMs to provide an xattr,
> > since call_int_hook() does not stop when an LSM returns zero. The caller of
> > security_old_inode_init_security() receives the last xattr set. The pointer
> > of the xattr value of previous LSMs is lost, causing memory leaks.
> > 
> > However, in practice, this scenario does not happen as the only in-tree
> > LSMs providing an xattr at inode creation time are SELinux and Smack, which
> > are mutually exclusive.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>b

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Roberto Sassu]

On Fri, 2022-11-18 at 09:31 -0800, Casey Schaufler wrote:
> On 11/18/2022 7:10 AM, Mimi Zohar wrote:
> > On Fri, 2022-11-18 at 10:14 +0100, Roberto Sassu wrote:
> > > > > +static int security_check_compact_xattrs(struct xattr *xattrs,
> > > > > +                                     int num_xattrs, int *checked_xattrs)
> > > > Perhaps the variable naming is off, making it difficult to read.   So
> > > > although this is a static function, which normally doesn't require a
> > > > comment, it's definitely needs one.
> > > Ok, will improve it.
> > > 
> > > > > +{
> > > > > +    int i;
> > > > > +
> > > > > +    for (i = *checked_xattrs; i < num_xattrs; i++) {
> > > > If the number of "checked" xattrs was kept up to date, removing the
> > > > empty xattr gaps wouldn't require a loop.  Is the purpose of this loop
> > > > to support multiple per LSM xattrs?
> > > An LSM might reserve one or more xattrs, but not set it/them (for 
> > > example because it is not initialized). In this case, removing the gaps 
> > > is needed for all subsequent LSMs.
> > Including this sort of info in the function description or as a comment
> > in the code would definitely simplify review.
> > 
> > security_check_compact_xattrs() is called in the loop after getting
> > each LSM's xattr(s).  Only the current LSMs xattrs need to be
> > compressed, yet the loop goes to the maximum number of xattrs each
> > time. Just wondering if there is a way of improving it.
> 
> At security module registration each module could identify how
> many xattrs it uses. That number could be used to limit the range
> of the loop. I have to do similar things for the forthcoming LSM
> syscalls and module stacking beyond that.

Yes, blob_sizes.lbs_xattr contains the total number of xattrs requested
by LSMs. To stop the loop earlier, at the offset of the next LSM, we
would need to search the LSM's lsm_info, using the LSM name in
the security_hook_list structure. Although it is not optimal, not doing
it makes the code simpler. I could do that, if preferred.

Roberto

Re: [PATCH v4 2/5] security: Rewrite security_old_inode_init_security()

[Mimi Zohar]

On Mon, 2022-11-21 at 10:45 +0100, Roberto Sassu wrote:
> > As ocfs2 already defines initxattrs, that leaves only reiserfs missing
> > initxattrs().  A better, cleaner solution would be to define one.
> 
> If I understood why security_old_inode_init_security() is called
> instead of security_inode_init_security(), the reason seems that the
> filesystem code uses the length of the obtained xattr to make some
> calculations (e.g. reserve space). The xattr is written at a later
> time.
> 
> Since for reiserfs there is a plan to deprecate it, it probably
> wouldn't be worth to support the creation of multiple xattrs. I would
> define a callback to take the first xattr and make a copy, so that
> calling security_inode_init_security() + reiserfs_initxattrs() is
> equivalent to calling security_old_inode_init_security().
> 
> But then, this is what anyway I was doing with the
> security_initxattrs() callback, for all callers of security_old_inode_i
> nit_security().
> 
> Also, security_old_inode_init_security() is exported to kernel modules.
> Maybe, it is used somewhere. So, unless we plan to remove it
> completely, it should be probably be fixed to avoid multiple LSMs
> successfully setting an xattr, and losing the memory of all except the
> last (which this patch fixes by calling security_inode_init_security())
> .
> 
> If there is still the preference, I will implement the reiserfs
> callback and make a fix for security_old_inode_init_security().

There's no sense in doing both, as the purpose of defining a reiserfs
initxattrs function was to clean up this code making it more readable.

Mimi

Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Mimi Zohar]

On Mon, 2022-11-21 at 14:29 +0100, Roberto Sassu wrote:
> On Fri, 2022-11-18 at 09:31 -0800, Casey Schaufler wrote:
> > On 11/18/2022 7:10 AM, Mimi Zohar wrote:
> > > On Fri, 2022-11-18 at 10:14 +0100, Roberto Sassu wrote:
> > > > > > +static int security_check_compact_xattrs(struct xattr *xattrs,
> > > > > > +                                     int num_xattrs, int *checked_xattrs)
> > > > > Perhaps the variable naming is off, making it difficult to read.   So
> > > > > although this is a static function, which normally doesn't require a
> > > > > comment, it's definitely needs one.
> > > > Ok, will improve it.
> > > > 
> > > > > > +{
> > > > > > +    int i;
> > > > > > +
> > > > > > +    for (i = *checked_xattrs; i < num_xattrs; i++) {
> > > > > If the number of "checked" xattrs was kept up to date, removing the
> > > > > empty xattr gaps wouldn't require a loop.  Is the purpose of this loop
> > > > > to support multiple per LSM xattrs?
> > > > An LSM might reserve one or more xattrs, but not set it/them (for 
> > > > example because it is not initialized). In this case, removing the gaps 
> > > > is needed for all subsequent LSMs.
> > > Including this sort of info in the function description or as a comment
> > > in the code would definitely simplify review.
> > > 
> > > security_check_compact_xattrs() is called in the loop after getting
> > > each LSM's xattr(s).  Only the current LSMs xattrs need to be
> > > compressed, yet the loop goes to the maximum number of xattrs each
> > > time. Just wondering if there is a way of improving it.
> > 
> > At security module registration each module could identify how
> > many xattrs it uses. That number could be used to limit the range
> > of the loop. I have to do similar things for the forthcoming LSM
> > syscalls and module stacking beyond that.
> 
> Yes, blob_sizes.lbs_xattr contains the total number of xattrs requested
> by LSMs. To stop the loop earlier, at the offset of the next LSM, we
> would need to search the LSM's lsm_info, using the LSM name in
> the security_hook_list structure. Although it is not optimal, not doing
> it makes the code simpler. I could do that, if preferred.

Either way is fine, as long as the code is readable.  At minimum add a
comment.

-- 
thanks,

Mimi

Re: [PATCH v4 1/5] reiserfs: Add missing calls to reiserfs_security_free()

[Paul Moore]

On Thu, Nov 10, 2022 at 4:47 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Commit 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes
> during inode creation") defined reiserfs_security_free() to free the name
> and value of a security xattr allocated by the active LSM through
> security_old_inode_init_security(). However, this function is not called
> in the reiserfs code.
>
> Thus, add a call to reiserfs_security_free() whenever
> reiserfs_security_init() is called, and initialize value to NULL, to avoid
> to call kfree() on an uninitialized pointer.
>
> Finally, remove the kfree() for the xattr name, as it is not allocated
> anymore.
>
> Fixes: 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes during inode creation")
> Cc: stable@vger.kernel.org
> Cc: Jeff Mahoney <jeffm@suse.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: Mimi Zohar <zohar@linux.ibm.com>
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  fs/reiserfs/namei.c          | 4 ++++
>  fs/reiserfs/xattr_security.c | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)

If I'm understanding this patch correctly, this is a standalone
bugfix, right?  Any reason this shouldn't be merged now, independent
of the rest of patches in this patchset?

> diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c
> index 3d7a35d6a18b..b916859992ec 100644
> --- a/fs/reiserfs/namei.c
> +++ b/fs/reiserfs/namei.c
> @@ -696,6 +696,7 @@ static int reiserfs_create(struct user_namespace *mnt_userns, struct inode *dir,
>
>  out_failed:
>         reiserfs_write_unlock(dir->i_sb);
> +       reiserfs_security_free(&security);
>         return retval;
>  }
>
> @@ -779,6 +780,7 @@ static int reiserfs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>
>  out_failed:
>         reiserfs_write_unlock(dir->i_sb);
> +       reiserfs_security_free(&security);
>         return retval;
>  }
>
> @@ -878,6 +880,7 @@ static int reiserfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>         retval = journal_end(&th);
>  out_failed:
>         reiserfs_write_unlock(dir->i_sb);
> +       reiserfs_security_free(&security);
>         return retval;
>  }
>
> @@ -1194,6 +1197,7 @@ static int reiserfs_symlink(struct user_namespace *mnt_userns,
>         retval = journal_end(&th);
>  out_failed:
>         reiserfs_write_unlock(parent_dir->i_sb);
> +       reiserfs_security_free(&security);
>         return retval;
>  }
>
> diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c
> index 8965c8e5e172..857a65b05726 100644
> --- a/fs/reiserfs/xattr_security.c
> +++ b/fs/reiserfs/xattr_security.c
> @@ -50,6 +50,7 @@ int reiserfs_security_init(struct inode *dir, struct inode *inode,
>         int error;
>
>         sec->name = NULL;
> +       sec->value = NULL;
>
>         /* Don't add selinux attributes on xattrs - they'll never get used */
>         if (IS_PRIVATE(dir))
> @@ -95,7 +96,6 @@ int reiserfs_security_write(struct reiserfs_transaction_handle *th,
>
>  void reiserfs_security_free(struct reiserfs_security_handle *sec)
>  {
> -       kfree(sec->name);
>         kfree(sec->value);
>         sec->name = NULL;
>         sec->value = NULL;
> --
> 2.25.1
>


-- 
paul-moore.com

Re: [PATCH v4 2/5] security: Rewrite security_old_inode_init_security()

[Paul Moore]

On Mon, Nov 21, 2022 at 3:54 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2022-11-21 at 10:45 +0100, Roberto Sassu wrote:
> > > As ocfs2 already defines initxattrs, that leaves only reiserfs missing
> > > initxattrs().  A better, cleaner solution would be to define one.
> >
> > If I understood why security_old_inode_init_security() is called
> > instead of security_inode_init_security(), the reason seems that the
> > filesystem code uses the length of the obtained xattr to make some
> > calculations (e.g. reserve space). The xattr is written at a later
> > time.
> >
> > Since for reiserfs there is a plan to deprecate it, it probably
> > wouldn't be worth to support the creation of multiple xattrs. I would
> > define a callback to take the first xattr and make a copy, so that
> > calling security_inode_init_security() + reiserfs_initxattrs() is
> > equivalent to calling security_old_inode_init_security().

FWIW, reiserfs isn't going to be removed until 2025, I'm hopeful we
can remove the IMA/EVM special cases before then :)

> > But then, this is what anyway I was doing with the
> > security_initxattrs() callback, for all callers of security_old_inode_i
> > nit_security().
> >
> > Also, security_old_inode_init_security() is exported to kernel modules.
> > Maybe, it is used somewhere. So, unless we plan to remove it
> > completely, it should be probably be fixed to avoid multiple LSMs
> > successfully setting an xattr, and losing the memory of all except the
> > last (which this patch fixes by calling security_inode_init_security()).

I would much rather remove security_old_inode_init_security() then
worry about what out-of-tree modules might be using it.  Hopefully we
can resolve the ocfs2 usage and get ocfs2 exclusively on the new hook
without too much trouble, which means all we have left is reiserfs ...
how difficult would you expect the conversion to be for reiserfs?

> > If there is still the preference, I will implement the reiserfs
> > callback and make a fix for security_old_inode_init_security().
>
> There's no sense in doing both, as the purpose of defining a reiserfs
> initxattrs function was to clean up this code making it more readable.
>
> Mimi

-- 
paul-moore.com

Re: [PATCH v4 1/5] reiserfs: Add missing calls to reiserfs_security_free()

[Roberto Sassu]

On Mon, 2022-11-21 at 18:41 -0500, Paul Moore wrote:
> On Thu, Nov 10, 2022 at 4:47 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Commit 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes
> > during inode creation") defined reiserfs_security_free() to free the name
> > and value of a security xattr allocated by the active LSM through
> > security_old_inode_init_security(). However, this function is not called
> > in the reiserfs code.
> > 
> > Thus, add a call to reiserfs_security_free() whenever
> > reiserfs_security_init() is called, and initialize value to NULL, to avoid
> > to call kfree() on an uninitialized pointer.
> > 
> > Finally, remove the kfree() for the xattr name, as it is not allocated
> > anymore.
> > 
> > Fixes: 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes during inode creation")
> > Cc: stable@vger.kernel.org
> > Cc: Jeff Mahoney <jeffm@suse.com>
> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Reported-by: Mimi Zohar <zohar@linux.ibm.com>
> > Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  fs/reiserfs/namei.c          | 4 ++++
> >  fs/reiserfs/xattr_security.c | 2 +-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> If I'm understanding this patch correctly, this is a standalone
> bugfix, right?  Any reason this shouldn't be merged now, independent
> of the rest of patches in this patchset?

Yes. It would be fine for me to pick this sooner.

Thanks

Roberto

> > diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c
> > index 3d7a35d6a18b..b916859992ec 100644
> > --- a/fs/reiserfs/namei.c
> > +++ b/fs/reiserfs/namei.c
> > @@ -696,6 +696,7 @@ static int reiserfs_create(struct user_namespace *mnt_userns, struct inode *dir,
> > 
> >  out_failed:
> >         reiserfs_write_unlock(dir->i_sb);
> > +       reiserfs_security_free(&security);
> >         return retval;
> >  }
> > 
> > @@ -779,6 +780,7 @@ static int reiserfs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> > 
> >  out_failed:
> >         reiserfs_write_unlock(dir->i_sb);
> > +       reiserfs_security_free(&security);
> >         return retval;
> >  }
> > 
> > @@ -878,6 +880,7 @@ static int reiserfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> >         retval = journal_end(&th);
> >  out_failed:
> >         reiserfs_write_unlock(dir->i_sb);
> > +       reiserfs_security_free(&security);
> >         return retval;
> >  }
> > 
> > @@ -1194,6 +1197,7 @@ static int reiserfs_symlink(struct user_namespace *mnt_userns,
> >         retval = journal_end(&th);
> >  out_failed:
> >         reiserfs_write_unlock(parent_dir->i_sb);
> > +       reiserfs_security_free(&security);
> >         return retval;
> >  }
> > 
> > diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c
> > index 8965c8e5e172..857a65b05726 100644
> > --- a/fs/reiserfs/xattr_security.c
> > +++ b/fs/reiserfs/xattr_security.c
> > @@ -50,6 +50,7 @@ int reiserfs_security_init(struct inode *dir, struct inode *inode,
> >         int error;
> > 
> >         sec->name = NULL;
> > +       sec->value = NULL;
> > 
> >         /* Don't add selinux attributes on xattrs - they'll never get used */
> >         if (IS_PRIVATE(dir))
> > @@ -95,7 +96,6 @@ int reiserfs_security_write(struct reiserfs_transaction_handle *th,
> > 
> >  void reiserfs_security_free(struct reiserfs_security_handle *sec)
> >  {
> > -       kfree(sec->name);
> >         kfree(sec->value);
> >         sec->name = NULL;
> >         sec->value = NULL;
> > --
> > 2.25.1
> > 
> 
> 

Re: [PATCH v4 2/5] security: Rewrite security_old_inode_init_security()

[Roberto Sassu]

On Mon, 2022-11-21 at 18:55 -0500, Paul Moore wrote:
> On Mon, Nov 21, 2022 at 3:54 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Mon, 2022-11-21 at 10:45 +0100, Roberto Sassu wrote:
> > > > As ocfs2 already defines initxattrs, that leaves only reiserfs missing
> > > > initxattrs().  A better, cleaner solution would be to define one.
> > > 
> > > If I understood why security_old_inode_init_security() is called
> > > instead of security_inode_init_security(), the reason seems that the
> > > filesystem code uses the length of the obtained xattr to make some
> > > calculations (e.g. reserve space). The xattr is written at a later
> > > time.
> > > 
> > > Since for reiserfs there is a plan to deprecate it, it probably
> > > wouldn't be worth to support the creation of multiple xattrs. I would
> > > define a callback to take the first xattr and make a copy, so that
> > > calling security_inode_init_security() + reiserfs_initxattrs() is
> > > equivalent to calling security_old_inode_init_security().
> 
> FWIW, reiserfs isn't going to be removed until 2025, I'm hopeful we
> can remove the IMA/EVM special cases before then :)

Well, we are not that far...

> > > But then, this is what anyway I was doing with the
> > > security_initxattrs() callback, for all callers of security_old_inode_i
> > > nit_security().
> > > 
> > > Also, security_old_inode_init_security() is exported to kernel modules.
> > > Maybe, it is used somewhere. So, unless we plan to remove it
> > > completely, it should be probably be fixed to avoid multiple LSMs
> > > successfully setting an xattr, and losing the memory of all except the
> > > last (which this patch fixes by calling security_inode_init_security()).
> 
> I would much rather remove security_old_inode_init_security() then
> worry about what out-of-tree modules might be using it.  Hopefully we
> can resolve the ocfs2 usage and get ocfs2 exclusively on the new hook
> without too much trouble, which means all we have left is reiserfs ...
> how difficult would you expect the conversion to be for reiserfs?

Ok for removing security_old_inode_init_security().

For reiserfs, I guess maintaining the current behavior of setting only
one xattr should be easy. For multiple xattrs, I need to understand
exactly how many blocks need to be reserved.

> > > If there is still the preference, I will implement the reiserfs
> > > callback and make a fix for security_old_inode_init_security().
> > 
> > There's no sense in doing both, as the purpose of defining a reiserfs
> > initxattrs function was to clean up this code making it more readable.

The fix for security_old_inode_init_security(), stopping at the first
LSM returning zero, was to avoid the memory leak. Will not be needed if
the function is removed.

Roberto

Re: [PATCH v4 1/5] reiserfs: Add missing calls to reiserfs_security_free()

[Paul Moore]

On Tue, Nov 22, 2022 at 3:12 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Mon, 2022-11-21 at 18:41 -0500, Paul Moore wrote:
> > On Thu, Nov 10, 2022 at 4:47 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > Commit 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes
> > > during inode creation") defined reiserfs_security_free() to free the name
> > > and value of a security xattr allocated by the active LSM through
> > > security_old_inode_init_security(). However, this function is not called
> > > in the reiserfs code.
> > >
> > > Thus, add a call to reiserfs_security_free() whenever
> > > reiserfs_security_init() is called, and initialize value to NULL, to avoid
> > > to call kfree() on an uninitialized pointer.
> > >
> > > Finally, remove the kfree() for the xattr name, as it is not allocated
> > > anymore.
> > >
> > > Fixes: 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes during inode creation")
> > > Cc: stable@vger.kernel.org
> > > Cc: Jeff Mahoney <jeffm@suse.com>
> > > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > > Reported-by: Mimi Zohar <zohar@linux.ibm.com>
> > > Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  fs/reiserfs/namei.c          | 4 ++++
> > >  fs/reiserfs/xattr_security.c | 2 +-
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > If I'm understanding this patch correctly, this is a standalone
> > bugfix, right?  Any reason this shouldn't be merged now, independent
> > of the rest of patches in this patchset?
>
> Yes. It would be fine for me to pick this sooner.

Okay, as it's been almost two weeks with no comments from the reiserfs
folks and this looks okay to me I'm going to go ahead and pull this
into the lsm/next branch as it's at least "LSM adjacent" :)  As it is
lsm/next and not lsm/stable-6.1, this should give the reiserfs folks
another couple of weeks to object if they find this to be problematic.

Thanks all.

-- 
paul-moore.com