EVM: turn evm_config_xattrnames into a list

Signed-off-by Matthew Garrett <[email protected]>

---

Updated to add support for locking down the list, and also to forbid adding the same xattr twice.

 Documentation/ABI/testing/evm       |  13 +++
 security/integrity/evm/Kconfig      |  11 ++
 security/integrity/evm/evm_crypto.c |   2 +-
 security/integrity/evm/evm_main.c   |   6 +-
 security/integrity/evm/evm_secfs.c  | 153 ++++++++++++++++++++++++++--
 5 files changed, 175 insertions(+), 10 deletions(-)

security/integrity/evm/evm_secfs.c +19 -0

+/**
+ * evm_write_xattrs - write() for <securityfs>/evm_xattrs
+ * @file: file pointer, not actually used
+ * @buf: where to get the data from
+ * @count: bytes sent
+ * @ppos: where to start
+ *
+ * Returns number of bytes written or error code, as appropriate
+ */
+static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	int len, err;
+	struct xattr_list *xattr, *tmp;
+
+	if (!capable(CAP_SYS_ADMIN) || evm_xattrs_locked)
+		return -EPERM;
+
+	if (*ppos != 0)

security/integrity/evm/evm_secfs.c +4 -0

+	if (IS_ERR(xattr->name)) {
+		err = PTR_ERR(xattr->name);
+		kfree(xattr);
+		return err;

security/integrity/evm/evm_secfs.c +21 -0

+		goto out;
+	}
+
+	/* Guard against races in evm_read_xattrs */
+	mutex_lock(&xattr_list_mutex);
+	list_for_each_entry(tmp, &evm_config_xattrnames, list) {
+		if (strcmp(xattr->name, tmp->name) == 0) {
+			err = -EEXIST;
+			mutex_unlock(&xattr_list_mutex);
+			goto out;
+		}
+	}
+	list_add_tail_rcu(&xattr->list, &evm_config_xattrnames);
+	mutex_unlock(&xattr_list_mutex);
+
+	return count;
+out:
+	kfree(xattr->name);
+	kfree(xattr);
+	return err;
+}

Quoted 1 line · Matthew Garrett

+ xattr->name = memdup_user_nul(buf, count);

Up to now, the set of protected EVM xattrs was in the security domain. The current code permits any string in any domain. If that is the intention, there needs to be an explanation of the security implications of this change at least in the patch description.

security/integrity/evm/evm_secfs.c static const struct file_operations evm_key_ops = { · line 203

Quoted 3 lines

security/integrity/evm/evm_secfs.c +3 -0

+     if (strcmp(xattr->name, ".") == 0) {
+             evm_xattrs_locked = 1;
+             err = count;

Please update the file mode bits of <securityfs>/evm_xattrs.

Should "evm_xattrs" be defined directly in the securityfs directory or in a subdirectory similar to ima? It will be difficult later on to move "evm_xattrs" to a subdirectory once applications start reading/writing to it. What would the subdirectory be called?

Quoted 4 lines

Should "evm_xattrs" be defined directly in the securityfs directory or in a subdirectory similar to ima? It will be difficult later on to move "evm_xattrs" to a subdirectory once applications start reading/writing to it. What would the subdirectory be called?

Yeah, that's tricky - the obvious directory would be evm, but that's already in use. integrity makes sense, but then ima should also be under it.

security/integrity/evm/evm_secfs.c +54 -0

+/**
+ * evm_write_xattrs - write() for <securityfs>/evm_xattrs
+ * @file: file pointer, not actually used
+ * @buf: where to get the data from
+ * @count: bytes sent
+ * @ppos: where to start
+ *
+ * Returns number of bytes written or error code, as appropriate
+ */
+static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	int len, err;
+	struct xattr_list *xattr, *tmp;
+
+	if (!capable(CAP_SYS_ADMIN) || evm_xattrs_locked)
+		return -EPERM;
+
+	if (*ppos != 0)
+		return -EINVAL;
+
+	if (count > XATTR_NAME_MAX)
+		return -E2BIG;
+
+	xattr = kmalloc(sizeof(struct xattr_list), GFP_KERNEL);
+	if (!xattr)
+		return -ENOMEM;
+
+	xattr->name = memdup_user_nul(buf, count);
+	if (IS_ERR(xattr->name)) {
+		err = PTR_ERR(xattr->name);
+		kfree(xattr);
+		return err;
+	}
+
+	/* Remove any trailing newline */
+	len = strlen(xattr->name);
+	if (xattr->name[len-1] == '\n')
+		xattr->name[len-1] = '\0';
+
+	if (strcmp(xattr->name, ".") == 0) {
+		evm_xattrs_locked = 1;
+		err = count;
+		goto out;
+	}
+
+	/* Guard against races in evm_read_xattrs */
+	mutex_lock(&xattr_list_mutex);
+	list_for_each_entry(tmp, &evm_config_xattrnames, list) {
+		if (strcmp(xattr->name, tmp->name) == 0) {
+			err = -EEXIST;
+			mutex_unlock(&xattr_list_mutex);
+			goto out;
+		}

Use a list of xattrs rather than an array - this makes it easier to extend the list at runtime.

Signed-off-by Matthew Garrett <[email protected]>

---
 security/integrity/evm/evm.h        |  7 ++-
 security/integrity/evm/evm_crypto.c | 10 ++--
 security/integrity/evm/evm_main.c   | 72 +++++++++++++++++++----------
 3 files changed, 59 insertions(+), 30 deletions(-)

security/integrity/evm/evm_main.c +0 -1

@@ -565,10 +588,11 @@ static int __init init_evm(void)
  */
 static int __init evm_display_config(void)
 {
-	char **xattrname;