Re: [PATCH] Smack: allow for significantly longer Smack labels

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]

  Powered by Linux