On 4/6/2012 7:35 PM, Tetsuo Handa wrote: > Casey Schaufler wrote: >> + if (len <= 0) >> + len = strlen(string) + 1; > Is this check correct? > If len > strlen(string), you might access string[strlen(string)] in below loop. string[strlen(string)] is the terminating '\0'. That is detected by the second test (string[i] <= ' ') in the loop. The loop is looking for the first character that is not valid in a label, not the last this is valid. >> + >> + /* >> + * Reserve a leading '-' as an indicator that >> + * this isn't a label, but an option to interfaces >> + * including /smack/cipso and /smack/cipso2 >> + */ >> + if (string[0] == '-') >> + return NULL; >> + >> + for (i = 0; i < len; i++) >> + if (string[i] > '~' || string[i] <= ' ' || string[i] == '/' || >> + string[i] == '"' || string[i] == '\\' || string[i] == '\'') >> + break; >> + >> + if (i == 0 || i >= SMK_LONGLABEL) >> + return NULL; > > >> if (skp == NULL) { >> - skp = kzalloc(sizeof(struct smack_known), GFP_KERNEL); >> + skp = kzalloc(sizeof(*skp), GFP_KERNEL); >> if (skp != NULL) { >> - strncpy(skp->smk_known, smack, SMK_MAXLEN); >> + memset(skp, 0, sizeof(*skp)); > kzalloc() will do memset(). Thanks for catching that. I'll take it out. >> @@ -1913,10 +1860,8 @@ static int smack_netlabel(struct sock *sk, int labeled) >> labeled == SMACK_UNLABELED_SOCKET) >> netlbl_sock_delattr(sk); >> else { >> - netlbl_secattr_init(&secattr); >> - smack_to_secattr(ssp->smk_out, &secattr); >> - rc = netlbl_sock_setattr(sk, sk->sk_family, &secattr); >> - netlbl_secattr_destroy(&secattr); >> + skp = smk_find_entry(ssp->smk_out); > What guarantees that skp != NULL? ssp->smk_out is a label that is on the label list. Since it's already on the list there is no chance that it won't be found. >> + rc = netlbl_sock_setattr(sk, sk->sk_family, &skp->smk_netlabel); > >> + rcu_read_lock(); >> + list_for_each_entry(kp, &smack_known_list, list) { >> + if (sap->attr.mls.lvl != kp->smk_netlabel.attr.mls.lvl) >> + continue; >> + if (memcmp(sap->attr.mls.cat, >> + kp->smk_netlabel.attr.mls.cat, >> + SMK_CIPSOLEN) != 0) >> + continue; >> + found = 1; >> + break; >> } >> - /* >> - * Look it up in the supplied table if it is not >> - * a direct mapping. >> - */ >> - sp = smack_from_cipso(sap->attr.mls.lvl, smack); >> - if (sp != NULL) >> - return sp; > What rcu_read_lock() was protecting? If elements in smack_known_list, it could > be removed before "return kp->smk_known;". Entries are never removed from smack_known_list, so there is no issue. >> + rcu_read_unlock(); >> + >> + if (found) >> + return kp->smk_known; >> + > > >> + hsp = smack_host_label(&addr); >> + rcu_read_unlock(); >> + >> + if (hsp == NULL) { >> + skp = smk_find_entry(sp); > What guarantees that skp != NULL? Again, sp is a label that is known, hence is on the list, hence can be found. >> + rc = netlbl_req_setattr(req, &skp->smk_netlabel); >> + } else >> netlbl_req_delattr(req); >> - } >> > > >> +static inline void smack_catset_bit(int cat, char *catsetp) >> +{ >> + if (cat > (SMK_CIPSOLEN * 8)) > Don't you want to also check that "|| cat < 1"? Good point. I'll add appropriate checks. >> + return; >> + >> + catsetp[(cat - 1) / 8] |= 0x80 >> ((cat - 1) % 8); >> +} >> + > > >> + data = kzalloc(count + 1, GFP_KERNEL); >> + if (data == NULL) >> + return -ENOMEM; >> + if (copy_from_user(data, buf, count) != 0) > kfree(data) is needed. Indeed. >> return -EFAULT; > > >> + data = kzalloc(count, GFP_KERNEL); >> + if (data == NULL) >> + return -ENOMEM; >> >> - if (copy_from_user(in, buf, count) != 0) >> + if (copy_from_user(data, buf, count) != 0) > kfree(data) is needed. Indeed. >> return -EFAULT; >> > > >> +static ssize_t smk_user_access(struct file *file, const char __user *buf, >> + size_t count, loff_t *ppos, int format) > (...snipped...) >> + } else { >> + /* >> + * Copy the data to make sure the string is terminated. >> + */ >> + cod = kzalloc(count + 1, GFP_KERNEL); > cod != NULL check is needed. If data is a string, kstrdup() would be better. You're right about the NULL check. kstrdup would be great, but part of what this code is doing is making sure that the data is a string. >> + memcpy(cod, data, count); >> + cod[count] = '\0'; >> + res = smk_parse_long_rule(cod, &rule, 0); >> + kfree(cod); >> + } >> Thanks for the review. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html