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@xxxxxxxxxx>
> ---
> 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
>
