On Fri, 2018-11-16 at 18:07 -0200, Thiago Jung Bauermann wrote:
> Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
> SHA1 digest, most of the code ignores the array and uses the struct to mean
> "type indicator followed by data of unspecified size" and tracks the real
> size of what the struct represents in a separate length variable.
>
> The only exception to that is the EVM code, which correctly uses the
> definition of struct evm_ima_xattr_data.
>
> So make this explicit in the code by removing the length specification from
> the array in struct evm_ima_xattr_data. Also, change the name of the
> element from digest to data since in most places the array doesn't hold a
> digest.
>
> A separate struct evm_xattr is introduced, with the original definition of
> evm_ima_xattr_data to be used in the places that actually expect that
> definition.
, specifically the EVM HMAC code.
>
> Signed-off-by: Thiago Jung Bauermann <bauerman@xxxxxxxxxxxxx>
Other than commenting the evm_xattr usage is limited to HMAC before
the structure definition, this looks good.
Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
> ---
> security/integrity/evm/evm_main.c | 8 ++++----
> security/integrity/ima/ima_appraise.c | 7 ++++---
> security/integrity/integrity.h | 5 +++++
> 3 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 7f3f54d89a6e..a1b42d10efc7 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -169,7 +169,7 @@ 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_ima_xattr_data)) {
> + if (xattr_len != sizeof(struct evm_xattr)) {
> evm_status = INTEGRITY_FAIL;
> goto out;
> }
> @@ -179,7 +179,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> xattr_value_len, &digest);
> if (rc)
> break;
> - rc = crypto_memneq(xattr_data->digest, digest.digest,
> + rc = crypto_memneq(xattr_data->data, digest.digest,
> SHA1_DIGEST_SIZE);
> if (rc)
> rc = -EINVAL;
> @@ -523,7 +523,7 @@ int evm_inode_init_security(struct inode *inode,
> const struct xattr *lsm_xattr,
> struct xattr *evm_xattr)
> {
> - struct evm_ima_xattr_data *xattr_data;
> + struct evm_xattr *xattr_data;
> int rc;
>
> if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
> @@ -533,7 +533,7 @@ int evm_inode_init_security(struct inode *inode,
> if (!xattr_data)
> return -ENOMEM;
>
> - xattr_data->type = EVM_XATTR_HMAC;
> + xattr_data->data.type = EVM_XATTR_HMAC;
> rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> if (rc < 0)
> goto out;
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index deec1804a00a..8bcef90939f8 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -167,7 +167,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
> return sig->hash_algo;
> break;
> case IMA_XATTR_DIGEST_NG:
> - ret = xattr_value->digest[0];
> + /* first byte contains algorithm id */
> + ret = xattr_value->data[0];
> if (ret < HASH_ALGO__LAST)
> return ret;
> break;
> @@ -175,7 +176,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
> /* this is for backward compatibility */
> if (xattr_len == 21) {
> unsigned int zero = 0;
> - if (!memcmp(&xattr_value->digest[16], &zero, 4))
> + if (!memcmp(&xattr_value->data[16], &zero, 4))
> return HASH_ALGO_MD5;
> else
> return HASH_ALGO_SHA1;
> @@ -274,7 +275,7 @@ int ima_appraise_measurement(enum ima_hooks func,
> /* xattr length may be longer. md5 hash in previous
> version occupied 20 bytes in xattr, instead of 16
> */
> - rc = memcmp(&xattr_value->digest[hash_start],
> + rc = memcmp(&xattr_value->data[hash_start],
> iint->ima_hash->digest,
> iint->ima_hash->length);
> else
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index e60473b13a8d..20ac02bf1b84 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -79,6 +79,11 @@ enum evm_ima_xattr_type {
>
> struct evm_ima_xattr_data {
> u8 type;
> + u8 data[];
> +} __packed;
> +
Please add a comment here saying that evm_xattr is limited to HMAC.
> +struct evm_xattr {
> + struct evm_ima_xattr_data data;
> u8 digest[SHA1_DIGEST_SIZE];
> } __packed;
>
