RE: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook

From: Roberto Sassu
Date: Thu Apr 22 2021 - 12:12:13 EST

Next message: Darrick J. Wong: "Re: [Virtio-fs] [PATCH v3 2/3] dax: Add a wakeup mode parameter to put_unlocked_entry()"
Previous message: Atul Gopinathan: "[PATCH] media: gspca: stv06xx: Fix memleak in stv06xx subdrivers"
In reply to: Casey Schaufler: "Re: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook"
Next in thread: Casey Schaufler: "Re: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> From: Casey Schaufler [mailto:casey@xxxxxxxxxxxxxxxx]
> Sent: Thursday, April 22, 2021 5:46 PM
> On 4/22/2021 6:46 AM, Roberto Sassu wrote:
> >> From: Casey Schaufler [mailto:casey@xxxxxxxxxxxxxxxx]
> >> 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@xxxxxxxxxx>
> >>> ---
> >>> 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;

Next message: Darrick J. Wong: "Re: [Virtio-fs] [PATCH v3 2/3] dax: Add a wakeup mode parameter to put_unlocked_entry()"
Previous message: Atul Gopinathan: "[PATCH] media: gspca: stv06xx: Fix memleak in stv06xx subdrivers"
In reply to: Casey Schaufler: "Re: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook"
Next in thread: Casey Schaufler: "Re: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the inode_init_security hook"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]