Re: [PATCH v7 15/28] LSM: Specify which LSM to display

[Casey Schaufler <casey@schaufler-ca.com>]

On 8/8/2019 2:39 PM, Kees Cook wrote:
> On Wed, Aug 07, 2019 at 12:43:57PM -0700, Casey Schaufler wrote:
>> @@ -1980,10 +2033,48 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>>  			 size_t size)
>>  {
>>  	struct security_hook_list *hp;
>> +	char *term;
>> +	char *cp;
>> +	int *display = current->security;
> So I went down a rat hole looking at setprocattr vs current. It looks
> like everything ignores the $pid part of /proc/$pid/attr/$name and only
> ever operates on "current". Is that the expected interface here?

Yes. procfs enforces the policy that writes can only affect "self".

>
>> +	int rc = -EINVAL;
>> +	int slot = 0;
>> +
>> +	if (!strcmp(name, "display")) {
>> +		if (!capable(CAP_MAC_ADMIN))
>> +			return -EPERM;
>> +		/*
>> +		 * lsm_slot will be 0 if there are no displaying modules.
>> +		 */
>> +		if (lsm_slot == 0 || size == 0)
>> +			return -EINVAL;
> ...
>
>> +		cp = kzalloc(size + 1, GFP_KERNEL);
>> +		if (cp == NULL)
>> +			return -ENOMEM;
>> +		memcpy(cp, value, size);
> Saving one line, the above can be:
>
> 		cp = kmemdup_nul(value, size, GFP_KERNEL);
> 		if (cp == NULL)
> 			return -ENOMEM;

Thanks. That would be better.

>
>> +		term = strchr(cp, ' ');
>> +		if (term == NULL)
>> +			term = strchr(cp, '\n');
> "foo\n " will result in "foo\n". I think you want strsep() instead of
> the above three lines:
>
> 		term = strsep(cp, " \n");

That would be better.

>
>> +		if (term != NULL)
>> +			*term = '\0';
>> +
>> +		for (slot = 0; slot < lsm_slot; slot++)
>> +			if (!strcmp(cp, lsm_slotlist[slot]->lsm)) {
>> +				*display = lsm_slotlist[slot]->slot;
>> +				rc = size;
>> +				break;
>> +			}
>> +
>> +		kfree(cp);
>> +		return rc;
>> +	}
>>  
>>  	hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>>  		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>>  			continue;
>> +		if (lsm == NULL && *display != LSMBLOB_INVALID &&
>> +		    *display != hp->lsmid->slot)
>> +			continue;
>>  		return hp->hook.setprocattr(name, value, size);
>>  	}
>>  	return -EINVAL;
> Otherwise, yeah, seems good.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>