On 8/8/2019 2:39 PM, Kees Cook wrote:

On Wed, Aug 07, 2019 at 12:43:57PM -0700, Casey Schaufler wrote:

quoted

@@ -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".

quoted

+	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;

...

quoted

+		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.

quoted

+		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.

quoted

+		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 <redacted>