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

PATCH In review Gustavo A. R. Silva 3 participants 5 messages Apr 4, 2024 – Apr 5, 2024

Subsystems (5)

Security (LSM) Keys-trusted Checkpatch Integrity measurement architecture (IMA) +1 more subsystems

lore

Share & tools

lore

Gustavo A. R. Silva

author

2024-04-04 15:00 UTC #0 lore raw

PATCH v5 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 a struct, we use the __struct_group() helper to separate the flexible array from the rest of the members in the flexible structure:

struct ima_digest_data {
__struct_group(ima_digest_data_hdr, hdr, __packed,

... the rest of the members

);
u8 digest[];
} __packed;

And similarly for struct evm_ima_xattr_data.

With the change described above, we can now declare an object of the type of the tagged struct ima_digest_data_hdr, 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;

And similarly for struct evm_digest and struct evm_xattr.

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:64: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/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/evm/../integrity.h:68: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/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/ima/../integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/platform_certs/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] security/integrity/platform_certs/../integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

Link https://github.com/KSPP/linux/issues/202

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v5:
- Rebase patch.

Changes in v4:
- Update changelog text. (Mimi Zohar)
- Include changes for struct evm_xattr.
- Add a couple of code comments.
- Link: https://lore.kernel.org/linux-hardening/ZgsAFhl90kecIR00@neat/

Changes in v3:
- struct ima_digest_data is a packed structure. So, to keep things consistent, use the attribute __packed on the tagged struct ima_digest_data_hdr. For this, we use __struct_group() instead of struct_group_tagged(). Update the changelog text, accordingly.
- Link: https://lore.kernel.org/linux-hardening/ZfuzWku+ip4fsZrb@neat/

Changes in v2:
- Include changes for struct evm_digest (Mimi Zohar)
- Link: https://lore.kernel.org/linux-hardening/ZfuvoIj+AJHjCdTs@neat/

v1:
- Link: https://lore.kernel.org/linux-hardening/ZeYKWrXvACBBrAP8@neat/

 security/integrity/evm/evm.h              |  2 +-
 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            | 12 +++++++++---
 7 files changed, 31 insertions(+), 15 deletions(-)

security/integrity/evm/evm.h +1 -1

index eb1a2c343bd7..72e3341ae6f7 100644
@@ -61,7 +61,7 @@ extern int evm_hmac_attrs;
 extern struct list_head evm_config_xattrnames;
 
 struct evm_digest {
-	struct ima_digest_data hdr;
+	struct ima_digest_data_hdr hdr;
 	char digest[IMA_MAX_DIGEST_SIZE];
 } __packed;

security/integrity/ima/ima_api.c +4 -2

index 1856981e33df..3d286de231e1 100644
@@ -246,6 +246,8 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
 	struct inode *inode = file_inode(file);
 	struct inode *real_inode = d_real_inode(file_dentry(file));
 	struct ima_max_digest_data hash;
+	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
+						struct ima_digest_data, hdr);
 	struct name_snapshot filename;
 	struct kstat stat;
 	int result = 0;

@@ -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 4b4348d681a6..fff155f230a5 100644
@@ -948,6 +948,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;

@@ -986,7 +988,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];

@@ -997,7 +999,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 3b2cb8f1002e..4183956c53af 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 +9 -3

index 50d6f798e613..660f76cb69d3 100644
@@ -31,19 +31,24 @@ enum evm_ima_xattr_type {
 };
 
 struct evm_ima_xattr_data {
-	u8 type;
+	/* New members must be added within the __struct_group() macro below. */
+	__struct_group(evm_ima_xattr_data_hdr, hdr, __packed,
+		u8 type;
+	);
 	u8 data[];
 } __packed;
 
 /* Only used in the EVM HMAC code. */
 struct evm_xattr {
-	struct evm_ima_xattr_data data;
+	struct evm_ima_xattr_data_hdr data;
 	u8 digest[SHA1_DIGEST_SIZE];
 } __packed;
 
 #define IMA_MAX_DIGEST_SIZE	HASH_MAX_DIGESTSIZE
 
 struct ima_digest_data {
+	/* New members must be added within the __struct_group() macro below. */
+	__struct_group(ima_digest_data_hdr, hdr, __packed,
 	u8 algo;
 	u8 length;
 	union {

@@ -57,6 +62,7 @@ struct ima_digest_data {
 		} ng;
 		u8 data[2];
 	} xattr;
+	);
 	u8 digest[];
 } __packed;

@@ -65,7 +71,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

Mimi Zohar

maintainer Reviewed-by on v5 1/1

2024-04-04 16:50 UTC #1 lore raw

Hi Gustavo,

Quoted 71 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 a struct, we use the __struct_group() helper to separate the flexible array from the rest of the members in the flexible structure:
struct ima_digest_data {
__struct_group(ima_digest_data_hdr, hdr, __packed,
... the rest of the members
);
u8 digest[];
} __packed;
And similarly for struct evm_ima_xattr_data.
With the change described above, we can now declare an object of the type of the tagged struct ima_digest_data_hdr, 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;
And similarly for struct evm_digest and struct evm_xattr.
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:64: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/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array- member-not-at-end]
security/integrity/evm/../integrity.h:68: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/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array- member-not-at-end]
security/integrity/ima/../integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array- member-not-at-end]
security/integrity/integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not- at-end]
security/integrity/integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not- at-end]
security/integrity/platform_certs/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [- Wflex-array-member-not-at-end] security/integrity/platform_certs/../integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [- Wflex-array-member-not-at-end]

Quoted 2 lines

Link https://github.com/KSPP/linux/issues/202
Signed-off-by Gustavo A. R. Silva <[email protected]>

Reviewed-by Mimi Zohar <[email protected]>

Thanks, Gustavo. I already applied and tested v4, but was trying to actually see the errors with before pushing it out.

Mimi

Mimi Zohar

maintainer

2024-04-04 20:12 UTC #2 lore raw

Quoted 79 lines · Mimi Zohar

Hi Gustavo,
Quoted 76 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 a struct, we use the __struct_group() helper to separate the flexible array from the rest of the members in the flexible structure:
struct ima_digest_data {
__struct_group(ima_digest_data_hdr, hdr, __packed,
... the rest of the members
);
u8 digest[];
} __packed;
And similarly for struct evm_ima_xattr_data.
With the change described above, we can now declare an object of the type of the tagged struct ima_digest_data_hdr, 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;
And similarly for struct evm_digest and struct evm_xattr.
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:64: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/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array- member-not-at-end]
security/integrity/evm/../integrity.h:68: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/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array- member-not-at-end]
security/integrity/ima/../integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array- member-not-at-end]
security/integrity/integrity.h:40:35: warning: structure containing a flexible
array member is not at the end of another structure [-Wflex-array-member- not-
at-end]
security/integrity/integrity.h:68:32: warning: structure containing a flexible
array member is not at the end of another structure [-Wflex-array-member- not-
at-end]
security/integrity/platform_certs/../integrity.h:40:35: warning: structure containing a flexible array member is not at the end of another structure [- Wflex-array-member-not-at-end] security/integrity/platform_certs/../integrity.h:68:32: warning: structure containing a flexible array member is not at the end of another structure [- Wflex-array-member-not-at-end]

Quoted 7 lines

Quoted 2 lines
Link https://github.com/KSPP/linux/issues/202
Signed-off-by Gustavo A. R. Silva <[email protected]>
Reviewed-by Mimi Zohar <[email protected]>
Thanks, Gustavo. I already applied and tested v4, but was trying to actually see the errors with before pushing it out.

With GCC-14, I'm seeing the "warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]" messages. As expected, with the patch no warnings.

"checkpatch.pl --strict" complains "CHECK: Alignment should match open parenthesis". I'll queue the patch, but how about teaching checkpatch.pl to ignore __struct_group()?

thanks,

Mimi

Gustavo A. R. Silva

2024-04-05 00:56 UTC #3 lore raw

Quoted 4 lines

"checkpatch.pl --strict" complains "CHECK: Alignment should match open parenthesis". I'll queue the patch, but how about teaching checkpatch.pl to ignore __struct_group()?

I think this would do it:

scripts/checkpatch.pl +1 -1

index 9c4c4a61bc83..e229b97f17f6 100755
@@ -3958,7 +3958,7 @@ sub process {
                        my $rest = $2;
 
                        my $pos = pos_last_openparen($rest);
-                       if ($pos >= 0) {
+                       if ($pos >= 0 && $rest !~ /(__)?struct_group(_(tagged|attr))?(\()/) {
                                $line =~ /^(\+| )([ \t]*)/;
                                my $newindent = $2;

I'll send a proper patch. Thanks for the suggestion, Mimi.

--
Gustavo

Mimi Zohar

maintainer

2024-04-05 14:17 UTC #4 lore raw

Quoted 20 lines · Gustavo A. R. Silva

Quoted 3 lines

"checkpatch.pl --strict" complains "CHECK: Alignment should match open parenthesis". I'll queue the patch, but how about teaching checkpatch.pl to ignore __struct_group()?

I think this would do it:

scripts/checkpatch.pl +1 -1

index 9c4c4a61bc83..e229b97f17f6 100755
@@ -3958,7 +3958,7 @@ sub process {
                         my $rest = $2;
 
                         my $pos = pos_last_openparen($rest);
-                       if ($pos >= 0) {
+                       if ($pos >= 0 && $rest !~
 /(__)?struct_group(_(tagged|attr))?(\()/) {
                                 $line =~ /^(\+| )([ \t]*)/;
                                 my $newindent = $2;

Quoted 1 line

I'll send a proper patch. Thanks for the suggestion, Mimi.

Thanks, it works.

Mimi