Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752948AbbEKUdo (ORCPT ); Mon, 11 May 2015 16:33:44 -0400 Received: from smtp106.biz.mail.bf1.yahoo.com ([98.139.244.54]:36688 "EHLO smtp106.biz.mail.bf1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751813AbbEKUdk (ORCPT ); Mon, 11 May 2015 16:33:40 -0400 X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: x4C4TmUVM1mHZPPcBdpku3bWQo17A8L975VhtGB1wVoGqnH BxtSGEaq4JpO69cYTu750IeI7DDA6aH5cagmnkRrWDKMw2fUlvJaDOUSm9dp .yyO7ppBDbnzO1ds.ecOOsz9oQugc4qhsRJiNnT6I7mvqykYI0D9SEIQHOxR 9niuP9raTywxxdn0rJqO88zxEC1uVQmMmcZntWm4afo9V5YmMFopijDtYOIE 2OcFa755PxFEe.hribm4t1z10AQLP_iBUXZfSTurZevx7JsfbjJFO7rggc42 FmZA.qFdCiLHcLEt5Sy.ZaqTQvOrLi17XFO4g7lIiWItUoqDi.OPs1hWkowG 0Fe3NZmgbaFJV2Dr2zEyag96tsD3VKG7vd3YImrm5wsrNqqVbHEzb0Ga8Jfv 9tISgs3uZqsCZY52MvWvrizcnDu6qtsiI0ZUuMZwyqMxASsrsGivl1iDuK7H oRMh0b_WvoAkMWrpVQvlebhbtxSgpFbuFKyV3oYcOYDpdm_CX7Skk1pY_7wD KO1liHfHGbI68PUtgcv1jnouuDF7Zprg_WA-- X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Message-ID: <55511222.7010300@schaufler-ca.com> Date: Mon, 11 May 2015 13:33:38 -0700 From: Casey Schaufler User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Lukasz Pawelczyk , James Morris , "Serge E. Hallyn" , linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org CC: Lukasz Pawelczyk , Casey Schaufler Subject: Re: [PATCH] smack: pass error code through pointers References: <1429542774-21110-1-git-send-email-l.pawelczyk@samsung.com> In-Reply-To: <1429542774-21110-1-git-send-email-l.pawelczyk@samsung.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 18232 Lines: 570 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 Acked-by: Casey Schaufler Applied to git@github.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/