libkmod: Fix overflow in kmod_module_hex_to_str

[stoeckmann]

If an overly long signature is found in a module file, it is possible to trigger an out of boundary write in kmod_module_hex_to_str due to integer and subsequent heap buffer overflow.

This approach replaces malloc + sprintf with a simple hex-lookup and a strbuf approach, being slightly faster in real life scenarios while adding around 100 bytes to library size. Even though it calls realloc due to strbuf_steal, sprintf for such simple format specifiers has still a larger overhead. A much faster approach could be done without strbuf and using our overflow check functions, but readability should win here.

Proof of Concept:

1. Create a module with a long signature (use any uncompressed module with .modinfo as foundation)

MODULE="/lib/modules/$(uname -r)/kernel/fs/ntfs3/ntfs3.ko"
cat > suffix.bin.b64 << EOF
AAAAAQEAAAAqqqqrfk1vZHVsZSBzaWduYXR1cmUgYXBwZW5kZWR+Cg==
EOF
base64 -d suffix.bin.b64 > suffix.bin
(cat $MODULE; dd if=/dev/zero bs=4096 count=349526; cat suffix.bin) | zstd > poc.ko.zst

2. Run modinfo

modinfo ./poc.ko.zst

If you do not run out of memory, you will see a segmentation fault.

This saves a few instructions and cycles in depmod, but the best thing about this diff is, that it removes the last sprintf call found in libkmod, replacing it with our safer and easier to handle strbuf implementation.

The previously mentioned alternative would be:

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 47c6305..bc1c35b 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -1798,28 +1798,42 @@ static struct kmod_list *kmod_module_info_append(struct kmod_list **list, const
 
 static char *kmod_module_hex_to_str(const char *hex, size_t len)
 {
-	char *str;
-	int i;
-	int j;
+	static const char hexdigits[] = "0123456789ABCDEF";
+	static const char separator[] = "\n\t\t";
+	char *s, *str;
 	const size_t line_limit = 20;
-	size_t str_len;
+	const size_t separator_len = strlen(separator);
+	size_t i, raw, str_len, tmp;
 
-	str_len = len * 3; /* XX: or XX\0 */
-	str_len += ((str_len + line_limit - 1) / line_limit - 1) * 3; /* \n\t\t */
+	/* len * 3 + ((len * 3 + line_limit - 1) / line_limit - 1) * separator_len */
+	if (umulsz_overflow(len, 3, &raw) || /* XX: or XX\0 */
+	    uaddsz_overflow(raw, line_limit, &tmp)) {
+		errno = ENOMEM;
+		return NULL;
+	}
+	tmp = ((tmp - 1) / line_limit - 1) * separator_len;
+	if (uaddsz_overflow(raw, tmp, &str_len)) {
+		errno = ENOMEM;
+		return NULL;
+	}
 
 	str = malloc(str_len);
 	if (str == NULL)
 		return NULL;
 
-	for (i = 0, j = 0; i < (int)len; i++) {
-		j += sprintf(str + j, "%02X", (unsigned char)hex[i]);
-		if (i < (int)len - 1) {
-			str[j++] = ':';
+	for (i = 0, s = str; i < len; i++) {
+		*s++ = hexdigits[(hex[i] >> 4) & 0x0F];
+		*s++ = hexdigits[hex[i] & 0x0F];
+		if (i < len - 1) {
+			*s++ = ':';
 
-			if ((i + 1) % line_limit == 0)
-				j += sprintf(str + j, "\n\t\t");
+			if ((i + 1) % line_limit == 0) {
+				memcpy(s, separator, separator_len);
+				s += separator_len;
+			}
 		}
 	}
+	*s = '\0';
 	return str;
 }

With huge signatures, it's 10 times faster, but when calling depmod, the difference is, of course, much smaller. It reduces the size of library by a few bytes, but in total I think it's not worth it to have a piece of code which is harder to read in this scenario. Still wanted to offer at least a tested alternative.

[evelikov]

I have a slight inclination towards the overflow check approach, since it consolidates the fallout into single place - aka check once and carry on, vs check every so often.

Although the strbuf approach is also fine IMHO.

Aside: github supports highlighting for code blocks - add "diff" or "c" or ... on the opening triple-backtick line.

-before
+after

[evelikov]

Nit: we can move the index declaration in the for loop.

[stoeckmann]

Adjusted

[codecov]

Codecov Report

Attention: Patch coverage is 47.05882% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
libkmod/libkmod-module.c	47.05%	6 Missing and 3 partials ⚠️

Files with missing lines	Coverage Δ
libkmod/libkmod-module.c	53.66% <47.05%> (ø)

[stoeckmann]

To simplify the alternative, we could also change the function signature to take a uint32_t, calculate with uint64_t and just check afterwards if the result is larger than SIZE_MAX. This would reduce quite some function calls, speed up the code, but also keep a manual memcpy.

Mentioned it here if either it's decided to go with this alternative instead or, alternatively, that we might perform these adjustments as "performance improvements" later on.

[lucasdemarchi]

Applied, thanks