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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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.

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;





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux