From: Roberto Sassu <[email protected]>
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
From: Roberto Sassu <[email protected]>
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 <[email protected]>
---
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
From: Roberto Sassu <[email protected]>
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 <[email protected]>
---
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
From: Roberto Sassu <[email protected]>
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: [email protected]
Cc: Jeff Mahoney <[email protected]>
Cc: Tetsuo Handa <[email protected]>
Reported-by: Mimi Zohar <[email protected]>
Reported-by: Tetsuo Handa <[email protected]>
Signed-off-by: Roberto Sassu <[email protected]>
---
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
On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> 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: [email protected]
> Cc: Jeff Mahoney <[email protected]>
> Cc: Tetsuo Handa <[email protected]>
> Reported-by: Mimi Zohar <[email protected]>
> Reported-by: Tetsuo Handa <[email protected]>
> Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Mimi Zohar <[email protected]>
Hi Roberto,
On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> 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 <[email protected]>b
On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> 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 <[email protected]>
Thanks, Roberto. This looks good.
Mimi
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 <[email protected]>
>>
>> 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 <[email protected]>b
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 <[email protected]>
> >
> > 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 <[email protected]>b
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
On Mon, Nov 21, 2022 at 3:54 PM Mimi Zohar <[email protected]> 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
On Thu, Nov 10, 2022 at 4:47 AM Roberto Sassu
<[email protected]> wrote:
>
> From: Roberto Sassu <[email protected]>
>
> 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: [email protected]
> Cc: Jeff Mahoney <[email protected]>
> Cc: Tetsuo Handa <[email protected]>
> Reported-by: Mimi Zohar <[email protected]>
> Reported-by: Tetsuo Handa <[email protected]>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> 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
On Mon, 2022-11-21 at 18:55 -0500, Paul Moore wrote:
> On Mon, Nov 21, 2022 at 3:54 PM Mimi Zohar <[email protected]> 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
On Mon, 2022-11-21 at 18:41 -0500, Paul Moore wrote:
> On Thu, Nov 10, 2022 at 4:47 AM Roberto Sassu
> <[email protected]> wrote:
> > From: Roberto Sassu <[email protected]>
> >
> > 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: [email protected]
> > Cc: Jeff Mahoney <[email protected]>
> > Cc: Tetsuo Handa <[email protected]>
> > Reported-by: Mimi Zohar <[email protected]>
> > Reported-by: Tetsuo Handa <[email protected]>
> > Signed-off-by: Roberto Sassu <[email protected]>
> > ---
> > 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
> >
>
>
On Tue, Nov 22, 2022 at 3:12 AM Roberto Sassu
<[email protected]> wrote:
> On Mon, 2022-11-21 at 18:41 -0500, Paul Moore wrote:
> > On Thu, Nov 10, 2022 at 4:47 AM Roberto Sassu
> > <[email protected]> wrote:
> > > From: Roberto Sassu <[email protected]>
> > >
> > > 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: [email protected]
> > > Cc: Jeff Mahoney <[email protected]>
> > > Cc: Tetsuo Handa <[email protected]>
> > > Reported-by: Mimi Zohar <[email protected]>
> > > Reported-by: Tetsuo Handa <[email protected]>
> > > Signed-off-by: Roberto Sassu <[email protected]>
> > > ---
> > > 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