| 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: <b0ba8db6-cf85-0ee3-af19-c8b1251d5b63@linux.ibm.com>
Date: Mon, 14 Feb 2022 15:13:32 -0500
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Mimi Zohar <zohar@...ux.ibm.com>, linux-integrity@...r.kernel.org
Cc: Eric Biggers <ebiggers@...nel.org>, linux-fscrypt@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/8] ima: define ima_max_digest_data struct without a
flexible array variable
On 2/11/22 16:43, Mimi Zohar wrote:
> To support larger hash digests in the 'iint' cache, instead of defining
> the 'digest' field as the maximum digest size, the 'digest' field was
> defined as a flexible array variable. The "ima_digest_data" struct was
> wrapped inside a local structure with the maximum digest size. But
> before adding the record to the iint cache, memory for the exact digest
> size was dynamically allocated.
>
> The original reason for defining the 'digest' field as a flexible array
> variable is still valid for the 'iint' cache use case. Instead of
> wrapping the 'ima_digest_data' struct in a local structure define
> 'ima_max_digest_data' struct.
>
> Signed-off-by: Mimi Zohar <zohar@...ux.ibm.com>
> ---
> security/integrity/ima/ima_api.c | 10 ++++------
> security/integrity/ima/ima_init.c | 5 +----
> security/integrity/ima/ima_main.c | 5 +----
> security/integrity/ima/ima_template_lib.c | 5 +----
> security/integrity/integrity.h | 10 ++++++++++
> 5 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 5b220a2fe573..c6805af46211 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -217,14 +217,11 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> const char *audit_cause = "failed";
> struct inode *inode = file_inode(file);
> const char *filename = file->f_path.dentry->d_name.name;
> + struct ima_max_digest_data hash;
> int result = 0;
> int length;
> void *tmpbuf;
> u64 i_version;
> - struct {
> - struct ima_digest_data hdr;
> - char digest[IMA_MAX_DIGEST_SIZE];
> - } hash;
>
> /*
> * Always collect the modsig, because IMA might have already collected
> @@ -239,8 +236,9 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>
> /*
> * Detecting file change is based on i_version. On filesystems
> - * which do not support i_version, support is limited to an initial
> - * measurement/appraisal/audit.
> + * which do not support i_version, support was originally limited
> + * to an initial measurement/appraisal/audit, but was modified to
> + * assume the file changed.
> */
> i_version = inode_query_iversion(inode);
> hash.hdr.algo = algo;
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index b26fa67476b4..63979aefc95f 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -47,12 +47,9 @@ static int __init ima_add_boot_aggregate(void)
> struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
> struct ima_event_data event_data = { .iint = iint,
> .filename = boot_aggregate_name };
> + struct ima_max_digest_data hash;
> int result = -ENOMEM;
> int violation = 0;
> - struct {
> - struct ima_digest_data hdr;
> - char digest[TPM_MAX_DIGEST_SIZE];
> - } hash;
>
> memset(iint, 0, sizeof(*iint));
> memset(&hash, 0, sizeof(hash));
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 7c80dfe2c7a5..c6412dec3810 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -874,10 +874,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
> .buf = buf,
> .buf_len = size};
> struct ima_template_desc *template;
> - struct {
> - struct ima_digest_data hdr;
> - char digest[IMA_MAX_DIGEST_SIZE];
> - } hash = {};
> + struct ima_max_digest_data hash;
It looks like the initialization wasn't necessary.
Reviewed-by: Stefan Berger <stefanb@...ux.ibm.com>
> char digest_hash[IMA_MAX_DIGEST_SIZE];
> int digest_hash_len = hash_digest_size[ima_hash_algo];
> int violation = 0;
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index 5a5d462ab36d..7155d17a3b75 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -307,10 +307,7 @@ static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
> int ima_eventdigest_init(struct ima_event_data *event_data,
> struct ima_field_data *field_data)
> {
> - struct {
> - struct ima_digest_data hdr;
> - char digest[IMA_MAX_DIGEST_SIZE];
> - } hash;
> + struct ima_max_digest_data hash;
> u8 *cur_digest = NULL;
> u32 cur_digestsize = 0;
> struct inode *inode;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index d045dccd415a..daf49894fd7d 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -15,6 +15,7 @@
> #include <linux/types.h>
> #include <linux/integrity.h>
> #include <crypto/sha1.h>
> +#include <crypto/hash.h>
> #include <linux/key.h>
> #include <linux/audit.h>
>
> @@ -110,6 +111,15 @@ struct ima_digest_data {
> u8 digest[];
> } __packed;
>
> +/*
> + * Instead of wrapping the ima_digest_data struct inside a local structure
> + * with the maximum hash size, define ima_max_digest_data struct.
> + */
> +struct ima_max_digest_data {
> + struct ima_digest_data hdr;
> + u8 digest[HASH_MAX_DIGESTSIZE];
> +} __packed;
> +
> /*
> * signature format v2 - for using with asymmetric keys
> */
Powered by blists - more mailing lists