Skip to contentSkip to conversation

integrity: Avoid -Wflex-array-member-not-at-end warnings

PATCH In review Gustavo A. R. Silva 3 participants 4 messages Mar 4, 2024 – Mar 21, 2024

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 integrity: Avoid -Wflex-array-member-not-at-end warnings

-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting ready to enable it globally.

There is currently an object (hdr) in struct ima_max_digest_data that contains a flexible structure (struct ima_digest_data):

 struct ima_max_digest_data {
	struct ima_digest_data hdr;
        u8 digest[HASH_MAX_DIGESTSIZE];

} __packed;

So, in order to avoid ending up with a flexible-array member in the middle of another struct, we use the struct_group_tagged() helper to separate the flexible array from the rest of the members in the flexible structure:

struct ima_digest_data {
        struct_group_tagged(ima_digest_data_hdr, hdr,

	... the rest of the members

        );
        u8 digest[];
} __packed;

With the change described above, we can now declare an object of the type of the tagged struct, without embedding the flexible array in the middle of another struct:

 struct ima_max_digest_data {
        struct ima_digest_data_hdr hdr;
        u8 digest[HASH_MAX_DIGESTSIZE];

} __packed;

We also use container_of() whenever we need to retrieve a pointer to the flexible structure.

So, with these changes, fix the following warnings:

security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

---
 security/integrity/ima/ima_api.c          |  6 ++++--
 security/integrity/ima/ima_appraise.c     |  4 +++-
 security/integrity/ima/ima_init.c         |  6 ++++--
 security/integrity/ima/ima_main.c         |  6 ++++--
 security/integrity/ima/ima_template_lib.c | 10 ++++++----
 security/integrity/integrity.h            |  4 +++-
 6 files changed, 24 insertions(+), 12 deletions(-)
security/integrity/ima/ima_api.c +4 -2
index b37d043d5748..c7c8d1bffb17 100644
@@ -247,6 +247,8 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
 	struct inode *real_inode = d_real_inode(file_dentry(file));
 	const char *filename = file->f_path.dentry->d_name.name;
 	struct ima_max_digest_data hash;
+	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
+						struct ima_digest_data, hdr);
 	struct kstat stat;
 	int result = 0;
 	int length;
@@ -286,9 +288,9 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
 			result = -ENODATA;
 		}
 	} else if (buf) {
-		result = ima_calc_buffer_hash(buf, size, &hash.hdr);
+		result = ima_calc_buffer_hash(buf, size, hash_hdr);
 	} else {
-		result = ima_calc_file_hash(file, &hash.hdr);
+		result = ima_calc_file_hash(file, hash_hdr);
 	}
 
 	if (result && result != -EBADF && result != -EINVAL)
security/integrity/ima/ima_appraise.c +3 -1
index 3497741caea9..656c709b974f 100644
@@ -378,7 +378,9 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint,
 		}
 
 		rc = calc_file_id_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo,
-				       iint->ima_hash->digest, &hash.hdr);
+				       iint->ima_hash->digest,
+				       container_of(&hash.hdr,
+					       struct ima_digest_data, hdr));
 		if (rc) {
 			*cause = "sigv3-hashing-error";
 			*status = INTEGRITY_FAIL;
security/integrity/ima/ima_init.c +4 -2
index 393f5c7912d5..4e208239a40e 100644
@@ -48,12 +48,14 @@ static int __init ima_add_boot_aggregate(void)
 	struct ima_event_data event_data = { .iint = iint,
 					     .filename = boot_aggregate_name };
 	struct ima_max_digest_data hash;
+	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
+						struct ima_digest_data, hdr);
 	int result = -ENOMEM;
 	int violation = 0;
 
 	memset(iint, 0, sizeof(*iint));
 	memset(&hash, 0, sizeof(hash));
-	iint->ima_hash = &hash.hdr;
+	iint->ima_hash = hash_hdr;
 	iint->ima_hash->algo = ima_hash_algo;
 	iint->ima_hash->length = hash_digest_size[ima_hash_algo];
 
@@ -70,7 +72,7 @@ static int __init ima_add_boot_aggregate(void)
 	 * is not found.
 	 */
 	if (ima_tpm_chip) {
-		result = ima_calc_boot_aggregate(&hash.hdr);
+		result = ima_calc_boot_aggregate(hash_hdr);
 		if (result < 0) {
 			audit_cause = "hashing_error";
 			goto err_out;
security/integrity/ima/ima_main.c +4 -2
index c84e8c55333d..0d3a7c864fd4 100644
@@ -941,6 +941,8 @@ int process_buffer_measurement(struct mnt_idmap *idmap,
 					    .buf_len = size};
 	struct ima_template_desc *template;
 	struct ima_max_digest_data hash;
+	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
+						struct ima_digest_data, hdr);
 	char digest_hash[IMA_MAX_DIGEST_SIZE];
 	int digest_hash_len = hash_digest_size[ima_hash_algo];
 	int violation = 0;
@@ -979,7 +981,7 @@ int process_buffer_measurement(struct mnt_idmap *idmap,
 	if (!pcr)
 		pcr = CONFIG_IMA_MEASURE_PCR_IDX;
 
-	iint.ima_hash = &hash.hdr;
+	iint.ima_hash = hash_hdr;
 	iint.ima_hash->algo = ima_hash_algo;
 	iint.ima_hash->length = hash_digest_size[ima_hash_algo];
 
@@ -990,7 +992,7 @@ int process_buffer_measurement(struct mnt_idmap *idmap,
 	}
 
 	if (buf_hash) {
-		memcpy(digest_hash, hash.hdr.digest, digest_hash_len);
+		memcpy(digest_hash, hash_hdr->digest, digest_hash_len);
 
 		ret = ima_calc_buffer_hash(digest_hash, digest_hash_len,
 					   iint.ima_hash);
security/integrity/ima/ima_template_lib.c +6 -4
index 6cd0add524cd..74198d7619da 100644
@@ -339,6 +339,8 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
 			 struct ima_field_data *field_data)
 {
 	struct ima_max_digest_data hash;
+	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
+						struct ima_digest_data, hdr);
 	u8 *cur_digest = NULL;
 	u32 cur_digestsize = 0;
 	struct inode *inode;
@@ -358,7 +360,7 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
 	if ((const char *)event_data->filename == boot_aggregate_name) {
 		if (ima_tpm_chip) {
 			hash.hdr.algo = HASH_ALGO_SHA1;
-			result = ima_calc_boot_aggregate(&hash.hdr);
+			result = ima_calc_boot_aggregate(hash_hdr);
 
 			/* algo can change depending on available PCR banks */
 			if (!result && hash.hdr.algo != HASH_ALGO_SHA1)
@@ -368,7 +370,7 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
 				memset(&hash, 0, sizeof(hash));
 		}
 
-		cur_digest = hash.hdr.digest;
+		cur_digest = hash_hdr->digest;
 		cur_digestsize = hash_digest_size[HASH_ALGO_SHA1];
 		goto out;
 	}
@@ -379,14 +381,14 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
 	inode = file_inode(event_data->file);
 	hash.hdr.algo = ima_template_hash_algo_allowed(ima_hash_algo) ?
 	    ima_hash_algo : HASH_ALGO_SHA1;
-	result = ima_calc_file_hash(event_data->file, &hash.hdr);
+	result = ima_calc_file_hash(event_data->file, hash_hdr);
 	if (result) {
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
 				    event_data->filename, "collect_data",
 				    "failed", result, 0);
 		return result;
 	}
-	cur_digest = hash.hdr.digest;
+	cur_digest = hash_hdr->digest;
 	cur_digestsize = hash.hdr.length;
 out:
 	return ima_eventdigest_init_common(cur_digest, cur_digestsize,
security/integrity/integrity.h +3 -1
index 50d6f798e613..fc1952da02ea 100644
@@ -44,6 +44,7 @@ struct evm_xattr {
 #define IMA_MAX_DIGEST_SIZE	HASH_MAX_DIGESTSIZE
 
 struct ima_digest_data {
+	struct_group_tagged(ima_digest_data_hdr, hdr,
 	u8 algo;
 	u8 length;
 	union {
@@ -57,6 +58,7 @@ struct ima_digest_data {
 		} ng;
 		u8 data[2];
 	} xattr;
+	);
 	u8 digest[];
 } __packed;
 
@@ -65,7 +67,7 @@ struct ima_digest_data {
  * with the maximum hash size, define ima_max_digest_data struct.
  */
 struct ima_max_digest_data {
-	struct ima_digest_data hdr;
+	struct ima_digest_data_hdr hdr;
 	u8 digest[HASH_MAX_DIGESTSIZE];
 } __packed;
 
Signature · 1 line
2.34.1
#1 lore raw

Hi all,

Friendly ping: who can take this, please? :)

Thanks!
--
Gustavo

Quoted 230 lines · Gustavo A. R. Silva

-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting ready to enable it globally.

There is currently an object (hdr) in struct ima_max_digest_data that contains a flexible structure (struct ima_digest_data):

  struct ima_max_digest_data {
	struct ima_digest_data hdr;
         u8 digest[HASH_MAX_DIGESTSIZE];

} __packed;

So, in order to avoid ending up with a flexible-array member in the middle of another struct, we use the struct_group_tagged() helper to separate the flexible array from the rest of the members in the flexible structure:

struct ima_digest_data {
         struct_group_tagged(ima_digest_data_hdr, hdr,

	... the rest of the members

         );
         u8 digest[];
} __packed;

With the change described above, we can now declare an object of the type of the tagged struct, without embedding the flexible array in the middle of another struct:

  struct ima_max_digest_data {
         struct ima_digest_data_hdr hdr;
         u8 digest[HASH_MAX_DIGESTSIZE];
  } __packed;

We also use container_of() whenever we need to retrieve a pointer to the flexible structure.

So, with these changes, fix the following warnings:

security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

---
  security/integrity/ima/ima_api.c          |  6 ++++--
  security/integrity/ima/ima_appraise.c     |  4 +++-
  security/integrity/ima/ima_init.c         |  6 ++++--
  security/integrity/ima/ima_main.c         |  6 ++++--
  security/integrity/ima/ima_template_lib.c | 10 ++++++----
  security/integrity/integrity.h            |  4 +++-
  6 files changed, 24 insertions(+), 12 deletions(-)
security/integrity/ima/ima_api.c +4 -2
index b37d043d5748..c7c8d1bffb17 100644
@@ -247,6 +247,8 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
  	struct inode *real_inode = d_real_inode(file_dentry(file));
  	const char *filename = file->f_path.dentry->d_name.name;
  	struct ima_max_digest_data hash;
+	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
+						struct ima_digest_data, hdr);
  	struct kstat stat;
  	int result = 0;
  	int length;
@@ -286,9 +288,9 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
  			result = -ENODATA;
  		}
  	} else if (buf) {
-		result = ima_calc_buffer_hash(buf, size, &hash.hdr);
+		result = ima_calc_buffer_hash(buf, size, hash_hdr);
  	} else {
-		result = ima_calc_file_hash(file, &hash.hdr);
+		result = ima_calc_file_hash(file, hash_hdr);
  	}
 
  	if (result && result != -EBADF && result != -EINVAL)
security/integrity/ima/ima_appraise.c +3 -1
index 3497741caea9..656c709b974f 100644
@@ -378,7 +378,9 @@ static int xattr_verify(enum ima_hooks func, struct ima_iint_cache *iint,
  		}
 
  		rc = calc_file_id_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo,
-				       iint->ima_hash->digest, &hash.hdr);
+				       iint->ima_hash->digest,
+				       container_of(&hash.hdr,
+					       struct ima_digest_data, hdr));
  		if (rc) {
  			*cause = "sigv3-hashing-error";
  			*status = INTEGRITY_FAIL;
security/integrity/ima/ima_init.c +4 -2
index 393f5c7912d5..4e208239a40e 100644
@@ -48,12 +48,14 @@ static int __init ima_add_boot_aggregate(void)
  	struct ima_event_data event_data = { .iint = iint,
  					     .filename = boot_aggregate_name };
  	struct ima_max_digest_data hash;
+	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
+						struct ima_digest_data, hdr);
  	int result = -ENOMEM;
  	int violation = 0;
 
  	memset(iint, 0, sizeof(*iint));
  	memset(&hash, 0, sizeof(hash));
-	iint->ima_hash = &hash.hdr;
+	iint->ima_hash = hash_hdr;
  	iint->ima_hash->algo = ima_hash_algo;
  	iint->ima_hash->length = hash_digest_size[ima_hash_algo];
 
@@ -70,7 +72,7 @@ static int __init ima_add_boot_aggregate(void)
  	 * is not found.
  	 */
  	if (ima_tpm_chip) {
-		result = ima_calc_boot_aggregate(&hash.hdr);
+		result = ima_calc_boot_aggregate(hash_hdr);
  		if (result < 0) {
  			audit_cause = "hashing_error";
  			goto err_out;
security/integrity/ima/ima_main.c +4 -2
index c84e8c55333d..0d3a7c864fd4 100644
@@ -941,6 +941,8 @@ int process_buffer_measurement(struct mnt_idmap *idmap,
  					    .buf_len = size};
  	struct ima_template_desc *template;
  	struct ima_max_digest_data hash;
+	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
+						struct ima_digest_data, hdr);
  	char digest_hash[IMA_MAX_DIGEST_SIZE];
  	int digest_hash_len = hash_digest_size[ima_hash_algo];
  	int violation = 0;
@@ -979,7 +981,7 @@ int process_buffer_measurement(struct mnt_idmap *idmap,
  	if (!pcr)
  		pcr = CONFIG_IMA_MEASURE_PCR_IDX;
 
-	iint.ima_hash = &hash.hdr;
+	iint.ima_hash = hash_hdr;
  	iint.ima_hash->algo = ima_hash_algo;
  	iint.ima_hash->length = hash_digest_size[ima_hash_algo];
 
@@ -990,7 +992,7 @@ int process_buffer_measurement(struct mnt_idmap *idmap,
  	}
 
  	if (buf_hash) {
-		memcpy(digest_hash, hash.hdr.digest, digest_hash_len);
+		memcpy(digest_hash, hash_hdr->digest, digest_hash_len);
 
  		ret = ima_calc_buffer_hash(digest_hash, digest_hash_len,
  					   iint.ima_hash);
security/integrity/ima/ima_template_lib.c +6 -4
index 6cd0add524cd..74198d7619da 100644
@@ -339,6 +339,8 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
  			 struct ima_field_data *field_data)
  {
  	struct ima_max_digest_data hash;
+	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
+						struct ima_digest_data, hdr);
  	u8 *cur_digest = NULL;
  	u32 cur_digestsize = 0;
  	struct inode *inode;
@@ -358,7 +360,7 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
  	if ((const char *)event_data->filename == boot_aggregate_name) {
  		if (ima_tpm_chip) {
  			hash.hdr.algo = HASH_ALGO_SHA1;
-			result = ima_calc_boot_aggregate(&hash.hdr);
+			result = ima_calc_boot_aggregate(hash_hdr);
 
  			/* algo can change depending on available PCR banks */
  			if (!result && hash.hdr.algo != HASH_ALGO_SHA1)
@@ -368,7 +370,7 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
  				memset(&hash, 0, sizeof(hash));
  		}
 
-		cur_digest = hash.hdr.digest;
+		cur_digest = hash_hdr->digest;
  		cur_digestsize = hash_digest_size[HASH_ALGO_SHA1];
  		goto out;
  	}
@@ -379,14 +381,14 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
  	inode = file_inode(event_data->file);
  	hash.hdr.algo = ima_template_hash_algo_allowed(ima_hash_algo) ?
  	    ima_hash_algo : HASH_ALGO_SHA1;
-	result = ima_calc_file_hash(event_data->file, &hash.hdr);
+	result = ima_calc_file_hash(event_data->file, hash_hdr);
  	if (result) {
  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
  				    event_data->filename, "collect_data",
  				    "failed", result, 0);
  		return result;
  	}
-	cur_digest = hash.hdr.digest;
+	cur_digest = hash_hdr->digest;
  	cur_digestsize = hash.hdr.length;
  out:
  	return ima_eventdigest_init_common(cur_digest, cur_digestsize,
security/integrity/integrity.h +3 -1
index 50d6f798e613..fc1952da02ea 100644
@@ -44,6 +44,7 @@ struct evm_xattr {
  #define IMA_MAX_DIGEST_SIZE	HASH_MAX_DIGESTSIZE
 
  struct ima_digest_data {
+	struct_group_tagged(ima_digest_data_hdr, hdr,
  	u8 algo;
  	u8 length;
  	union {
@@ -57,6 +58,7 @@ struct ima_digest_data {
  		} ng;
  		u8 data[2];
  	} xattr;
+	);
  	u8 digest[];
  } __packed;
 
@@ -65,7 +67,7 @@ struct ima_digest_data {
   * with the maximum hash size, define ima_max_digest_data struct.
   */
  struct ima_max_digest_data {
-	struct ima_digest_data hdr;
+	struct ima_digest_data_hdr hdr;
  	u8 digest[HASH_MAX_DIGESTSIZE];
  } __packed;
 
maintainer
#2 lore raw
In reply to Gustavo A. R. Silva

Hi Gustavo,

Sorry for the delay...

Quoted 29 lines · Gustavo A. R. Silva

-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting ready to enable it globally.

There is currently an object (hdr) in struct ima_max_digest_data that contains a flexible structure (struct ima_digest_data):

 struct ima_max_digest_data {
	struct ima_digest_data hdr;
        u8 digest[HASH_MAX_DIGESTSIZE];

} __packed;

So, in order to avoid ending up with a flexible-array member in the middle of another struct, we use the struct_group_tagged() helper to separate the flexible array from the rest of the members in the flexible structure:

struct ima_digest_data {
        struct_group_tagged(ima_digest_data_hdr, hdr,

	... the rest of the members

        );
        u8 digest[];
} __packed;

With the change described above, we can now declare an object of the type of the tagged struct, without embedding the flexible array in the middle of another struct:

Quoted 7 lines
 struct ima_max_digest_data {
        struct ima_digest_data_hdr hdr;
        u8 digest[HASH_MAX_DIGESTSIZE];

} __packed;

We also use container_of() whenever we need to retrieve a pointer to the flexible structure.

Nice!

Quoted 12 lines

So, with these changes, fix the following warnings:

security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not- at-end]
security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not- at-end]
security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not- at-end]

A similar change would need to be made to struct evm_digest:

struct evm_digest {
        struct ima_digest_data hdr;
        char digest[IMA_MAX_DIGEST_SIZE];
} __packed;

Is there are another patch?

Mimi

Quoted 3 lines
#3 lore raw
Quoted 3 lines · Mimi Zohar

Hi Gustavo,

Sorry for the delay...

No worries. :)

Quoted 54 lines · Gustavo A. R. Silva
Quoted 29 lines

-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting ready to enable it globally.

There is currently an object (hdr) in struct ima_max_digest_data that contains a flexible structure (struct ima_digest_data):

  struct ima_max_digest_data {
	struct ima_digest_data hdr;
         u8 digest[HASH_MAX_DIGESTSIZE];

} __packed;

So, in order to avoid ending up with a flexible-array member in the middle of another struct, we use the struct_group_tagged() helper to separate the flexible array from the rest of the members in the flexible structure:

struct ima_digest_data {
         struct_group_tagged(ima_digest_data_hdr, hdr,

	... the rest of the members

         );
         u8 digest[];
} __packed;

With the change described above, we can now declare an object of the type of the tagged struct, without embedding the flexible array in the middle of another struct:

Quoted 7 lines
  struct ima_max_digest_data {
         struct ima_digest_data_hdr hdr;
         u8 digest[HASH_MAX_DIGESTSIZE];
  } __packed;

We also use container_of() whenever we need to retrieve a pointer to the flexible structure.

Nice!

Quoted 12 lines

So, with these changes, fix the following warnings:

security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not- at-end]
security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not- at-end]
security/integrity/evm/evm.h:45:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not- at-end]

Quoted 8 lines

A similar change would need to be made to struct evm_digest:

struct evm_digest {
         struct ima_digest_data hdr;
         char digest[IMA_MAX_DIGEST_SIZE];
} __packed;

Is there are another patch?

Oh, I missed that one. I'll include it and send v2, shortly.

Thanks!
--
Gustavo