Message-ID: <55511222.7010300@schaufler-ca.com>
Date:	Mon, 11 May 2015 13:33:38 -0700
From:	Casey Schaufler <casey@...aufler-ca.com>
To:	Lukasz Pawelczyk <l.pawelczyk@...sung.com>,
	James Morris <james.l.morris@...cle.com>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
CC:	Lukasz Pawelczyk <havner@...il.com>,
	Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH] smack: pass error code through pointers

On 4/20/2015 8:12 AM, Lukasz Pawelczyk wrote:
> This patch makes the following functions to use ERR_PTR() and related
> macros to pass the appropriate error code through returned pointers:
>
> smk_parse_smack()
> smk_import_entry()
> smk_fetch()
>
> It also makes all the other functions that use them to handle the
> error cases properly. This ways correct error codes from places
> where they happened can be propagated to the user space if necessary.
>
> Doing this it fixes a bug in onlycap and unconfined files
> handling. Previously their content was cleared on any error from
> smk_import_entry/smk_parse_smack, be it EINVAL (as originally intended)
> or ENOMEM. Right now it only reacts on EINVAL passing other codes
> properly to userspace.
>
> Comments have been updated accordingly.
>
> Signed-off-by: Lukasz Pawelczyk <l.pawelczyk@...sung.com>

Acked-by: Casey Schaufler <casey@...aufler-ca.com>
Applied to git@...hub.com:cschaufler/smack-next.git smack-for-4.2

> ---
>  security/smack/smack_access.c |  27 ++++++----
>  security/smack/smack_lsm.c    |  93 +++++++++++++++++++--------------
>  security/smack/smackfs.c      | 116 +++++++++++++++++++++++++-----------------
>  3 files changed, 139 insertions(+), 97 deletions(-)
>
> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
> index 0f410fc..408e20b 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -425,7 +425,7 @@ void smk_insert_entry(struct smack_known *skp)
>   * @string: a text string that might be a Smack label
>   *
>   * Returns a pointer to the entry in the label list that
> - * matches the passed string.
> + * matches the passed string or NULL if not found.
>   */
>  struct smack_known *smk_find_entry(const char *string)
>  {
> @@ -448,7 +448,7 @@ struct smack_known *smk_find_entry(const char *string)
>   * @string: a text string that might contain a Smack label
>   * @len: the maximum size, or zero if it is NULL terminated.
>   *
> - * Returns a pointer to the clean label, or NULL
> + * Returns a pointer to the clean label or an error code.
>   */
>  char *smk_parse_smack(const char *string, int len)
>  {
> @@ -464,7 +464,7 @@ char *smk_parse_smack(const char *string, int len)
>  	 * including /smack/cipso and /smack/cipso2
>  	 */
>  	if (string[0] == '-')
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  
>  	for (i = 0; i < len; i++)
>  		if (string[i] > '~' || string[i] <= ' ' || string[i] == '/' ||
> @@ -472,11 +472,13 @@ char *smk_parse_smack(const char *string, int len)
>  			break;
>  
>  	if (i == 0 || i >= SMK_LONGLABEL)
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  
>  	smack = kzalloc(i + 1, GFP_KERNEL);
> -	if (smack != NULL)
> -		strncpy(smack, string, i);
> +	if (smack == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	strncpy(smack, string, i);
>  
>  	return smack;
>  }
> @@ -523,7 +525,8 @@ int smk_netlbl_mls(int level, char *catset, struct netlbl_lsm_secattr *sap,
>   * @len: the maximum size, or zero if it is NULL terminated.
>   *
>   * Returns a pointer to the entry in the label list that
> - * matches the passed string, adding it if necessary.
> + * matches the passed string, adding it if necessary,
> + * or an error code.
>   */
>  struct smack_known *smk_import_entry(const char *string, int len)
>  {
> @@ -533,8 +536,8 @@ struct smack_known *smk_import_entry(const char *string, int len)
>  	int rc;
>  
>  	smack = smk_parse_smack(string, len);
> -	if (smack == NULL)
> -		return NULL;
> +	if (IS_ERR(smack))
> +		return ERR_CAST(smack);
>  
>  	mutex_lock(&smack_known_lock);
>  
> @@ -543,8 +546,10 @@ struct smack_known *smk_import_entry(const char *string, int len)
>  		goto freeout;
>  
>  	skp = kzalloc(sizeof(*skp), GFP_KERNEL);
> -	if (skp == NULL)
> +	if (skp == NULL) {
> +		skp = ERR_PTR(-ENOMEM);
>  		goto freeout;
> +	}
>  
>  	skp->smk_known = smack;
>  	skp->smk_secid = smack_next_secid++;
> @@ -577,7 +582,7 @@ struct smack_known *smk_import_entry(const char *string, int len)
>  	 * smk_netlbl_mls failed.
>  	 */
>  	kfree(skp);
> -	skp = NULL;
> +	skp = ERR_PTR(rc);
>  freeout:
>  	kfree(smack);
>  unlockout:
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 6f3c7d8..bb9b917 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -245,8 +245,8 @@ static int smk_bu_credfile(const struct cred *cred, struct file *file,
>   * @ip: a pointer to the inode
>   * @dp: a pointer to the dentry
>   *
> - * Returns a pointer to the master list entry for the Smack label
> - * or NULL if there was no label to fetch.
> + * Returns a pointer to the master list entry for the Smack label,
> + * NULL if there was no label to fetch, or an error code.
>   */
>  static struct smack_known *smk_fetch(const char *name, struct inode *ip,
>  					struct dentry *dp)
> @@ -256,14 +256,18 @@ static struct smack_known *smk_fetch(const char *name, struct inode *ip,
>  	struct smack_known *skp = NULL;
>  
>  	if (ip->i_op->getxattr == NULL)
> -		return NULL;
> +		return ERR_PTR(-EOPNOTSUPP);
>  
>  	buffer = kzalloc(SMK_LONGLABEL, GFP_KERNEL);
>  	if (buffer == NULL)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	rc = ip->i_op->getxattr(dp, name, buffer, SMK_LONGLABEL);
> -	if (rc > 0)
> +	if (rc < 0)
> +		skp = ERR_PTR(rc);
> +	else if (rc == 0)
> +		skp = NULL;
> +	else
>  		skp = smk_import_entry(buffer, rc);
>  
>  	kfree(buffer);
> @@ -615,40 +619,44 @@ static int smack_sb_kern_mount(struct super_block *sb, int flags, void *data)
>  		if (strncmp(op, SMK_FSHAT, strlen(SMK_FSHAT)) == 0) {
>  			op += strlen(SMK_FSHAT);
>  			skp = smk_import_entry(op, 0);
> -			if (skp != NULL) {
> -				sp->smk_hat = skp;
> -				specified = 1;
> -			}
> +			if (IS_ERR(skp))
> +				return PTR_ERR(skp);
> +			sp->smk_hat = skp;
> +			specified = 1;
> +
>  		} else if (strncmp(op, SMK_FSFLOOR, strlen(SMK_FSFLOOR)) == 0) {
>  			op += strlen(SMK_FSFLOOR);
>  			skp = smk_import_entry(op, 0);
> -			if (skp != NULL) {
> -				sp->smk_floor = skp;
> -				specified = 1;
> -			}
> +			if (IS_ERR(skp))
> +				return PTR_ERR(skp);
> +			sp->smk_floor = skp;
> +			specified = 1;
> +
>  		} else if (strncmp(op, SMK_FSDEFAULT,
>  				   strlen(SMK_FSDEFAULT)) == 0) {
>  			op += strlen(SMK_FSDEFAULT);
>  			skp = smk_import_entry(op, 0);
> -			if (skp != NULL) {
> -				sp->smk_default = skp;
> -				specified = 1;
> -			}
> +			if (IS_ERR(skp))
> +				return PTR_ERR(skp);
> +			sp->smk_default = skp;
> +			specified = 1;
> +
>  		} else if (strncmp(op, SMK_FSROOT, strlen(SMK_FSROOT)) == 0) {
>  			op += strlen(SMK_FSROOT);
>  			skp = smk_import_entry(op, 0);
> -			if (skp != NULL) {
> -				sp->smk_root = skp;
> -				specified = 1;
> -			}
> +			if (IS_ERR(skp))
> +				return PTR_ERR(skp);
> +			sp->smk_root = skp;
> +			specified = 1;
> +
>  		} else if (strncmp(op, SMK_FSTRANS, strlen(SMK_FSTRANS)) == 0) {
>  			op += strlen(SMK_FSTRANS);
>  			skp = smk_import_entry(op, 0);
> -			if (skp != NULL) {
> -				sp->smk_root = skp;
> -				transmute = 1;
> -				specified = 1;
> -			}
> +			if (IS_ERR(skp))
> +				return PTR_ERR(skp);
> +			sp->smk_root = skp;
> +			transmute = 1;
> +			specified = 1;
>  		}
>  	}
>  
> @@ -1136,7 +1144,9 @@ static int smack_inode_setxattr(struct dentry *dentry, const char *name,
>  
>  	if (rc == 0 && check_import) {
>  		skp = size ? smk_import_entry(value, size) : NULL;
> -		if (skp == NULL || (check_star &&
> +		if (IS_ERR(skp))
> +			rc = PTR_ERR(skp);
> +		else if (skp == NULL || (check_star &&
>  		    (skp == &smack_known_star || skp == &smack_known_web)))
>  			rc = -EINVAL;
>  	}
> @@ -1176,19 +1186,19 @@ static void smack_inode_post_setxattr(struct dentry *dentry, const char *name,
>  
>  	if (strcmp(name, XATTR_NAME_SMACK) == 0) {
>  		skp = smk_import_entry(value, size);
> -		if (skp != NULL)
> +		if (!IS_ERR(skp))
>  			isp->smk_inode = skp;
>  		else
>  			isp->smk_inode = &smack_known_invalid;
>  	} else if (strcmp(name, XATTR_NAME_SMACKEXEC) == 0) {
>  		skp = smk_import_entry(value, size);
> -		if (skp != NULL)
> +		if (!IS_ERR(skp))
>  			isp->smk_task = skp;
>  		else
>  			isp->smk_task = &smack_known_invalid;
>  	} else if (strcmp(name, XATTR_NAME_SMACKMMAP) == 0) {
>  		skp = smk_import_entry(value, size);
> -		if (skp != NULL)
> +		if (!IS_ERR(skp))
>  			isp->smk_mmap = skp;
>  		else
>  			isp->smk_mmap = &smack_known_invalid;
> @@ -2433,8 +2443,8 @@ static int smack_inode_setsecurity(struct inode *inode, const char *name,
>  		return -EINVAL;
>  
>  	skp = smk_import_entry(value, size);
> -	if (skp == NULL)
> -		return -EINVAL;
> +	if (IS_ERR(skp))
> +		return PTR_ERR(skp);
>  
>  	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
>  		nsp->smk_inode = skp;
> @@ -3207,7 +3217,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>  		 */
>  		dp = dget(opt_dentry);
>  		skp = smk_fetch(XATTR_NAME_SMACK, inode, dp);
> -		if (skp != NULL)
> +		if (!IS_ERR_OR_NULL(skp))
>  			final = skp;
>  
>  		/*
> @@ -3244,11 +3254,14 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>  		 * Don't let the exec or mmap label be "*" or "@".
>  		 */
>  		skp = smk_fetch(XATTR_NAME_SMACKEXEC, inode, dp);
> -		if (skp == &smack_known_star || skp == &smack_known_web)
> +		if (IS_ERR(skp) || skp == &smack_known_star ||
> +		    skp == &smack_known_web)
>  			skp = NULL;
>  		isp->smk_task = skp;
> +
>  		skp = smk_fetch(XATTR_NAME_SMACKMMAP, inode, dp);
> -		if (skp == &smack_known_star || skp == &smack_known_web)
> +		if (IS_ERR(skp) || skp == &smack_known_star ||
> +		    skp == &smack_known_web)
>  			skp = NULL;
>  		isp->smk_mmap = skp;
>  
> @@ -3332,8 +3345,8 @@ static int smack_setprocattr(struct task_struct *p, char *name,
>  		return -EINVAL;
>  
>  	skp = smk_import_entry(value, size);
> -	if (skp == NULL)
> -		return -EINVAL;
> +	if (IS_ERR(skp))
> +		return PTR_ERR(skp);
>  
>  	/*
>  	 * No process is ever allowed the web ("@") label.
> @@ -4108,8 +4121,10 @@ static int smack_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
>  		return -EINVAL;
>  
>  	skp = smk_import_entry(rulestr, 0);
> -	if (skp)
> -		*rule = skp->smk_known;
> +	if (IS_ERR(skp))
> +		return PTR_ERR(skp);
> +
> +	*rule = skp->smk_known;
>  
>  	return 0;
>  }
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 06f719e..bf183f9 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -338,8 +338,7 @@ static int smk_perm_from_str(const char *string)
>   * @import: if non-zero, import labels
>   * @len: label length limit
>   *
> - * Returns 0 on success, -EINVAL on failure and -ENOENT when either subject
> - * or object is missing.
> + * Returns 0 on success, appropriate error code on failure.
>   */
>  static int smk_fill_rule(const char *subject, const char *object,
>  				const char *access1, const char *access2,
> @@ -351,16 +350,16 @@ static int smk_fill_rule(const char *subject, const char *object,
>  
>  	if (import) {
>  		rule->smk_subject = smk_import_entry(subject, len);
> -		if (rule->smk_subject == NULL)
> -			return -EINVAL;
> +		if (IS_ERR(rule->smk_subject))
> +			return PTR_ERR(rule->smk_subject);
>  
>  		rule->smk_object = smk_import_entry(object, len);
> -		if (rule->smk_object == NULL)
> -			return -EINVAL;
> +		if (IS_ERR(rule->smk_object))
> +			return PTR_ERR(rule->smk_object);
>  	} else {
>  		cp = smk_parse_smack(subject, len);
> -		if (cp == NULL)
> -			return -EINVAL;
> +		if (IS_ERR(cp))
> +			return PTR_ERR(cp);
>  		skp = smk_find_entry(cp);
>  		kfree(cp);
>  		if (skp == NULL)
> @@ -368,8 +367,8 @@ static int smk_fill_rule(const char *subject, const char *object,
>  		rule->smk_subject = skp;
>  
>  		cp = smk_parse_smack(object, len);
> -		if (cp == NULL)
> -			return -EINVAL;
> +		if (IS_ERR(cp))
> +			return PTR_ERR(cp);
>  		skp = smk_find_entry(cp);
>  		kfree(cp);
>  		if (skp == NULL)
> @@ -412,7 +411,7 @@ static int smk_parse_rule(const char *data, struct smack_parsed_rule *rule,
>   * @import: if non-zero, import labels
>   * @tokens: numer of substrings expected in data
>   *
> - * Returns number of processed bytes on success, -1 on failure.
> + * Returns number of processed bytes on success, -ERRNO on failure.
>   */
>  static ssize_t smk_parse_long_rule(char *data, struct smack_parsed_rule *rule,
>  				int import, int tokens)
> @@ -431,7 +430,7 @@ static ssize_t smk_parse_long_rule(char *data, struct smack_parsed_rule *rule,
>  
>  		if (data[cnt] == '\0')
>  			/* Unexpected end of data */
> -			return -1;
> +			return -EINVAL;
>  
>  		tok[i] = data + cnt;
>  
> @@ -529,14 +528,14 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>  	while (cnt < count) {
>  		if (format == SMK_FIXED24_FMT) {
>  			rc = smk_parse_rule(data, &rule, 1);
> -			if (rc != 0) {
> -				rc = -EINVAL;
> +			if (rc < 0)
>  				goto out;
> -			}
>  			cnt = count;
>  		} else {
>  			rc = smk_parse_long_rule(data + cnt, &rule, 1, tokens);
> -			if (rc <= 0) {
> +			if (rc < 0)
> +				goto out;
> +			if (rc == 0) {
>  				rc = -EINVAL;
>  				goto out;
>  			}
> @@ -915,8 +914,10 @@ static ssize_t smk_set_cipso(struct file *file, const char __user *buf,
>  	mutex_lock(&smack_cipso_lock);
>  
>  	skp = smk_import_entry(rule, 0);
> -	if (skp == NULL)
> +	if (IS_ERR(skp)) {
> +		rc = PTR_ERR(skp);
>  		goto out;
> +	}
>  
>  	if (format == SMK_FIXED24_FMT)
>  		rule += SMK_LABELLEN;
> @@ -1237,8 +1238,8 @@ static ssize_t smk_write_netlbladdr(struct file *file, const char __user *buf,
>  	 */
>  	if (smack[0] != '-') {
>  		skp = smk_import_entry(smack, 0);
> -		if (skp == NULL) {
> -			rc = -EINVAL;
> +		if (IS_ERR(skp)) {
> +			rc = PTR_ERR(skp);
>  			goto free_out;
>  		}
>  	} else {
> @@ -1619,8 +1620,8 @@ static ssize_t smk_write_ambient(struct file *file, const char __user *buf,
>  	}
>  
>  	skp = smk_import_entry(data, count);
> -	if (skp == NULL) {
> -		rc = -EINVAL;
> +	if (IS_ERR(skp)) {
> +		rc = PTR_ERR(skp);
>  		goto out;
>  	}
>  
> @@ -1704,21 +1705,31 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
>  	if (data == NULL)
>  		return -ENOMEM;
>  
> +	if (copy_from_user(data, buf, count) != 0) {
> +		rc = -EFAULT;
> +		goto freeout;
> +	}
> +
>  	/*
> -	 * Should the null string be passed in unset the onlycap value.
> -	 * This seems like something to be careful with as usually
> -	 * smk_import only expects to return NULL for errors. It
> -	 * is usually the case that a nullstring or "\n" would be
> -	 * bad to pass to smk_import but in fact this is useful here.
> +	 * Clear the smack_onlycap on invalid label errors. This means
> +	 * that we can pass a null string to unset the onlycap value.
>  	 *
> -	 * smk_import will also reject a label beginning with '-',
> +	 * Importing will also reject a label beginning with '-',
>  	 * so "-usecapabilities" will also work.
> +	 *
> +	 * But do so only on invalid label, not on system errors.
>  	 */
> -	if (copy_from_user(data, buf, count) != 0)
> -		rc = -EFAULT;
> -	else
> -		smack_onlycap = smk_import_entry(data, count);
> +	skp = smk_import_entry(data, count);
> +	if (PTR_ERR(skp) == -EINVAL)
> +		skp = NULL;
> +	else if (IS_ERR(skp)) {
> +		rc = PTR_ERR(skp);
> +		goto freeout;
> +	}
> +
> +	smack_onlycap = skp;
>  
> +freeout:
>  	kfree(data);
>  	return rc;
>  }
> @@ -1773,6 +1784,7 @@ static ssize_t smk_write_unconfined(struct file *file, const char __user *buf,
>  					size_t count, loff_t *ppos)
>  {
>  	char *data;
> +	struct smack_known *skp;
>  	int rc = count;
>  
>  	if (!smack_privileged(CAP_MAC_ADMIN))
> @@ -1782,21 +1794,31 @@ static ssize_t smk_write_unconfined(struct file *file, const char __user *buf,
>  	if (data == NULL)
>  		return -ENOMEM;
>  
> +	if (copy_from_user(data, buf, count) != 0) {
> +		rc = -EFAULT;
> +		goto freeout;
> +	}
> +
>  	/*
> -	 * Should the null string be passed in unset the unconfined value.
> -	 * This seems like something to be careful with as usually
> -	 * smk_import only expects to return NULL for errors. It
> -	 * is usually the case that a nullstring or "\n" would be
> -	 * bad to pass to smk_import but in fact this is useful here.
> +	 * Clear the smack_unconfined on invalid label errors. This means
> +	 * that we can pass a null string to unset the unconfined value.
>  	 *
> -	 * smk_import will also reject a label beginning with '-',
> +	 * Importing will also reject a label beginning with '-',
>  	 * so "-confine" will also work.
> +	 *
> +	 * But do so only on invalid label, not on system errors.
>  	 */
> -	if (copy_from_user(data, buf, count) != 0)
> -		rc = -EFAULT;
> -	else
> -		smack_unconfined = smk_import_entry(data, count);
> +	skp = smk_import_entry(data, count);
> +	if (PTR_ERR(skp) == -EINVAL)
> +		skp = NULL;
> +	else if (IS_ERR(skp)) {
> +		rc = PTR_ERR(skp);
> +		goto freeout;
> +	}
> +
> +	smack_unconfined = skp;
>  
> +freeout:
>  	kfree(data);
>  	return rc;
>  }
> @@ -1980,7 +2002,7 @@ static ssize_t smk_user_access(struct file *file, const char __user *buf,
>  		res = smk_access(rule.smk_subject, rule.smk_object,
>  				 rule.smk_access1, NULL);
>  	else if (res != -ENOENT)
> -		return -EINVAL;
> +		return res;
>  
>  	/*
>  	 * smk_access() can return a value > 0 in the "bringup" case.
> @@ -2209,8 +2231,8 @@ static ssize_t smk_write_revoke_subj(struct file *file, const char __user *buf,
>  	}
>  
>  	cp = smk_parse_smack(data, count);
> -	if (cp == NULL) {
> -		rc = -EINVAL;
> +	if (IS_ERR(cp)) {
> +		rc = PTR_ERR(cp);
>  		goto free_out;
>  	}
>  
> @@ -2341,10 +2363,10 @@ static ssize_t smk_write_syslog(struct file *file, const char __user *buf,
>  		rc = -EFAULT;
>  	else {
>  		skp = smk_import_entry(data, count);
> -		if (skp == NULL)
> -			rc = -EINVAL;
> +		if (IS_ERR(skp))
> +			rc = PTR_ERR(skp);
>  		else
> -			smack_syslog_label = smk_import_entry(data, count);
> +			smack_syslog_label = skp;
>  	}
>  
>  	kfree(data);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists