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@xxxxxxxxxxxx>
>
