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?
> + 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;
> + 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");
> + 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>
--
Kees Cook
