| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d1d63c522a3842ccaf74c4462ee06bf82ce3b3f5.camel@huaweicloud.com>
Date: Wed, 04 Feb 2026 13:50:29 +0100
From: Roberto Sassu <roberto.sassu@...weicloud.com>
To: Daniel Hodges <hodgesd@...a.com>, zohar@...ux.ibm.com,
roberto.sassu@...wei.com
Cc: dmitry.kasatkin@...il.com, eric.snowberg@...cle.com,
paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] evm: check return values of crypto_shash functions
On Sat, 2026-01-31 at 10:22 -0800, Daniel Hodges wrote:
> The crypto_shash_update() and crypto_shash_final() functions can fail
> and return error codes, but their return values were being ignored in
> several places in evm_crypto.c:
>
> - hmac_add_misc(): ignores returns from crypto_shash_update() and
> crypto_shash_final()
> - evm_calc_hmac_or_hash(): ignores returns from crypto_shash_update()
> - evm_init_hmac(): ignores returns from crypto_shash_update()
>
> If these hash operations fail silently, the resulting HMAC could be
> invalid or incomplete. This could potentially allow integrity
> verification to pass with incorrect HMACs, weakening EVM's security
> guarantees.
>
> Fix this by:
> - Changing hmac_add_misc() from void to int return type
> - Checking and propagating error codes from all crypto_shash calls
> - Updating all callers to check the return values
>
> Fixes: 66dbc325afce ("evm: re-release")
> Signed-off-by: Daniel Hodges <hodgesd@...a.com>
> ---
> security/integrity/evm/evm_crypto.c | 45 +++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index a5e730ffda57..286f23a1a26b 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -132,58 +132,65 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
> }
> return desc;
> }
>
> /* Protect against 'cutting & pasting' security.evm xattr, include inode
> * specific info.
> *
> * (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 {
> unsigned long ino;
> __u32 generation;
> uid_t uid;
> gid_t gid;
> umode_t mode;
> } hmac_misc;
> + int ret;
>
> memset(&hmac_misc, 0, sizeof(hmac_misc));
> /* Don't include the inode or generation number in portable
> * signatures
> */
> if (type != EVM_XATTR_PORTABLE_DIGSIG) {
> hmac_misc.ino = inode->i_ino;
> hmac_misc.generation = inode->i_generation;
> }
> /* The hmac uid and gid must be encoded in the initial user
> * namespace (not the filesystems user namespace) as encoding
> * them in the filesystems user namespace allows an attack
> * where first they are written in an unprivileged fuse mount
> * of a filesystem and then the system is tricked to mount the
> * filesystem for real on next boot and trust it because
> * everything is signed.
> */
> 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));
> + ret = crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
> + if (ret)
> + return ret;
> 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) {
> + ret = crypto_shash_update(desc, (u8 *)&inode->i_sb->s_uuid, UUID_SIZE);
> + if (ret)
> + return ret;
> + }
> + ret = crypto_shash_final(desc, digest);
Maybe we should also indicate if an error occurred, with a separate
error message, or adding the result in the message below.
Thanks
Roberto
> pr_debug("hmac_misc: (%zu) [%*phN]\n", sizeof(struct h_misc),
> (int)sizeof(struct h_misc), &hmac_misc);
> + return ret;
> }
>
> /*
> * Dump large security xattr values as a continuous ascii hexadecimal string.
> * (pr_debug is limited to 64 bytes.)
> */
> static void dump_security_xattr_l(const char *prefix, const void *src,
> size_t count)
> {
> #if defined(DEBUG) || defined(CONFIG_DYNAMIC_DEBUG)
> @@ -253,23 +260,24 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>
> /*
> * Skip non-enabled xattrs for locally calculated
> * signatures/HMACs.
> */
> if (type != EVM_XATTR_PORTABLE_DIGSIG && !xattr->enabled)
> continue;
>
> 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)
> + goto out;
> if (is_ima)
> ima_present = true;
>
> dump_security_xattr(req_xattr_name,
> req_xattr_value,
> req_xattr_value_len);
> continue;
> }
> size = vfs_getxattr_alloc(&nop_mnt_idmap, dentry, xattr->name,
> &xattr_value, xattr_size, GFP_NOFS);
> @@ -279,29 +287,32 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> }
> if (size < 0)
> continue;
>
> user_space_size = vfs_getxattr(&nop_mnt_idmap, dentry,
> xattr->name, NULL, 0);
> if (user_space_size != size)
> 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)
> + 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))
> i_version = inode_query_iversion(inode);
> integrity_inode_attrs_store(&iint->metadata_inode, i_version,
> inode);
> }
>
> /* Portable EVM signatures must include an IMA hash */
> if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present)
> @@ -394,37 +405,41 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
> rc = __vfs_removexattr(&nop_mnt_idmap, dentry, XATTR_NAME_EVM);
> }
> return rc;
> }
>
> int evm_init_hmac(struct inode *inode, const struct xattr *xattrs,
> char *hmac_val)
> {
> struct shash_desc *desc;
> const struct xattr *xattr;
> + int ret;
>
> desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
> if (IS_ERR(desc)) {
> pr_info("init_desc failed\n");
> return PTR_ERR(desc);
> }
>
> for (xattr = xattrs; xattr->name; xattr++) {
> if (!evm_protected_xattr(xattr->name))
> continue;
>
> - crypto_shash_update(desc, xattr->value, xattr->value_len);
> + ret = crypto_shash_update(desc, xattr->value, xattr->value_len);
> + if (ret)
> + goto out;
> }
>
> - hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
> + ret = hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
> +out:
> kfree(desc);
> - return 0;
> + return ret;
> }
>
> /*
> * Get the key from the TPM for the SHA1-HMAC
> */
> int evm_init_key(void)
> {
> struct key *evm_key;
> struct encrypted_key_payload *ekp;
> int rc;
Powered by blists - more mailing lists