Re: [PATCH] evm: check return values of crypto_shash functions

On 8/7/2026 7:42 PM, Roberto Sassu wrote:
> On Tue, 2026-08-04 at 16:07 -0400, Mimi Zohar wrote:
>> From: Daniel Hodges <[email protected]>
>>
>> The crypto_shash_update() and crypto_shash_final() functions can fail
>> and return error codes, but their return values were not being checked
>> in several places in security/integrity/evm/evm_crypto.c:
>>
>> - hmac_add_misc() ignored returns from crypto_shash_update() and
>>    crypto_shash_final()
>> - evm_calc_hmac_or_hash() ignored returns from crypto_shash_update()
>> - evm_init_hmac() ignored returns from crypto_shash_update()
>>
>> If these hash operations fail silently, the resulting HMAC could be
>> invalid or incomplete, which could weaken the integrity verification
>> security that EVM provides.
>>
>> This patch converts hmac_add_misc() from void to int return type and
>> adds proper error checking and propagation for all crypto_shash_*
>> function calls. All callers are updated to handle the new return values.
>> Additionally, error messages are logged when cryptographic operations
>> fail to provide visibility into the failure rather than silently
>> returning error codes.
>>
>> Fixes: 66dbc325afce ("evm: re-release")
>> Signed-off-by: Daniel Hodges <[email protected]>
>> Reviewed-by: Roberto Sassu <[email protected]>
> 
> Looks good to me. I confirm the Reviewed-by.

We got a report from Sashiko. As it pointed out, we should not override 
the error when calling hmac_add_misc() if no xattr is present
(-ENODATA).

Roberto

> Thanks
> 
> Roberto
> 
>> Link: https://github.com/linux-integrity/linux/issues/17
>> [[email protected]: fixed commit 0496fc9cdc38 merge conflict,
>> checkpatch.pl warnings]
>> Signed-off-by: Mimi Zohar <[email protected]>
>> ---
>> Daniel, Roberto, please review this version of the patch.
>>
>>   security/integrity/evm/evm_crypto.c | 67 ++++++++++++++++++++++-------
>>   1 file changed, 51 insertions(+), 16 deletions(-)
>>
>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>> index 1c41af2f91a6..aef0f8f3d9d6 100644
>> --- a/security/integrity/evm/evm_crypto.c
>> +++ b/security/integrity/evm/evm_crypto.c
>> @@ -140,8 +140,8 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
>>    * (Additional directory/file metadata needs to be added for more complete
>>    * protection.)
>>    */
>> -static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
>> -			  char type, char *digest)
>> +static int hmac_add_misc(struct shash_desc *desc, struct inode *inode,
>> +			 char type, char *digest)
>>   {
>>   	struct h_misc {
>>   		/*
>> @@ -156,6 +156,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
>>   		gid_t gid;
>>   		umode_t mode;
>>   	} hmac_misc;
>> +	int error;
>>
>>   	memset(&hmac_misc, 0, sizeof(hmac_misc));
>>   	/* Don't include the inode or generation number in portable
>> @@ -176,14 +177,30 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
>>   	hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
>>   	hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
>>   	hmac_misc.mode = inode->i_mode;
>> -	crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
>> +	error = crypto_shash_update(desc, (const u8 *)&hmac_misc,
>> +				    sizeof(hmac_misc));
>> +	if (error) {
>> +		pr_err("crypto_shash_update() failed: %d\n", error);
>> +		return error;
>> +	}
>>   	if ((evm_hmac_attrs & EVM_ATTR_FSUUID) &&
>> -	    type != EVM_XATTR_PORTABLE_DIGSIG)
>> -		crypto_shash_update(desc, (u8 *)&inode->i_sb->s_uuid, UUID_SIZE);
>> -	crypto_shash_final(desc, digest);
>> +	    type != EVM_XATTR_PORTABLE_DIGSIG) {
>> +		error = crypto_shash_update(desc, (u8 *)&inode->i_sb->s_uuid,
>> +					    UUID_SIZE);
>> +		if (error) {
>> +			pr_err("crypto_shash_update() failed: %d\n", error);
>> +			return error;
>> +		}
>> +	}
>> +	error = crypto_shash_final(desc, digest);
>> +	if (error) {
>> +		pr_err("crypto_shash_final() failed: %d\n", error);
>> +		return error;
>> +	}
>>
>>   	pr_debug("hmac_misc: (%zu) [%*phN]\n", sizeof(struct h_misc),
>>   		 (int)sizeof(struct h_misc), &hmac_misc);
>> +	return 0;
>>   }
>>
>>   /*
>> @@ -267,9 +284,14 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>>
>>   		if ((req_xattr_name && req_xattr_value)
>>   		    && !strcmp(xattr->name, req_xattr_name)) {
>> -			error = 0;
>> -			crypto_shash_update(desc, (const u8 *)req_xattr_value,
>> -					     req_xattr_value_len);
>> +			error = crypto_shash_update(desc,
>> +						    (const u8 *)req_xattr_value,
>> +						    req_xattr_value_len);
>> +			if (error) {
>> +				pr_err("crypto_shash_update() failed: %d\n",
>> +				       error);
>> +				goto out;
>> +			}
>>   			if (is_ima)
>>   				ima_present = true;
>>
>> @@ -293,15 +315,21 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>>   			pr_debug("file %s: xattr %s size mismatch (kernel: %d, user: %d)\n",
>>   				 dentry->d_name.name, xattr->name, size,
>>   				 user_space_size);
>> -		error = 0;
>>   		xattr_size = size;
>> -		crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
>> +		error = crypto_shash_update(desc, (const u8 *)xattr_value,
>> +					    xattr_size);
>> +		if (error) {
>> +			pr_err("crypto_shash_update() failed: %d\n", error);
>> +			goto out;
>> +		}
>>   		if (is_ima)
>>   			ima_present = true;
>>
>>   		dump_security_xattr(xattr->name, xattr_value, xattr_size);
>>   	}
>> -	hmac_add_misc(desc, inode, type, data->digest);
>> +	error = hmac_add_misc(desc, inode, type, data->digest);
>> +	if (error)
>> +		goto out;
>>
>>   	if (inode != d_backing_inode(dentry) && iint) {
>>   		if (IS_I_VERSION(inode))
>> @@ -409,6 +437,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *xattrs,
>>   	struct shash_desc *desc;
>>   	const struct xattr *xattr;
>>   	struct xattr_list *xattr_entry;
>> +	int error;
>>
>>   	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
>>   	if (IS_ERR(desc)) {
>> @@ -423,14 +452,20 @@ int evm_init_hmac(struct inode *inode, const struct xattr *xattrs,
>>   				   XATTR_SECURITY_PREFIX_LEN, xattr->name) != 0)
>>   				continue;
>>
>> -			crypto_shash_update(desc, xattr->value,
>> -					    xattr->value_len);
>> +			error = crypto_shash_update(desc, xattr->value,
>> +						    xattr->value_len);
>> +			if (error) {
>> +				pr_err("crypto_shash_update() failed: %d\n",
>> +				       error);
>> +				goto out;
>> +			}
>>   		}
>>   	}
>>
>> -	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
>> +	error = hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
>> +out:
>>   	kfree(desc);
>> -	return 0;
>> +	return error;
>>   }
>>
>>   /*
>> --
>> 2.55.0
>>
>