Re: [PATCH] smack: pass error code through pointers
From: Casey Schaufler
Date: Mon May 11 2015 - 16:33:48 EST
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@xxxxxxxxxxx>
Acked-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
Applied to git@xxxxxxxxxx: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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/