Message-id: <1429542774-21110-1-git-send-email-l.pawelczyk@samsung.com>
Date:	Mon, 20 Apr 2015 17:12:54 +0200
From:	Lukasz Pawelczyk <l.pawelczyk@...sung.com>
To:	Casey Schaufler <casey@...aufler-ca.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>,
	Lukasz Pawelczyk <l.pawelczyk@...sung.com>
Subject: [PATCH] smack: pass error code through pointers

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>
---
 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);
-- 
2.1.0

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