linux-kernel - Re: [PATCH -next] evm: Use __vfs_setxattr() to update security.evm

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3c34c1e8c74722110e5d7e87146b090791734916.camel@linux.ibm.com>
Date:   Wed, 18 Jan 2023 16:45:02 -0500
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Xiu Jianfeng <xiujianfeng@...wei.com>, dmitry.kasatkin@...il.com,
        paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com
Cc:     linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next] evm: Use __vfs_setxattr() to update security.evm

On Wed, 2022-12-28 at 11:02 +0800, Xiu Jianfeng wrote:
> Currently it uses __vfs_setxattr_noperm() to update "security.evm",
> however there are two lsm hooks(inode_post_setxattr and inode_setsecurity)
> being called inside this function, which don't make any sense for xattr
> "security.evm", because the handlers of these two hooks, such as selinux
> and smack, only care about their own xattr.

Updating the security.ima hash triggers re-calculating and writing the
security.evm HMAC.  Refer to evm_inode_post_setxattr().

Mimi

> 
> On the other hand, there is a literally rather than actually cyclical
> callchain as follows:
> security_inode_post_setxattr
>   ->evm_inode_post_setxattr
>     ->evm_update_evmxattr
>       ->__vfs_setxattr_noperm
>         ->security_inode_post_setxattr
> 
> So use __vfs_setxattr() to update "security.evm".
> 
> Signed-off-by: Xiu Jianfeng <xiujianfeng@...wei.com>
> ---
>  security/integrity/evm/evm_crypto.c   | 7 +++----
>  security/integrity/ima/ima_appraise.c | 8 ++++----
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index fa5ff13fa8c9..d8275dfa49ef 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -376,10 +376,9 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>  			   xattr_value_len, &data);
>  	if (rc == 0) {
>  		data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
> -		rc = __vfs_setxattr_noperm(&init_user_ns, dentry,
> -					   XATTR_NAME_EVM,
> -					   &data.hdr.xattr.data[1],
> -					   SHA1_DIGEST_SIZE + 1, 0);
> +		rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
> +				    XATTR_NAME_EVM, &data.hdr.xattr.data[1],
> +				    SHA1_DIGEST_SIZE + 1, 0);
>  	} else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
>  		rc = __vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_EVM);
>  	}
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ee6f7e237f2e..d2de9dc6c345 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -98,10 +98,10 @@ static int ima_fix_xattr(struct dentry *dentry,
>  		iint->ima_hash->xattr.ng.type = IMA_XATTR_DIGEST_NG;
>  		iint->ima_hash->xattr.ng.algo = algo;
>  	}
> -	rc = __vfs_setxattr_noperm(&init_user_ns, dentry, XATTR_NAME_IMA,
> -				   &iint->ima_hash->xattr.data[offset],
> -				   (sizeof(iint->ima_hash->xattr) - offset) +
> -				   iint->ima_hash->length, 0);
> +	rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
> +			    XATTR_NAME_IMA, &iint->ima_hash->xattr.data[offset],
> +			    (sizeof(iint->ima_hash->xattr) - offset) +
> +			    iint->ima_hash->length, 0);
>  	return rc;
>  }
>