Re: [PATCH V4 2/2] EVM: Allow runtime modification of the set of verified xattrs

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Wed, 2018-05-09 at 13:28 -0700, Matthew Garrett wrote:

> +/**
> + * 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);

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. 

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

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

> +		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;
> +}
> +
> +static const struct file_operations evm_xattr_ops = {
> +	.read		= evm_read_xattrs,
> +	.write		= evm_write_xattrs,
> +};
> +
> +static int evm_init_xattrs(void)
> +{
> +	evm_xattrs = securityfs_create_file("evm_xattrs", 0440, NULL, NULL,
> +					    &evm_xattr_ops);

The mode bits should reflect the status of the current status of
evm_xattrs.  Initially it would be writeable, but later it might
change to read-only.

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?

Mimi