Skip to contentSkip to conversation

ima: Add __counted_by for struct modsig and use struct_size()

PATCH Applied Gustavo A. R. Silva 3 participants 4 messages Oct 9, 2023 – Oct 24, 2023

Subsystems (4)
Security (LSM) Keys-trusted Integrity measurement architecture (IMA) Extended Verification Module (EVM)
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 ima: Add __counted_by for struct modsig and use struct_size()

Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).

Also, relocate hdr->raw_pkcs7_len = sig_len; so that the __counted_by annotation has effect, and flex-array member raw_pkcs7 can be properly bounds-checked at run-time.

While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member.

This code was found with the help of Coccinelle, and audited and fixed manually.

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 security/integrity/ima/ima_modsig.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
security/integrity/ima/ima_modsig.c +3 -3
index 3e7bee30080f..3265d744d5ce 100644
@@ -29,7 +29,7 @@ struct modsig {
 	 * storing the signature.
 	 */
 	int raw_pkcs7_len;
-	u8 raw_pkcs7[];
+	u8 raw_pkcs7[] __counted_by(raw_pkcs7_len);
 };
 
 /*

@@ -65,10 +65,11 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
 	buf_len -= sig_len + sizeof(*sig);
 
 	/* Allocate sig_len additional bytes to hold the raw PKCS#7 data. */
-	hdr = kzalloc(sizeof(*hdr) + sig_len, GFP_KERNEL);
+	hdr = kzalloc(struct_size(hdr, raw_pkcs7, sig_len), GFP_KERNEL);
 	if (!hdr)
 		return -ENOMEM;
 
+	hdr->raw_pkcs7_len = sig_len;
 	hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len);
 	if (IS_ERR(hdr->pkcs7_msg)) {
 		rc = PTR_ERR(hdr->pkcs7_msg);

@@ -77,7 +78,6 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
 	}
 
 	memcpy(hdr->raw_pkcs7, buf + buf_len, sig_len);
-	hdr->raw_pkcs7_len = sig_len;
 
 	/* We don't know the hash algorithm yet. */
 	hdr->hash_algo = HASH_ALGO__LAST;
Signature · 1 line
2.34.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 10 lines · Gustavo A. R. Silva

Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).

Also, relocate hdr->raw_pkcs7_len = sig_len; so that the __counted_by annotation has effect, and flex-array member raw_pkcs7 can be properly bounds-checked at run-time.

Quoted 8 lines

While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member.

This code was found with the help of Coccinelle, and audited and fixed manually.

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

Looks right; good catch on moving the assignment.

Reviewed-by Kees Cook <[email protected]>
Signature · 1 line
Kees Cook
maintainer Reviewed-by on v1 1/1
#2 lore raw
In reply to Gustavo A. R. Silva
Quoted 10 lines · Gustavo A. R. Silva

Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).

Also, relocate hdr->raw_pkcs7_len = sig_len; so that the __counted_by annotation has effect, and flex-array member raw_pkcs7 can be properly bounds-checked at run-time.

Quoted 8 lines

While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member.

This code was found with the help of Coccinelle, and audited and fixed manually.

Signed-off-by Gustavo A. R. Silva <[email protected]>
Reviewed-by Mimi Zohar <[email protected]>
Applied
#3 lore raw
In reply to Gustavo A. R. Silva
Quoted 11 lines · Gustavo A. R. Silva

Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions).

Also, relocate hdr->raw_pkcs7_len = sig_len; so that the __counted_by annotation has effect, and flex-array member raw_pkcs7 can be properly bounds-checked at run-time.

[...]

Applied to for-next/hardening, thanks!

[1/1] ima: Add __counted_by for struct modsig and use struct_size()

      https://git.kernel.org/kees/c/68a8f644575b

Take care,

Signature · 1 line
Kees Cook