* [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
* 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 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
* [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
* 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
* [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