On Mon, 2021-05-03 at 07:55 +0000, Roberto Sassu wrote:

quoted

quoted

diff --git a/security/integrity/evm/evm_main.c

b/security/integrity/evm/evm_main.c

quoted

quoted

@@ -354,6 +372,8 @@ static int evm_protect_xattr(struct dentry *dentry,

const char *xattr_name,

quoted

 				    -EPERM, 0);
 	}
 out:
+	if (evm_ignore_error_safe(evm_status))
+		return 0;

I agree with the concept, but the function name doesn't provide enough
context.  Perhaps defining a function more along the lines of
"evm_hmac_disabled()" would be more appropriate and at the same time
self documenting.

Since the function checks if the passed error can be ignored,
would evm_ignore_error_hmac_disabled() also be ok?

The purpose of evm_protect_xattr() is to prevent allowing an invalid
security.evm xattr from being re-calculated and updated, making it
valid.   Refer to the first line of the function description.  That
hasn't changed.

One of the reasons for defining a new function is to avoid code
duplication, but it should not come at the expense of clear and easily
understood code.   In this case, the reason for "ignoring" certain
return codes needs to be highlighted, not hidden.  
(is_)evm_hmac_disabled() makes this very clear.

Please update the function description to include the reason why making
an exception is safe.

thanks,

Mimi

quoted

quoted

 	if (evm_status != INTEGRITY_PASS)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,

d_backing_inode(dentry),

quoted

 				    dentry->d_name.name,

"appraise_metadata",