linux-hardening - Re: [RESEND] evm: Refector struct 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: <20220518051326.GB16144@embeddedor>
Date:   Wed, 18 May 2022 00:13:26 -0500
From:   "Gustavo A. R. Silva" <gustavoars@...nel.org>
To:     GUO Zihua <guozihua@...wei.com>
Cc:     linux-integrity@...r.kernel.org, zohar@...ux.ibm.com,
        dmitry.kasatkin@...il.com, linux-hardening@...r.kernel.org
Subject: Re: [RESEND] evm: Refector struct evm_xattr

On Wed, May 18, 2022 at 09:14:39AM +0800, GUO Zihua wrote:
> struct evm_xattr is only used for EVM_XATTR_HMAC type evm digest and is
> glued together one flexible array and one fixed length array. The
> original intention seems to be shortening the code as the "data" field
> would always be a SHA1 digest.
> 
> This implementation is not complying with GCC's specification about
> flexible array and spars yield the following warning:

The sparse warning has nothing to do with any GCC specification. It's
perfectly fine to apply the sizeof operator to a struct-with-flex-array.
However, it might be suspicious if the intention is to also get the
_dynamic_ size of the flexible array, because in that case the size of
the flex array is always zero. See the example below:

	struct foo {
		uint8_t len;
		struct boo data[];
	};

	sizeof(struct foo) == 1

Also, you sent this patch twice in the last 24 hours. Give the maintainers
time to review your patch (usually a couple of weeks) before resending it.

> 
> security/integrity/evm/evm_main.c:852:30: warning: using sizeof on a flexible
> structure
> security/integrity/evm/evm_main.c:862:32: warning: using sizeof on a flexible
> structure

Regarding the warnings above, please take a look at my response to your
other patch (the same applies in this case). :)

Thanks
--
Gustavo

> 
> Fix it by:
> 1. Remove struct evm_xattr and use struct evm_ima_xattr_data directly.
> 2. Get array size with struct_size instead of sizeof.
> 
> Reference: https://github.com/KSPP/linux/issues/174
> 
> Fixes: 6be5cc5246f80 ("evm: add support for different security.evm data types")
> Signed-off-by: GUO Zihua <guozihua@...wei.com>
> ---
>  security/integrity/evm/evm_main.c | 14 ++++++++------
>  security/integrity/integrity.h    |  6 ------
>  2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 7d87772f0ce6..f2c4501a287a 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -211,7 +211,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  	/* check value type */
>  	switch (xattr_data->type) {
>  	case EVM_XATTR_HMAC:
> -		if (xattr_len != sizeof(struct evm_xattr)) {
> +		if (xattr_len != struct_size(*xattr_data, data,
> +					     SHA1_DIGEST_SIZE)) {
>  			evm_status = INTEGRITY_FAIL;
>  			goto out;
>  		}
> @@ -842,24 +843,25 @@ int evm_inode_init_security(struct inode *inode,
>  				 const struct xattr *lsm_xattr,
>  				 struct xattr *evm_xattr)
>  {
> -	struct evm_xattr *xattr_data;
> +	struct evm_ima_xattr_data *xattr_data;
>  	int rc;
>  
>  	if (!(evm_initialized & EVM_INIT_HMAC) ||
>  	    !evm_protected_xattr(lsm_xattr->name))
>  		return 0;
>  
> -	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> +	xattr_data = kzalloc(struct_size(*xattr_data, data,
> +			     SHA1_DIGEST_SIZE), GFP_NOFS);
>  	if (!xattr_data)
>  		return -ENOMEM;
>  
> -	xattr_data->data.type = EVM_XATTR_HMAC;
> -	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> +	xattr_data->type = EVM_XATTR_HMAC;
> +	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->data);
>  	if (rc < 0)
>  		goto out;
>  
>  	evm_xattr->value = xattr_data;
> -	evm_xattr->value_len = sizeof(*xattr_data);
> +	evm_xattr->value_len = struct_size(*xattr_data, data, SHA1_DIGEST_SIZE);
>  	evm_xattr->name = XATTR_EVM_SUFFIX;
>  	return 0;
>  out:
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 3510e413ea17..91b16d620dd9 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -86,12 +86,6 @@ struct evm_ima_xattr_data {
>  	u8 data[];
>  } __packed;
>  
> -/* Only used in the EVM HMAC code. */
> -struct evm_xattr {
> -	struct evm_ima_xattr_data data;
> -	u8 digest[SHA1_DIGEST_SIZE];
> -} __packed;
> -
>  #define IMA_MAX_DIGEST_SIZE	64
>  
>  struct ima_digest_data {
> -- 
> 2.36.0
>