Re: [PATCH v2 09/12] evm: Allow setxattr() and setattr() if metadata digest won't change

[Mimi Zohar]

Hi Roberto,

"if" doesn't belong in the subject line.  In this case, instead of "if
metadata ...", how about something like "for unmodified metadata"?

On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> With the patch to allow xattr/attr operations if a portable signature
> verification fails, cp and tar can copy all xattrs/attrs so that at the
> end of the process verification succeeds.
> 
> However, it might happen that xattrs/attrs are already set to the correct

^ the xattrs/attrs

> value (taken at signing time) and signature verification succeeds before
> the copy is completed. For example, an archive might contains files owned

^ has completed.

> by root and the archive is extracted by root.
> 
> Then, since portable signatures are immutable, all subsequent operations
> fail (e.g. fchown()), even if the operation is legitimate (does not alter
> the current value).
> 
> This patch avoids this problem by reporting successful operation to user
> space when that operation does not alter the current value of xattrs/attrs.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>

<snip>

> +/*
> + * evm_xattr_change - check if passed xattr value differs from current value
> + * @dentry: pointer to the affected dentry
> + * @xattr_name: requested xattr
> + * @xattr_value: requested xattr value
> + * @xattr_value_len: requested xattr value length
> + *
> + * Check if passed xattr value differs from current value.
> + *
> + * Returns 1 if passed xattr value differs from current value, 0 otherwise.
> + */
> +static int evm_xattr_change(struct dentry *dentry, const char *xattr_name,
> +			    const void *xattr_value, size_t xattr_value_len)
> +{
> +	char *xattr_data = NULL;
> +	int rc = 0;
> +
> +	if (posix_xattr_acl(xattr_name))
> +		return evm_xattr_acl_change(dentry, xattr_name, xattr_value,
> +					    xattr_value_len);
> +
> +	rc = vfs_getxattr_alloc(dentry, xattr_name, &xattr_data, 0, GFP_NOFS);
> +	if (rc < 0)
> +		return 1;
> +
> +	if (rc == xattr_value_len)
> +		rc = memcmp(xattr_value, xattr_data, rc);

This should probably be changed to crypto_memneq().   Refer to commit
613317bd212c ("EVM: Use crypto_memneq() for digest comparisons").

thanks,

Mimi

> +	else
> +		rc = 1;
> +
> +	kfree(xattr_data);
> +	return rc;
> +}
> +
>