* [PATCH v2 0/6] evm: Prepare for moving to the LSM infrastructure
@ 2021-04-21 16:19 Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 1/6] xattr: Complete constify ->name member of "struct xattr" Roberto Sassu
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Roberto Sassu @ 2021-04-21 16:19 UTC (permalink / raw)
To: zohar, jmorris, paul, casey
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Roberto Sassu
This patch set depends on:
https://lore.kernel.org/linux-integrity/20210409114313.4073-1-roberto.sassu@huawei.com/
https://lore.kernel.org/linux-integrity/20210407105252.30721-1-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() and
security_old_inode_init_security() are currently limited to support one
xattr provided by LSM and one by EVM.
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, EVM must know how many elements are in the xattr array. It seems
that it is not necessary to add another parameter, as all filesystems that
define an initxattr function expect that the last element of the array is
one with the name field set to NULL. EVM reuses the same assumption.
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). The patch is not included
in this set but it is available here:
https://github.com/robertosassu/linux/commit/dbe867fdf0b61b9bd73332f159e7ce5ca2a9b980
The test, added to ima-evm-utils, is available here:
https://github.com/robertosassu/ima-evm-utils/blob/evm-multiple-lsms-v2-devel-v2/tests/evm_multiple_lsms.test
The test takes a UML kernel built by Travis 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://travis-ci.com/github/robertosassu/ima-evm-utils/jobs/500142666
SELinux Test Suite result (diff 5.11.14-200.fc33.x86_64 5.12.0-rc8+):
-Files=70, Tests=1099, 82 wallclock secs ( 0.35 usr 0.09 sys + 7.39 cusr 10.14 csys = 17.97 CPU)
+Files=70, Tests=1108, 104 wallclock secs ( 0.35 usr 0.09 sys + 7.35 cusr 11.35 csys = 19.14 CPU)
Result: FAIL
-Failed 2/70 test programs. 5/1099 subtests failed.
+Failed 2/70 test programs. 5/1108 subtests failed.
Smack Test Suite result:
smack_set_ambient 1 TPASS: Test "smack_set_ambient" success.
smack_set_current 1 TPASS: Test "smack_set_current" success.
smack_set_doi 1 TPASS: Test "smack_set_doi" success.
smack_set_netlabel 1 TPASS: Test "smack_set_netlabel" success.
smack_set_socket_labels 1 TPASS : Test smack_set_socket_labels success.
smack_set_cipso 1 TPASS: Test "smack_set_cipso" success.
smack_set_direct 1 TPASS: Test "smack_set_direct" success.
smack_set_load 1 TPASS: Test "smack_set_load" success.
smack_set_onlycap 1 TFAIL: The smack label reported for "/smack/onlycap"
Lastly, running the test on reiserfs to check
security_old_inode_init_security(), some issues have been discovered: a
free of xattr->name which is not correct after commit 9548906b2bb7 ('xattr:
Constify ->name member of "struct xattr"'), missing calls to
reiserfs_security_free() and a misalignment with
security_inode_init_security() (the old version expects the full xattr name
with the security. prefix, the new version just the suffix). The last issue
has not been fixed yet.
Changelog
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 (6):
xattr: Complete constify ->name member of "struct xattr"
reiserfs: Add missing calls to reiserfs_security_free()
security: Pass xattrs allocated by LSMs to the inode_init_security
hook
security: Support multiple LSMs implementing the 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 | 3 +-
include/linux/evm.h | 17 +++--
include/linux/lsm_hook_defs.h | 4 +-
include/linux/lsm_hooks.h | 22 ++++--
security/integrity/evm/evm.h | 2 +
security/integrity/evm/evm_crypto.c | 9 ++-
security/integrity/evm/evm_main.c | 28 +++++---
security/security.c | 105 +++++++++++++++++++++++-----
security/selinux/hooks.c | 13 ++--
security/smack/smack_lsm.c | 20 +++---
11 files changed, 167 insertions(+), 60 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/6] xattr: Complete constify ->name member of "struct xattr"
2021-04-21 16:19 [PATCH v2 0/6] evm: Prepare for moving to the LSM infrastructure Roberto Sassu
@ 2021-04-21 16:19 ` Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 2/6] reiserfs: Add missing calls to reiserfs_security_free() Roberto Sassu
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Roberto Sassu @ 2021-04-21 16:19 UTC (permalink / raw)
To: zohar, jmorris, paul, casey
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Roberto Sassu, stable, Jeff Mahoney,
Tetsuo Handa
This patch completes commit 9548906b2bb7 ('xattr: Constify ->name member of
"struct xattr"'). It fixes the documentation of the inode_init_security
hook, by removing the xattr name from the objects that are expected to be
allocated by LSMs (only the value is allocated).
Also, it removes the kfree() of name and setting it to NULL in
reiserfs_security_free().
Fixes: 9548906b2bb7 ('xattr: Constify ->name member of "struct xattr"')
Cc: stable@vger.kernel.org
Cc: Jeff Mahoney <jeffm@suse.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
fs/reiserfs/xattr_security.c | 2 --
include/linux/lsm_hooks.h | 4 ++--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c
index 8965c8e5e172..bb2a0062e0e5 100644
--- a/fs/reiserfs/xattr_security.c
+++ b/fs/reiserfs/xattr_security.c
@@ -95,9 +95,7 @@ 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;
}
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index fb7f3193753d..c5498f5174ce 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -219,8 +219,8 @@
* 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.
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/6] reiserfs: Add missing calls to reiserfs_security_free()
2021-04-21 16:19 [PATCH v2 0/6] evm: Prepare for moving to the LSM infrastructure Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 1/6] xattr: Complete constify ->name member of "struct xattr" Roberto Sassu
@ 2021-04-21 16:19 ` Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook Roberto Sassu
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Roberto Sassu @ 2021-04-21 16:19 UTC (permalink / raw)
To: zohar, jmorris, paul, casey
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Roberto Sassu, stable, Jeff Mahoney,
Tetsuo Handa
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, this patch adds a call to reiserfs_security_free() whenever
reiserfs_security_init() is called, and initializes value to NULL, to avoid
to call kfree() on an uninitialized pointer.
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 | 1 +
2 files changed, 5 insertions(+)
diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c
index e6eb05e2b2f1..6b5c51a77fae 100644
--- a/fs/reiserfs/namei.c
+++ b/fs/reiserfs/namei.c
@@ -695,6 +695,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;
}
@@ -778,6 +779,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;
}
@@ -877,6 +879,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;
}
@@ -1193,6 +1196,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 bb2a0062e0e5..b1ad93b60475 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))
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook
2021-04-21 16:19 [PATCH v2 0/6] evm: Prepare for moving to the LSM infrastructure Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 1/6] xattr: Complete constify ->name member of "struct xattr" Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 2/6] reiserfs: Add missing calls to reiserfs_security_free() Roberto Sassu
@ 2021-04-21 16:19 ` Roberto Sassu
2021-04-21 22:43 ` Casey Schaufler
2021-04-21 16:19 ` [PATCH v2 4/6] security: Support multiple LSMs implementing " Roberto Sassu
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Roberto Sassu @ 2021-04-21 16:19 UTC (permalink / raw)
To: zohar, jmorris, paul, casey
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Roberto Sassu
In preparation for moving EVM to the LSM infrastructure, this patch
replaces the name, value, len triple with the xattr array pointer provided
by security_inode_init_security(). LSMs are expected to call the new
function lsm_find_xattr_slot() to find the first unused slot of the array
where the xattr should be written.
This patch modifies also SELinux and Smack to search for an unused slot, to
have a consistent behavior across LSMs (the unmodified version would
overwrite the xattr set by the first LSM in the chain). It is also
desirable to have the modification in those LSMs, as they are likely used
as a reference for the development of new LSMs.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
include/linux/lsm_hook_defs.h | 4 ++--
include/linux/lsm_hooks.h | 18 +++++++++++++++---
security/security.c | 13 +++++++------
security/selinux/hooks.c | 13 ++++++-------
security/smack/smack_lsm.c | 20 +++++++++-----------
5 files changed, 39 insertions(+), 29 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 477a597db013..afb9dd122f60 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -111,8 +111,8 @@ 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,
+ void *fs_data)
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 c5498f5174ce..e8c9bac29b9d 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -27,6 +27,7 @@
#include <linux/security.h>
#include <linux/init.h>
+#include <linux/xattr.h>
#include <linux/rculist.h>
/**
@@ -227,9 +228,11 @@
* @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.
+ * @xattrs contains the full array of xattrs allocated by LSMs where
+ * ->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.
+ * @fs_data contains filesystem-specific data.
* Returns 0 if @name and @value have been successfully set,
* -EOPNOTSUPP if no security attribute is needed, or
* -ENOMEM on memory allocation failure.
@@ -1661,4 +1664,13 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
extern int lsm_inode_alloc(struct inode *inode);
+static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs)
+{
+ struct xattr *slot;
+
+ for (slot = xattrs; slot && slot->name != NULL; slot++)
+ ;
+
+ return slot;
+}
#endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/security.c b/security/security.c
index 7f14e59c4f8e..2c1fe1496069 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1037,18 +1037,16 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
if (!initxattrs)
return call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
- dir, qstr, NULL, NULL, NULL);
+ dir, qstr, NULL, fs_data);
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);
+ lsm_xattr, fs_data);
if (ret)
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, evm_xattr);
if (ret)
goto out;
ret = initxattrs(inode, new_xattrs, fs_data);
@@ -1071,10 +1069,13 @@ int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
+ struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 };
+ struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL;
+
if (unlikely(IS_PRIVATE(inode)))
return -EOPNOTSUPP;
return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
- qstr, name, value, len);
+ qstr, lsm_xattr, NULL);
}
EXPORT_SYMBOL(security_old_inode_init_security);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ddd097790d47..806827eb132a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2916,11 +2916,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, void *fs_data)
{
const struct task_security_struct *tsec = selinux_cred(current_cred());
struct superblock_security_struct *sbsec;
+ struct xattr *xattr = lsm_find_xattr_slot(xattrs);
u32 newsid, clen;
int rc;
char *context;
@@ -2947,16 +2947,15 @@ 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 (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;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 12a45e61c1a5..af7eee0fee52 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -962,26 +962,24 @@ 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, void *fs_data)
{
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 = lsm_find_xattr_slot(xattrs);
int may;
- if (name)
- *name = XATTR_SMACK_SUFFIX;
+ 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);
@@ -999,11 +997,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;
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 4/6] security: Support multiple LSMs implementing the inode_init_security hook
2021-04-21 16:19 [PATCH v2 0/6] evm: Prepare for moving to the LSM infrastructure Roberto Sassu
` (2 preceding siblings ...)
2021-04-21 16:19 ` [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook Roberto Sassu
@ 2021-04-21 16:19 ` Roberto Sassu
2021-04-21 23:09 ` Casey Schaufler
2021-04-21 16:19 ` [PATCH v2 5/6] evm: Align evm_inode_init_security() definition with LSM infrastructure Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 6/6] evm: Support multiple LSMs providing an xattr Roberto Sassu
5 siblings, 1 reply; 13+ messages in thread
From: Roberto Sassu @ 2021-04-21 16:19 UTC (permalink / raw)
To: zohar, jmorris, paul, casey
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Roberto Sassu
The current implementation of security_inode_init_security() is capable of
handling only one LSM providing an xattr to be set at inode creation. That
xattr is then passed to EVM to calculate the HMAC.
To support multiple LSMs, each providing an xattr, this patch makes the
following modifications:
security_inode_init_security():
- dynamically allocates new_xattrs, based on the number of
inode_init_security hooks registered by LSMs;
- replaces the call_int_hook() macro with its definition, to correctly
handle the case of an LSM returning -EOPNOTSUPP (the loop should not be
stopped).
security_old_inode_init_security():
- replaces the call_int_hook() macro with its definition, to stop the loop
at the first LSM providing an xattr, to avoid a memory leak (due to
overwriting the *value pointer).
The modifications necessary for EVM to calculate the HMAC on all xattrs
will be done in a separate patch.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/security.c | 93 +++++++++++++++++++++++++++++++++++++++------
1 file changed, 82 insertions(+), 11 deletions(-)
diff --git a/security/security.c b/security/security.c
index 2c1fe1496069..2ab67fa4422e 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)
@@ -1028,9 +1026,10 @@ 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 *new_xattrs;
struct xattr *lsm_xattr, *evm_xattr, *xattr;
- int ret;
+ struct security_hook_list *P;
+ int ret, max_new_xattrs = 0;
if (unlikely(IS_PRIVATE(inode)))
return 0;
@@ -1038,14 +1037,52 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
if (!initxattrs)
return call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
dir, qstr, NULL, fs_data);
- memset(new_xattrs, 0, sizeof(new_xattrs));
+
+ /* Determine at run-time the max number of xattr structs to allocate. */
+ hlist_for_each_entry(P, &security_hook_heads.inode_init_security, list)
+ max_new_xattrs++;
+
+ if (!max_new_xattrs)
+ return 0;
+
+ /* Allocate +1 for EVM and +1 as terminator. */
+ new_xattrs = kcalloc(max_new_xattrs + 2, sizeof(*new_xattrs), GFP_NOFS);
+ if (!new_xattrs)
+ return -ENOMEM;
+
lsm_xattr = new_xattrs;
- ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
- lsm_xattr, fs_data);
- if (ret)
+ hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
+ list) {
+ ret = P->hook.inode_init_security(inode, dir, qstr, new_xattrs,
+ fs_data);
+ if (ret) {
+ if (ret != -EOPNOTSUPP)
+ goto out;
+
+ continue;
+ }
+
+ /* LSM implementation error. */
+ if (lsm_xattr->name == NULL || lsm_xattr->value == NULL) {
+ WARN_ONCE(
+ "LSM %s: ret = 0 but xattr name/value = NULL\n",
+ P->lsm);
+ ret = -ENOENT;
+ goto out;
+ }
+
+ lsm_xattr++;
+
+ if (!--max_new_xattrs)
+ break;
+ }
+
+ if (!new_xattrs->name) {
+ ret = -EOPNOTSUPP;
goto out;
+ }
- evm_xattr = lsm_xattr + 1;
+ evm_xattr = lsm_xattr;
ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
if (ret)
goto out;
@@ -1053,6 +1090,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
out:
for (xattr = new_xattrs; xattr->value != NULL; xattr++)
kfree(xattr->value);
+ kfree(new_xattrs);
return (ret == -EOPNOTSUPP) ? 0 : ret;
}
EXPORT_SYMBOL(security_inode_init_security);
@@ -1071,11 +1109,44 @@ int security_old_inode_init_security(struct inode *inode, struct inode *dir,
{
struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 };
struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL;
+ struct security_hook_list *P;
+ int ret;
if (unlikely(IS_PRIVATE(inode)))
return -EOPNOTSUPP;
- return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
- qstr, lsm_xattr, NULL);
+
+ hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
+ list) {
+ ret = P->hook.inode_init_security(inode, dir, qstr, lsm_xattr,
+ NULL);
+ if (ret) {
+ if (ret != -EOPNOTSUPP)
+ return ret;
+
+ continue;
+ }
+
+ if (!lsm_xattr)
+ continue;
+
+ /* LSM implementation error. */
+ if (lsm_xattr->name == NULL || lsm_xattr->value == NULL) {
+ WARN_ONCE(
+ "LSM %s: ret = 0 but xattr name/value = NULL\n",
+ P->lsm);
+
+ /* Callers should do the cleanup. */
+ return -ENOENT;
+ }
+
+ *name = lsm_xattr->name;
+ *value = lsm_xattr->value;
+ *len = lsm_xattr->value_len;
+
+ return ret;
+ }
+
+ return -EOPNOTSUPP;
}
EXPORT_SYMBOL(security_old_inode_init_security);
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 5/6] evm: Align evm_inode_init_security() definition with LSM infrastructure
2021-04-21 16:19 [PATCH v2 0/6] evm: Prepare for moving to the LSM infrastructure Roberto Sassu
` (3 preceding siblings ...)
2021-04-21 16:19 ` [PATCH v2 4/6] security: Support multiple LSMs implementing " Roberto Sassu
@ 2021-04-21 16:19 ` Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 6/6] evm: Support multiple LSMs providing an xattr Roberto Sassu
5 siblings, 0 replies; 13+ messages in thread
From: Roberto Sassu @ 2021-04-21 16:19 UTC (permalink / raw)
To: zohar, jmorris, paul, casey
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Roberto Sassu
This patch changes the evm_inode_init_security() definition to align with
the LSM infrastructure, in preparation for moving IMA and EVM to that
infrastructure.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
include/linux/evm.h | 17 ++++++++++-------
security/integrity/evm/evm_main.c | 17 +++++++++++------
security/security.c | 7 +++----
3 files changed, 24 insertions(+), 17 deletions(-)
diff --git a/include/linux/evm.h b/include/linux/evm.h
index 8cad46bcec9d..b6c4092426f3 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -34,9 +34,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, void *fs_data);
extern bool evm_status_revalidate(const char *xattr_name);
#ifdef CONFIG_FS_POSIX_ACL
extern int posix_xattr_acl(const char *xattrname);
@@ -102,11 +102,14 @@ 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, void *fs_data)
{
- return 0;
+ if (!xattrs || !xattrs->name)
+ return 0;
+
+ return -EOPNOTSUPP;
}
static inline bool evm_status_revalidate(const char *xattr_name)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 84a9b7a69b1f..336a421e2e5a 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -17,6 +17,7 @@
#include <linux/xattr.h>
#include <linux/integrity.h>
#include <linux/evm.h>
+#include <linux/lsm_hooks.h>
#include <linux/magic.h>
#include <linux/posix_acl_xattr.h>
@@ -706,23 +707,27 @@ 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, void *fs_data)
{
struct evm_xattr *xattr_data;
+ struct xattr *evm_xattr = lsm_find_xattr_slot(xattrs);
int rc;
- if (!(evm_initialized & EVM_INIT_HMAC) ||
- !evm_protected_xattr(lsm_xattr->name))
+ if (!xattrs || !xattrs->name)
return 0;
+ if (!(evm_initialized & EVM_INIT_HMAC) ||
+ !evm_protected_xattr(xattrs->name))
+ return -EOPNOTSUPP;
+
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 2ab67fa4422e..ee5b9caccc40 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1027,7 +1027,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
const initxattrs initxattrs, void *fs_data)
{
struct xattr *new_xattrs;
- struct xattr *lsm_xattr, *evm_xattr, *xattr;
+ struct xattr *lsm_xattr, *xattr;
struct security_hook_list *P;
int ret, max_new_xattrs = 0;
@@ -1082,9 +1082,8 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
goto out;
}
- evm_xattr = lsm_xattr;
- ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
- if (ret)
+ ret = evm_inode_init_security(inode, dir, qstr, new_xattrs, fs_data);
+ if (ret && ret != -EOPNOTSUPP)
goto out;
ret = initxattrs(inode, new_xattrs, fs_data);
out:
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 6/6] evm: Support multiple LSMs providing an xattr
2021-04-21 16:19 [PATCH v2 0/6] evm: Prepare for moving to the LSM infrastructure Roberto Sassu
` (4 preceding siblings ...)
2021-04-21 16:19 ` [PATCH v2 5/6] evm: Align evm_inode_init_security() definition with LSM infrastructure Roberto Sassu
@ 2021-04-21 16:19 ` Roberto Sassu
5 siblings, 0 replies; 13+ messages in thread
From: Roberto Sassu @ 2021-04-21 16:19 UTC (permalink / raw)
To: zohar, jmorris, paul, casey
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Roberto Sassu
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(), called by security_inode_init_security(), expects
that this array is terminated when the xattr name is set to NULL, this
patch reuses the same assumption for evm_inode_init_security() 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 | 15 +++++++++++----
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index ae590f71ce7d..24eac42b9f32 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -49,6 +49,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 __init evm_init_crypto(void);
int evm_update_evmxattr(struct dentry *dentry,
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index b66264b53d5d..35c5eec0517d 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -358,6 +358,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, evm_hash_algo);
if (IS_ERR(desc)) {
@@ -365,7 +366,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 336a421e2e5a..c43e75cd37f3 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -261,7 +261,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
return evm_status;
}
-static int evm_protected_xattr(const char *req_xattr_name)
+int evm_protected_xattr(const char *req_xattr_name)
{
int namelen;
int found = 0;
@@ -712,14 +712,21 @@ int evm_inode_init_security(struct inode *inode, struct inode *dir,
struct xattr *xattrs, void *fs_data)
{
struct evm_xattr *xattr_data;
+ struct xattr *xattr;
struct xattr *evm_xattr = lsm_find_xattr_slot(xattrs);
- int rc;
+ int rc, evm_protected_xattrs = 0;
if (!xattrs || !xattrs->name)
return 0;
- if (!(evm_initialized & EVM_INIT_HMAC) ||
- !evm_protected_xattr(xattrs->name))
+ if (!(evm_initialized & EVM_INIT_HMAC))
+ return -EOPNOTSUPP;
+
+ for (xattr = xattrs; xattr->name != NULL; xattr++)
+ if (evm_protected_xattr(xattr->name))
+ evm_protected_xattrs++;
+
+ if (!evm_protected_xattrs)
return -EOPNOTSUPP;
xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook
2021-04-21 16:19 ` [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook Roberto Sassu
@ 2021-04-21 22:43 ` Casey Schaufler
2021-04-22 13:46 ` Roberto Sassu
0 siblings, 1 reply; 13+ messages in thread
From: Casey Schaufler @ 2021-04-21 22:43 UTC (permalink / raw)
To: Roberto Sassu, zohar, jmorris, paul
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Casey Schaufler
On 4/21/2021 9:19 AM, Roberto Sassu wrote:
> In preparation for moving EVM to the LSM infrastructure, this patch
> replaces the name, value, len triple with the xattr array pointer provided
> by security_inode_init_security(). LSMs are expected to call the new
> function lsm_find_xattr_slot() to find the first unused slot of the array
> where the xattr should be written.
>
> This patch modifies also SELinux and Smack to search for an unused slot, to
> have a consistent behavior across LSMs (the unmodified version would
> overwrite the xattr set by the first LSM in the chain). It is also
> desirable to have the modification in those LSMs, as they are likely used
> as a reference for the development of new LSMs.
This looks better than V1. One safety comment below.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> include/linux/lsm_hook_defs.h | 4 ++--
> include/linux/lsm_hooks.h | 18 +++++++++++++++---
> security/security.c | 13 +++++++------
> security/selinux/hooks.c | 13 ++++++-------
> security/smack/smack_lsm.c | 20 +++++++++-----------
> 5 files changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 477a597db013..afb9dd122f60 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -111,8 +111,8 @@ 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,
> + void *fs_data)
> 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 c5498f5174ce..e8c9bac29b9d 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -27,6 +27,7 @@
>
> #include <linux/security.h>
> #include <linux/init.h>
> +#include <linux/xattr.h>
> #include <linux/rculist.h>
>
> /**
> @@ -227,9 +228,11 @@
> * @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.
> + * @xattrs contains the full array of xattrs allocated by LSMs where
> + * ->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.
> + * @fs_data contains filesystem-specific data.
> * Returns 0 if @name and @value have been successfully set,
> * -EOPNOTSUPP if no security attribute is needed, or
> * -ENOMEM on memory allocation failure.
> @@ -1661,4 +1664,13 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
>
> extern int lsm_inode_alloc(struct inode *inode);
>
Some "security researcher" with a fuzz tester is going to manage to dump junk
into the slots and ruin your week. I suggest a simple change to make bounds checking
possible. It should never happen, but if that was sufficient people would
love C
string processing better.
> +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs)
+static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs, int available)
> +{
> + struct xattr *slot;
> +
> + for (slot = xattrs; slot && slot->name != NULL; slot++)
+ for (slot = xattrs; slot && slot->name != NULL; slot++)
if (WARN_ON(slot > xattrs[available]))
return NULL;
> + ;
> +
> + return slot;
> +}
> #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/security/security.c b/security/security.c
> index 7f14e59c4f8e..2c1fe1496069 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1037,18 +1037,16 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>
> if (!initxattrs)
> return call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
> - dir, qstr, NULL, NULL, NULL);
> + dir, qstr, NULL, fs_data);
> 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);
> + lsm_xattr, fs_data);
> if (ret)
> 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, evm_xattr);
> if (ret)
> goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> @@ -1071,10 +1069,13 @@ int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> + struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 };
> + struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL;
> +
> if (unlikely(IS_PRIVATE(inode)))
> return -EOPNOTSUPP;
> return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
> - qstr, name, value, len);
> + qstr, lsm_xattr, NULL);
> }
> EXPORT_SYMBOL(security_old_inode_init_security);
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ddd097790d47..806827eb132a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2916,11 +2916,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, void *fs_data)
> {
> const struct task_security_struct *tsec = selinux_cred(current_cred());
> struct superblock_security_struct *sbsec;
> + struct xattr *xattr = lsm_find_xattr_slot(xattrs);
> u32 newsid, clen;
> int rc;
> char *context;
> @@ -2947,16 +2947,15 @@ 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 (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;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 12a45e61c1a5..af7eee0fee52 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -962,26 +962,24 @@ 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, void *fs_data)
> {
> 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 = lsm_find_xattr_slot(xattrs);
> int may;
>
> - if (name)
> - *name = XATTR_SMACK_SUFFIX;
> + 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);
> @@ -999,11 +997,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;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/6] security: Support multiple LSMs implementing the inode_init_security hook
2021-04-21 16:19 ` [PATCH v2 4/6] security: Support multiple LSMs implementing " Roberto Sassu
@ 2021-04-21 23:09 ` Casey Schaufler
0 siblings, 0 replies; 13+ messages in thread
From: Casey Schaufler @ 2021-04-21 23:09 UTC (permalink / raw)
To: Roberto Sassu, zohar, jmorris, paul
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Casey Schaufler
On 4/21/2021 9:19 AM, Roberto Sassu wrote:
> The current implementation of security_inode_init_security() is capable
of
> handling only one LSM providing an xattr to be set at inode creation. That
> xattr is then passed to EVM to calculate the HMAC.
>
> To support multiple LSMs, each providing an xattr, this patch makes the
> following modifications:
>
> security_inode_init_security():
> - dynamically allocates new_xattrs, based on the number of
> inode_init_security hooks registered by LSMs;
> - replaces the call_int_hook() macro with its definition, to correctly
> handle the case of an LSM returning -EOPNOTSUPP (the loop should not be
> stopped).
>
> security_old_inode_init_security():
> - replaces the call_int_hook() macro with its definition, to stop the loop
> at the first LSM providing an xattr, to avoid a memory leak (due to
> overwriting the *value pointer).
>
> The modifications necessary for EVM to calculate the HMAC on all xattrs
> will be done in a separate patch.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> security/security.c | 93 +++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 82 insertions(+), 11 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 2c1fe1496069..2ab67fa4422e 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)
>
> @@ -1028,9 +1026,10 @@ 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 *new_xattrs;
> struct xattr *lsm_xattr, *evm_xattr, *xattr;
> - int ret;
> + struct security_hook_list *P;
> + int ret, max_new_xattrs = 0;
>
> if (unlikely(IS_PRIVATE(inode)))
> return 0;
> @@ -1038,14 +1037,52 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> if (!initxattrs)
> return call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
> dir, qstr, NULL, fs_data);
> - memset(new_xattrs, 0, sizeof(new_xattrs));
> +
> + /* Determine at run-time the max number of xattr structs to allocate.
*/
> + hlist_for_each_entry(P, &security_hook_heads.inode_init_security, list)
> + max_new_xattrs++;
Don't do this here! You can count the number of modules providing inode_init_security
hooks when the modules register and save the value for use here. It's not
going to
change.
> +
> + if (!max_new_xattrs)
> + return 0;
> +
> + /* Allocate +1 for EVM and +1 as terminator. */
> + new_xattrs = kcalloc(max_new_xattrs + 2, sizeof(*new_xattrs), GFP_NOFS);
> + if (!new_xattrs)
> + return -ENOMEM;
> +
> lsm_xattr = new_xattrs;
> - ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> - lsm_xattr, fs_data);
> - if (ret)
> + hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
> + list) {
> + ret = P->hook.inode_init_security(inode, dir, qstr, new_xattrs,
> + fs_data);
> + if (ret) {
> + if (ret != -EOPNOTSUPP)
> + goto out;
> +
> + continue;
> + }
> +
> + /* LSM implementation error. */
> + if (lsm_xattr->name == NULL || lsm_xattr->value == NULL) {
> + WARN_ONCE(
> + "LSM %s: ret = 0 but xattr name/value = NULL\n",
> + P->lsm);
> + ret = -ENOENT;
> + goto out;
> + }
> +
> + lsm_xattr++;
> +
> + if (!--max_new_xattrs)
> + break;
> + }
> +
> + if (!new_xattrs->name) {
> + ret = -EOPNOTSUPP;
> goto out;
> + }
>
> - evm_xattr = lsm_xattr + 1;
> + evm_xattr = lsm_xattr;
> ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
+ ret = evm_inode_init_security(inode, new_xattrs, lsm_xattr);
then you don't need evm_xattr at all.
> if (ret)
> goto out;
> @@ -1053,6 +1090,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> out:
> for (xattr = new_xattrs; xattr->value != NULL; xattr++)
> kfree(xattr->value);
> + kfree(new_xattrs);
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
> @@ -1071,11 +1109,44 @@ int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> {
> struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 };
> struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL;
> + struct security_hook_list *P;
> + int ret;
>
> if (unlikely(IS_PRIVATE(inode)))
> return -EOPNOTSUPP;
> - return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
> - qstr, lsm_xattr, NULL);
> +
> + hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
> + list) {
> + ret = P->hook.inode_init_security(inode, dir, qstr, lsm_xattr,
> + NULL);
> + if (ret) {
> + if (ret != -EOPNOTSUPP)
> + return ret;
> +
> + continue;
> + }
> +
> + if (!lsm_xattr)
> + continue;
> +
> + /* LSM implementation error. */
> + if (lsm_xattr->name == NULL || lsm_xattr->value == NULL) {
> + WARN_ONCE(
> + "LSM %s: ret = 0 but xattr name/value = NULL\n",
> + P->lsm);
> +
> + /* Callers should do the cleanup. */
> + return -ENOENT;
> + }
> +
> + *name = lsm_xattr->name;
> + *value = lsm_xattr->value;
> + *len = lsm_xattr->value_len;
> +
> + return ret;
> + }
> +
> + return -EOPNOTSUPP;
> }
> EXPORT_SYMBOL(security_old_inode_init_security);
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook
2021-04-21 22:43 ` Casey Schaufler
@ 2021-04-22 13:46 ` Roberto Sassu
2021-04-22 15:46 ` Casey Schaufler
0 siblings, 1 reply; 13+ messages in thread
From: Roberto Sassu @ 2021-04-22 13:46 UTC (permalink / raw)
To: Casey Schaufler, zohar, jmorris, paul
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel
> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Thursday, April 22, 2021 12:44 AM
> On 4/21/2021 9:19 AM, Roberto Sassu wrote:
> > In preparation for moving EVM to the LSM infrastructure, this patch
> > replaces the name, value, len triple with the xattr array pointer provided
> > by security_inode_init_security(). LSMs are expected to call the new
> > function lsm_find_xattr_slot() to find the first unused slot of the array
> > where the xattr should be written.
> >
> > This patch modifies also SELinux and Smack to search for an unused slot, to
> > have a consistent behavior across LSMs (the unmodified version would
> > overwrite the xattr set by the first LSM in the chain). It is also
> > desirable to have the modification in those LSMs, as they are likely used
> > as a reference for the development of new LSMs.
>
> This looks better than V1. One safety comment below.
>
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > include/linux/lsm_hook_defs.h | 4 ++--
> > include/linux/lsm_hooks.h | 18 +++++++++++++++---
> > security/security.c | 13 +++++++------
> > security/selinux/hooks.c | 13 ++++++-------
> > security/smack/smack_lsm.c | 20 +++++++++-----------
> > 5 files changed, 39 insertions(+), 29 deletions(-)
> >
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 477a597db013..afb9dd122f60 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -111,8 +111,8 @@ 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,
> > + void *fs_data)
> > 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 c5498f5174ce..e8c9bac29b9d 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -27,6 +27,7 @@
> >
> > #include <linux/security.h>
> > #include <linux/init.h>
> > +#include <linux/xattr.h>
> > #include <linux/rculist.h>
> >
> > /**
> > @@ -227,9 +228,11 @@
> > * @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.
> > + * @xattrs contains the full array of xattrs allocated by LSMs where
> > + * ->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.
> > + * @fs_data contains filesystem-specific data.
> > * Returns 0 if @name and @value have been successfully set,
> > * -EOPNOTSUPP if no security attribute is needed, or
> > * -ENOMEM on memory allocation failure.
> > @@ -1661,4 +1664,13 @@ static inline void security_delete_hooks(struct
> security_hook_list *hooks,
> >
> > extern int lsm_inode_alloc(struct inode *inode);
> >
>
> Some "security researcher" with a fuzz tester is going to manage to dump junk
> into the slots and ruin your week. I suggest a simple change to make bounds
> checking
> possible. It should never happen, but if that was sufficient people would
> love C
> string processing better.
>
> > +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs)
>
> +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs, int available)
Ok. I looked at how I should do that. Initially, I thought that I could
use a global variable storing the number of inode_init_security
implementations, determined at LSM registration time. Then,
I realized that this would not work, as the number of array elements
when security_old_inode_init_security() is called is 1.
I modified the patch set to pass also the number of array elements.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> > +{
> > + struct xattr *slot;
> > +
> > + for (slot = xattrs; slot && slot->name != NULL; slot++)
>
> + for (slot = xattrs; slot && slot->name != NULL; slot++)
> if (WARN_ON(slot > xattrs[available]))
> return NULL;
>
> > + ;
> > +
> > + return slot;
> > +}
> > #endif /* ! __LINUX_LSM_HOOKS_H */
> > diff --git a/security/security.c b/security/security.c
> > index 7f14e59c4f8e..2c1fe1496069 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1037,18 +1037,16 @@ int security_inode_init_security(struct inode
> *inode, struct inode *dir,
> >
> > if (!initxattrs)
> > return call_int_hook(inode_init_security, -EOPNOTSUPP,
> inode,
> > - dir, qstr, NULL, NULL, NULL);
> > + dir, qstr, NULL, fs_data);
> > 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);
> > + lsm_xattr, fs_data);
> > if (ret)
> > 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, evm_xattr);
> > if (ret)
> > goto out;
> > ret = initxattrs(inode, new_xattrs, fs_data);
> > @@ -1071,10 +1069,13 @@ int security_old_inode_init_security(struct inode
> *inode, struct inode *dir,
> > const struct qstr *qstr, const char **name,
> > void **value, size_t *len)
> > {
> > + struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 };
> > + struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL;
> > +
> > if (unlikely(IS_PRIVATE(inode)))
> > return -EOPNOTSUPP;
> > return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
> > - qstr, name, value, len);
> > + qstr, lsm_xattr, NULL);
> > }
> > EXPORT_SYMBOL(security_old_inode_init_security);
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index ddd097790d47..806827eb132a 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2916,11 +2916,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, void *fs_data)
> > {
> > const struct task_security_struct *tsec = selinux_cred(current_cred());
> > struct superblock_security_struct *sbsec;
> > + struct xattr *xattr = lsm_find_xattr_slot(xattrs);
> > u32 newsid, clen;
> > int rc;
> > char *context;
> > @@ -2947,16 +2947,15 @@ 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 (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;
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index 12a45e61c1a5..af7eee0fee52 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -962,26 +962,24 @@ 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, void *fs_data)
> > {
> > 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 = lsm_find_xattr_slot(xattrs);
> > int may;
> >
> > - if (name)
> > - *name = XATTR_SMACK_SUFFIX;
> > + 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);
> > @@ -999,11 +997,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;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook
2021-04-22 13:46 ` Roberto Sassu
@ 2021-04-22 15:46 ` Casey Schaufler
2021-04-22 16:12 ` Roberto Sassu
0 siblings, 1 reply; 13+ messages in thread
From: Casey Schaufler @ 2021-04-22 15:46 UTC (permalink / raw)
To: Roberto Sassu, zohar, jmorris, paul
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Casey Schaufler
On 4/22/2021 6:46 AM, Roberto Sassu wrote:
>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
>> Sent: Thursday, April 22, 2021 12:44 AM
>> On 4/21/2021 9:19 AM, Roberto Sassu wrote:
>>> In preparation for moving EVM to the LSM infrastructure, this patch
>>> replaces the name, value, len triple with the xattr array pointer provided
>>> by security_inode_init_security(). LSMs are expected to call the new
>>> function lsm_find_xattr_slot() to find the first unused slot of the array
>>> where the xattr should be written.
>>>
>>> This patch modifies also SELinux and Smack to search for an unused slot, to
>>> have a consistent behavior across LSMs (the unmodified version would
>>> overwrite the xattr set by the first LSM in the chain). It is also
>>> desirable to have the modification in those LSMs, as they are likely used
>>> as a reference for the development of new LSMs.
>> This looks better than V1. One safety comment below.
>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>> ---
>>> include/linux/lsm_hook_defs.h | 4 ++--
>>> include/linux/lsm_hooks.h | 18 +++++++++++++++---
>>> security/security.c | 13 +++++++------
>>> security/selinux/hooks.c | 13 ++++++-------
>>> security/smack/smack_lsm.c | 20 +++++++++-----------
>>> 5 files changed, 39 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>>> index 477a597db013..afb9dd122f60 100644
>>> --- a/include/linux/lsm_hook_defs.h
>>> +++ b/include/linux/lsm_hook_defs.h
>>> @@ -111,8 +111,8 @@ 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,
>>> + void *fs_data)
>>> 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 c5498f5174ce..e8c9bac29b9d 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -27,6 +27,7 @@
>>>
>>> #include <linux/security.h>
>>> #include <linux/init.h>
>>> +#include <linux/xattr.h>
>>> #include <linux/rculist.h>
>>>
>>> /**
>>> @@ -227,9 +228,11 @@
>>> * @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.
>>> + * @xattrs contains the full array of xattrs allocated by LSMs where
>>> + * ->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.
>>> + * @fs_data contains filesystem-specific data.
>>> * Returns 0 if @name and @value have been successfully set,
>>> * -EOPNOTSUPP if no security attribute is needed, or
>>> * -ENOMEM on memory allocation failure.
>>> @@ -1661,4 +1664,13 @@ static inline void security_delete_hooks(struct
>> security_hook_list *hooks,
>>> extern int lsm_inode_alloc(struct inode *inode);
>>>
>> Some "security researcher" with a fuzz tester is going to manage to dump junk
>> into the slots and ruin your week. I suggest a simple change to make bounds
>> checking
>> possible. It should never happen, but if that was sufficient people would
>> love C
>> string processing better.
>>
>>> +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs)
>> +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs,
int available)
> Ok. I looked at how I should do that. Initially, I thought that I could
> use a global variable storing the number of inode_init_security
> implementations, determined at LSM registration time. Then,
> I realized that this would not work, as the number of array elements
> when security_old_inode_init_security() is called is 1.
You can address that by expanding the call_int_hook MACRO in
security_old_inode_init_security() in place and change it to stop
after the first call. The two callers of security_old_inode_init_security()
are going to need to be converted to security_inode_init_security()
when the "complete" stacking (i.e. SELinux + Smack) anyway, so I don't
see that as an issue.
Is anyone concerned that ocfs2 and reiserfs aren't EVM capable?
>
> I modified the patch set to pass also the number of array elements.
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli
>
>>> +{
>>> + struct xattr *slot;
>>> +
>>> + for (slot = xattrs; slot && slot->name != NULL; slot++)
>> + for (slot = xattrs; slot && slot->name != NULL; slot++)
>> if (WARN_ON(slot > xattrs[available]))
>> return NULL;
>>
>>> + ;
>>> +
>>> + return slot;
>>> +}
>>> #endif /* ! __LINUX_LSM_HOOKS_H */
>>> diff --git a/security/security.c b/security/security.c
>>> index 7f14e59c4f8e..2c1fe1496069 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -1037,18 +1037,16 @@ int security_inode_init_security(struct inode
>> *inode, struct inode *dir,
>>> if (!initxattrs)
>>> return call_int_hook(inode_init_security, -EOPNOTSUPP,
>> inode,
>>> - dir, qstr, NULL, NULL, NULL);
>>> + dir, qstr, NULL, fs_data);
>>> 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);
>>> + lsm_xattr, fs_data);
>>> if (ret)
>>> 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, evm_xattr);
>>> if (ret)
>>> goto out;
>>> ret = initxattrs(inode, new_xattrs, fs_data);
>>> @@ -1071,10 +1069,13 @@ int security_old_inode_init_security(struct inode
>> *inode, struct inode *dir,
>>> const struct qstr *qstr, const char **name,
>>> void **value, size_t *len)
>>> {
>>> + struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 };
>>> + struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL;
>>> +
>>> if (unlikely(IS_PRIVATE(inode)))
>>> return -EOPNOTSUPP;
>>> return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
>>> - qstr, name, value, len);
>>> + qstr, lsm_xattr, NULL);
>>> }
>>> EXPORT_SYMBOL(security_old_inode_init_security);
>>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index ddd097790d47..806827eb132a 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -2916,11 +2916,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, void *fs_data)
>>> {
>>> const struct task_security_struct *tsec = selinux_cred(current_cred());
>>> struct superblock_security_struct *sbsec;
>>> + struct xattr *xattr = lsm_find_xattr_slot(xattrs);
>>> u32 newsid, clen;
>>> int rc;
>>> char *context;
>>> @@ -2947,16 +2947,15 @@ 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 (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;
>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>> index 12a45e61c1a5..af7eee0fee52 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -962,26 +962,24 @@ 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, void *fs_data)
>>> {
>>> 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 = lsm_find_xattr_slot(xattrs);
>>> int may;
>>>
>>> - if (name)
>>> - *name = XATTR_SMACK_SUFFIX;
>>> + 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);
>>> @@ -999,11 +997,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;
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook
2021-04-22 15:46 ` Casey Schaufler
@ 2021-04-22 16:12 ` Roberto Sassu
2021-04-22 21:39 ` Casey Schaufler
0 siblings, 1 reply; 13+ messages in thread
From: Roberto Sassu @ 2021-04-22 16:12 UTC (permalink / raw)
To: Casey Schaufler, zohar, jmorris, paul
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel
> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Thursday, April 22, 2021 5:46 PM
> On 4/22/2021 6:46 AM, Roberto Sassu wrote:
> >> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> >> Sent: Thursday, April 22, 2021 12:44 AM
> >> On 4/21/2021 9:19 AM, Roberto Sassu wrote:
> >>> In preparation for moving EVM to the LSM infrastructure, this patch
> >>> replaces the name, value, len triple with the xattr array pointer provided
> >>> by security_inode_init_security(). LSMs are expected to call the new
> >>> function lsm_find_xattr_slot() to find the first unused slot of the array
> >>> where the xattr should be written.
> >>>
> >>> This patch modifies also SELinux and Smack to search for an unused slot, to
> >>> have a consistent behavior across LSMs (the unmodified version would
> >>> overwrite the xattr set by the first LSM in the chain). It is also
> >>> desirable to have the modification in those LSMs, as they are likely used
> >>> as a reference for the development of new LSMs.
> >> This looks better than V1. One safety comment below.
> >>
> >>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> >>> ---
> >>> include/linux/lsm_hook_defs.h | 4 ++--
> >>> include/linux/lsm_hooks.h | 18 +++++++++++++++---
> >>> security/security.c | 13 +++++++------
> >>> security/selinux/hooks.c | 13 ++++++-------
> >>> security/smack/smack_lsm.c | 20 +++++++++-----------
> >>> 5 files changed, 39 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/include/linux/lsm_hook_defs.h
> b/include/linux/lsm_hook_defs.h
> >>> index 477a597db013..afb9dd122f60 100644
> >>> --- a/include/linux/lsm_hook_defs.h
> >>> +++ b/include/linux/lsm_hook_defs.h
> >>> @@ -111,8 +111,8 @@ 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,
> >>> + void *fs_data)
> >>> 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 c5498f5174ce..e8c9bac29b9d 100644
> >>> --- a/include/linux/lsm_hooks.h
> >>> +++ b/include/linux/lsm_hooks.h
> >>> @@ -27,6 +27,7 @@
> >>>
> >>> #include <linux/security.h>
> >>> #include <linux/init.h>
> >>> +#include <linux/xattr.h>
> >>> #include <linux/rculist.h>
> >>>
> >>> /**
> >>> @@ -227,9 +228,11 @@
> >>> * @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.
> >>> + * @xattrs contains the full array of xattrs allocated by LSMs where
> >>> + * ->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.
> >>> + * @fs_data contains filesystem-specific data.
> >>> * Returns 0 if @name and @value have been successfully set,
> >>> * -EOPNOTSUPP if no security attribute is needed, or
> >>> * -ENOMEM on memory allocation failure.
> >>> @@ -1661,4 +1664,13 @@ static inline void security_delete_hooks(struct
> >> security_hook_list *hooks,
> >>> extern int lsm_inode_alloc(struct inode *inode);
> >>>
> >> Some "security researcher" with a fuzz tester is going to manage to dump
> junk
> >> into the slots and ruin your week. I suggest a simple change to make bounds
> >> checking
> >> possible. It should never happen, but if that was sufficient people would
> >> love C
> >> string processing better.
> >>
> >>> +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs)
> >> +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs,
> int available)
> > Ok. I looked at how I should do that. Initially, I thought that I could
> > use a global variable storing the number of inode_init_security
> > implementations, determined at LSM registration time. Then,
> > I realized that this would not work, as the number of array elements
> > when security_old_inode_init_security() is called is 1.
>
> You can address that by expanding the call_int_hook MACRO in
> security_old_inode_init_security() in place and change it to stop
> after the first call. The two callers of security_old_inode_init_security()
> are going to need to be converted to security_inode_init_security()
> when the "complete" stacking (i.e. SELinux + Smack) anyway, so I don't
> see that as an issue.
The current version already does it. I was more concerned about LSMs
requesting more than one slot. In this case, lsm_find_xattr_slot()
could return a slot outside the array, unless we pass the correct size.
If we convert ocfs2 and reiserfs to use security_inode_init_security(),
we could use the global variable set at LSM registration time, and we
don't need to add a new parameter.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> Is anyone concerned that ocfs2 and reiserfs aren't EVM capable?
>
> >
> > I modified the patch set to pass also the number of array elements.
> >
> > Roberto
> >
> > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> > Managing Director: Li Peng, Li Jian, Shi Yanli
> >
> >>> +{
> >>> + struct xattr *slot;
> >>> +
> >>> + for (slot = xattrs; slot && slot->name != NULL; slot++)
> >> + for (slot = xattrs; slot && slot->name != NULL; slot++)
> >> if (WARN_ON(slot > xattrs[available]))
> >> return NULL;
> >>
> >>> + ;
> >>> +
> >>> + return slot;
> >>> +}
> >>> #endif /* ! __LINUX_LSM_HOOKS_H */
> >>> diff --git a/security/security.c b/security/security.c
> >>> index 7f14e59c4f8e..2c1fe1496069 100644
> >>> --- a/security/security.c
> >>> +++ b/security/security.c
> >>> @@ -1037,18 +1037,16 @@ int security_inode_init_security(struct inode
> >> *inode, struct inode *dir,
> >>> if (!initxattrs)
> >>> return call_int_hook(inode_init_security, -EOPNOTSUPP,
> >> inode,
> >>> - dir, qstr, NULL, NULL, NULL);
> >>> + dir, qstr, NULL, fs_data);
> >>> 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);
> >>> + lsm_xattr, fs_data);
> >>> if (ret)
> >>> 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, evm_xattr);
> >>> if (ret)
> >>> goto out;
> >>> ret = initxattrs(inode, new_xattrs, fs_data);
> >>> @@ -1071,10 +1069,13 @@ int security_old_inode_init_security(struct
> inode
> >> *inode, struct inode *dir,
> >>> const struct qstr *qstr, const char **name,
> >>> void **value, size_t *len)
> >>> {
> >>> + struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 };
> >>> + struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL;
> >>> +
> >>> if (unlikely(IS_PRIVATE(inode)))
> >>> return -EOPNOTSUPP;
> >>> return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
> >>> - qstr, name, value, len);
> >>> + qstr, lsm_xattr, NULL);
> >>> }
> >>> EXPORT_SYMBOL(security_old_inode_init_security);
> >>>
> >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>> index ddd097790d47..806827eb132a 100644
> >>> --- a/security/selinux/hooks.c
> >>> +++ b/security/selinux/hooks.c
> >>> @@ -2916,11 +2916,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, void *fs_data)
> >>> {
> >>> const struct task_security_struct *tsec = selinux_cred(current_cred());
> >>> struct superblock_security_struct *sbsec;
> >>> + struct xattr *xattr = lsm_find_xattr_slot(xattrs);
> >>> u32 newsid, clen;
> >>> int rc;
> >>> char *context;
> >>> @@ -2947,16 +2947,15 @@ 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 (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;
> >>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> >>> index 12a45e61c1a5..af7eee0fee52 100644
> >>> --- a/security/smack/smack_lsm.c
> >>> +++ b/security/smack/smack_lsm.c
> >>> @@ -962,26 +962,24 @@ 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, void *fs_data)
> >>> {
> >>> 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 = lsm_find_xattr_slot(xattrs);
> >>> int may;
> >>>
> >>> - if (name)
> >>> - *name = XATTR_SMACK_SUFFIX;
> >>> + 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);
> >>> @@ -999,11 +997,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;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook
2021-04-22 16:12 ` Roberto Sassu
@ 2021-04-22 21:39 ` Casey Schaufler
0 siblings, 0 replies; 13+ messages in thread
From: Casey Schaufler @ 2021-04-22 21:39 UTC (permalink / raw)
To: Roberto Sassu, zohar, jmorris, paul
Cc: linux-integrity, linux-security-module, linux-kernel, selinux,
reiserfs-devel, Casey Schaufler
On 4/22/2021 9:12 AM, Roberto Sassu wrote:
>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
>> Sent: Thursday, April 22, 2021 5:46 PM
>> On 4/22/2021 6:46 AM, Roberto Sassu wrote:
>>>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
>>>> Sent: Thursday, April 22, 2021 12:44 AM
>>>> On 4/21/2021 9:19 AM, Roberto Sassu wrote:
>>>>> In preparation for moving EVM to the LSM infrastructure, this patch
>>>>> replaces the name, value, len triple with the xattr array pointer provided
>>>>> by security_inode_init_security(). LSMs are expected to call the new
>>>>> function lsm_find_xattr_slot() to find the first unused slot of the
array
>>>>> where the xattr should be written.
>>>>>
>>>>> This patch modifies also SELinux and Smack to search for an unused slot, to
>>>>> have a consistent behavior across LSMs (the unmodified version would
>>>>> overwrite the xattr set by the first LSM in the chain). It is also
>>>>> desirable to have the modification in those LSMs, as they are likely used
>>>>> as a reference for the development of new LSMs.
>>>> This looks better than V1. One safety comment below.
>>>>
>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>>> ---
>>>>> include/linux/lsm_hook_defs.h | 4 ++--
>>>>> include/linux/lsm_hooks.h | 18 +++++++++++++++---
>>>>> security/security.c | 13 +++++++------
>>>>> security/selinux/hooks.c | 13 ++++++-------
>>>>> security/smack/smack_lsm.c | 20 +++++++++-----------
>>>>> 5 files changed, 39 insertions(+), 29 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/lsm_hook_defs.h
>> b/include/linux/lsm_hook_defs.h
>>>>> index 477a597db013..afb9dd122f60 100644
>>>>> --- a/include/linux/lsm_hook_defs.h
>>>>> +++ b/include/linux/lsm_hook_defs.h
>>>>> @@ -111,8 +111,8 @@ 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,
>>>>> + void *fs_data)
>>>>> 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 c5498f5174ce..e8c9bac29b9d 100644
>>>>> --- a/include/linux/lsm_hooks.h
>>>>> +++ b/include/linux/lsm_hooks.h
>>>>> @@ -27,6 +27,7 @@
>>>>>
>>>>> #include <linux/security.h>
>>>>> #include <linux/init.h>
>>>>> +#include <linux/xattr.h>
>>>>> #include <linux/rculist.h>
>>>>>
>>>>> /**
>>>>> @@ -227,9 +228,11 @@
>>>>> * @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.
>>>>> + * @xattrs contains the full array of xattrs allocated by LSMs where
>>>>> + * ->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.
>>>>> + * @fs_data contains filesystem-specific data.
>>>>> * Returns 0 if @name and @value have been successfully set,
>>>>> * -EOPNOTSUPP if no security attribute is needed, or
>>>>> * -ENOMEM on memory allocation failure.
>>>>> @@ -1661,4 +1664,13 @@ static inline void security_delete_hooks(struct
>>>> security_hook_list *hooks,
>>>>> extern int lsm_inode_alloc(struct inode *inode);
>>>>>
>>>> Some "security researcher" with a fuzz tester is going to manage to dump
>> junk
>>>> into the slots and ruin your week. I suggest a simple change to make
bounds
>>>> checking
>>>> possible. It should never happen, but if that was sufficient people would
>>>> love C
>>>> string processing better.
>>>>
>>>>> +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs)
>>>> +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs,
>> int available)
>>> Ok. I looked at how I should do that. Initially, I thought that I could
>>> use a global variable storing the number of inode_init_security
>>> implementations, determined at LSM registration time. Then,
>>> I realized that this would not work, as the number of array elements
>>> when security_old_inode_init_security() is called is 1.
>> You can address that by expanding the call_int_hook MACRO in
>> security_old_inode_init_security() in place and change it to stop
>> after the first call. The two callers of security_old_inode_init_security()
>> are going to need to be converted to security_inode_init_security()
>> when the "complete" stacking (i.e. SELinux + Smack) anyway, so I don't
>> see that as an issue.
> The current version already does it. I was more concerned about LSMs
> requesting more than one slot. In this case, lsm_find_xattr_slot()
> could return a slot outside the array, unless we pass the correct size.
That would be a bit of a problem. It would be possible to add the number
of inode_init "slots" to struct lsm_info or to struct lsm_blob_sizes.
That would require dynamic allocation, but you've been advocating
that anyway. Ding! If we add the number of slots required to lsm_blob_sizes
we can replace that with the slot number to use in the inode_init_security
array. This is the same technique used for blob offsets in the LSM managed
blobs. The EVM hook will know the size of the array. We pass the base of
array to the module inode_init_security hooks, and they know what slot(s)
they've been told to use. The EVM hook starts with base and any slot that's
filled get processed.
Unfortunately, security_old_inode_init_security() will have to allocate
the array and copy results from the right slot into the parameters passed.
Essentially a complete rewrite.
>
> If we convert ocfs2 and reiserfs to use security_inode_init_security(),
> we could use the global variable set at LSM registration time, and we
> don't need to add a new parameter.
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli
>
>> Is anyone concerned that ocfs2 and reiserfs aren't EVM capable?
>>
>>> I modified the patch set to pass also the number of array elements.
>>>
>>> Roberto
>>>
>>> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
>>> Managing Director: Li Peng, Li Jian, Shi Yanli
>>>
>>>>> +{
>>>>> + struct xattr *slot;
>>>>> +
>>>>> + for (slot = xattrs; slot && slot->name != NULL; slot++)
>>>> + for (slot = xattrs; slot && slot->name != NULL; slot++)
>>>> if (WARN_ON(slot > xattrs[available]))
>>>> return NULL;
>>>>
>>>>> + ;
>>>>> +
>>>>> + return slot;
>>>>> +}
>>>>> #endif /* ! __LINUX_LSM_HOOKS_H */
>>>>> diff --git a/security/security.c b/security/security.c
>>>>> index 7f14e59c4f8e..2c1fe1496069 100644
>>>>> --- a/security/security.c
>>>>> +++ b/security/security.c
>>>>> @@ -1037,18 +1037,16 @@ int security_inode_init_security(struct inode
>>>> *inode, struct inode *dir,
>>>>> if (!initxattrs)
>>>>> return call_int_hook(inode_init_security, -EOPNOTSUPP,
>>>> inode,
>>>>> - dir, qstr, NULL, NULL, NULL);
>>>>> + dir, qstr, NULL, fs_data);
>>>>> 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);
>>>>> + lsm_xattr, fs_data);
>>>>> if (ret)
>>>>> 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, evm_xattr);
>>>>> if (ret)
>>>>> goto out;
>>>>> ret = initxattrs(inode, new_xattrs, fs_data);
>>>>> @@ -1071,10 +1069,13 @@ int security_old_inode_init_security(struct
>> inode
>>>> *inode, struct inode *dir,
>>>>> const struct qstr *qstr, const char **name,
>>>>> void **value, size_t *len)
>>>>> {
>>>>> + struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 };
>>>>> + struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL;
>>>>> +
>>>>> if (unlikely(IS_PRIVATE(inode)))
>>>>> return -EOPNOTSUPP;
>>>>> return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
>>>>> - qstr, name, value, len);
>>>>> + qstr, lsm_xattr, NULL);
>>>>> }
>>>>> EXPORT_SYMBOL(security_old_inode_init_security);
>>>>>
>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>> index ddd097790d47..806827eb132a 100644
>>>>> --- a/security/selinux/hooks.c
>>>>> +++ b/security/selinux/hooks.c
>>>>> @@ -2916,11 +2916,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, void *fs_data)
>>>>> {
>>>>> const struct task_security_struct *tsec = selinux_cred(current_cred());
>>>>> struct superblock_security_struct *sbsec;
>>>>> + struct xattr *xattr = lsm_find_xattr_slot(xattrs);
>>>>> u32 newsid, clen;
>>>>> int rc;
>>>>> char *context;
>>>>> @@ -2947,16 +2947,15 @@ 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 (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;
>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>>> index 12a45e61c1a5..af7eee0fee52 100644
>>>>> --- a/security/smack/smack_lsm.c
>>>>> +++ b/security/smack/smack_lsm.c
>>>>> @@ -962,26 +962,24 @@ 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, void *fs_data)
>>>>> {
>>>>> 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 = lsm_find_xattr_slot(xattrs);
>>>>> int may;
>>>>>
>>>>> - if (name)
>>>>> - *name = XATTR_SMACK_SUFFIX;
>>>>> + 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);
>>>>> @@ -999,11 +997,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;
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-04-22 21:39 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-21 16:19 [PATCH v2 0/6] evm: Prepare for moving to the LSM infrastructure Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 1/6] xattr: Complete constify ->name member of "struct xattr" Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 2/6] reiserfs: Add missing calls to reiserfs_security_free() Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook Roberto Sassu
2021-04-21 22:43 ` Casey Schaufler
2021-04-22 13:46 ` Roberto Sassu
2021-04-22 15:46 ` Casey Schaufler
2021-04-22 16:12 ` Roberto Sassu
2021-04-22 21:39 ` Casey Schaufler
2021-04-21 16:19 ` [PATCH v2 4/6] security: Support multiple LSMs implementing " Roberto Sassu
2021-04-21 23:09 ` Casey Schaufler
2021-04-21 16:19 ` [PATCH v2 5/6] evm: Align evm_inode_init_security() definition with LSM infrastructure Roberto Sassu
2021-04-21 16:19 ` [PATCH v2 6/6] evm: Support multiple LSMs providing an xattr Roberto Sassu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).