firmware: dmi_scan: Prevent dmi_num integer overflow

dmi_num is a u16, dmi_len is a u32, so this construct:

        dmi_num = dmi_len / 4;

would result in an integer overflow for a DMI table larger than 256 kB. I've never see such a large table so far, but SMBIOS 3.0 makes it possible so maybe we'll see such tables in the future.

So instead of faking a structure count when the entry point does not provide it, adjust the loop condition in dmi_table() to properly deal with the case where dmi_num is not set.

Signed-off-by Jean Delvare <[email protected]>

Cc Matt Fleming <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Ivan Khoronzhuk <[email protected]>

---
 drivers/firmware/dmi_scan.c |   22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

drivers/firmware/dmi_scan.c 2015-03-20 09:23:37.689542485 +0100 renamed from drivers/firmware/dmi_scan.c 2015-03-19 14:19:56.384426825 +0100 +7 -16

@@ -86,10 +86,13 @@ static void dmi_table(u8 *buf, u32 len,
        int i = 0;
 
        /*
-        *      Stop when we see all the items the table claimed to have
-        *      OR we run off the end of the table (also happens)
+        * Stop when we have seen all the items the table claimed to have
+        * (SMBIOS < 3.0 only) OR we reach an end-of-table marker OR we run
+        * off the end of the table (should never happen but sometimes does
+        * on bogus implementations.)
         */
-       while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
+       while ((!num || i < num) &&
+              (data - buf + sizeof(struct dmi_header)) <= len) {
                const struct dmi_header *dm = (const struct dmi_header *)data;
 
                /*

@@ -529,21 +532,10 @@ static int __init dmi_smbios3_present(co
        if (memcmp(buf, "_SM3_", 5) == 0 &&
            buf[6] < 32 && dmi_checksum(buf, buf[6])) {
                dmi_ver = get_unaligned_be16(buf + 7);
+               dmi_num = 0;                    /* No longer specified */
                dmi_len = get_unaligned_le32(buf + 12);
                dmi_base = get_unaligned_le64(buf + 16);
 
-               /*
-                * The 64-bit SMBIOS 3.0 entry point no longer has a field
-                * containing the number of structures present in the table.
-                * Instead, it defines the table size as a maximum size, and
-                * relies on the end-of-table structure type (#127) to be used
-                * to signal the end of the table.
-                * So let's define dmi_num as an upper bound as well: each
-                * structure has a 4 byte header, so dmi_len / 4 is an upper
-                * bound for the number of structures in the table.
-                */
-               dmi_num = dmi_len / 4;
-
                if (dmi_walk_early(dmi_decode) == 0) {
                        pr_info("SMBIOS %d.%d present.\n",
                                dmi_ver >> 8, dmi_ver & 0xFF);
 
 
--

Jean Delvare
SUSE L3 Support

dmi_num is a u16, dmi_len is a u32, so this construct:

	dmi_num = dmi_len / 4;

would result in an integer overflow for a DMI table larger than 256 kB. I've never see such a large table so far, but SMBIOS 3.0 makes it possible so maybe we'll see such tables in the future.

So instead of faking a structure count when the entry point does not provide it, adjust the loop condition in dmi_table() to properly deal with the case where dmi_num is not set.

Signed-off-by Jean Delvare <[email protected]>

Cc Matt Fleming <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Ivan Khoronzhuk <[email protected]>

---
 drivers/firmware/dmi_scan.c |   22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

Quoted 12 lines · Jean Delvare

dmi_num is a u16, dmi_len is a u32, so this construct:

	dmi_num = dmi_len / 4;

would result in an integer overflow for a DMI table larger than 256 kB. I've never see such a large table so far, but SMBIOS 3.0 makes it possible so maybe we'll see such tables in the future.

So instead of faking a structure count when the entry point does not provide it, adjust the loop condition in dmi_table() to properly deal with the case where dmi_num is not set.

Quoted 7 lines

Signed-off-by Jean Delvare <[email protected]>

Cc Matt Fleming <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Ivan Khoronzhuk <[email protected]>

---
 drivers/firmware/dmi_scan.c |   22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

Jean, are you taking this through your tree?

I don't actually have a tree, so feel free to pick it.

From 8aabaf128c85bab02fdbe594481826e137c25da4 Mon Sep 17 00:00:00 2001

Quoted 2 lines · Jean Delvare

I don't actually have a tree, so feel free to pick it.

OK will do, but this is a regression fix for a potential bug introduced in commit 6d9ff4733172 ("firmware: dmi_scan: Fix dmi_len type"), right?

If so, it would be useful for this patch to make reference to that commit.

How about this?

---

From 8aabaf128c85bab02fdbe594481826e137c25da4 Mon Sep 17 00:00:00 2001
From: Jean Delvare <[email protected]>
Date: Fri, 20 Mar 2015 09:59:47 +0100
Subject: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow

dmi_num is a u16, dmi_len is a u32, so this construct:

	dmi_num = dmi_len / 4;

would result in an integer overflow for a DMI table larger than 256 kB. I've never see such a large table so far, but SMBIOS 3.0 makes it possible so maybe we'll see such tables in the future.

So instead of faking a structure count when the entry point does not provide it, adjust the loop condition in dmi_table() to properly deal with the case where dmi_num is not set.

This potential overflow was introduced in commit 6d9ff4733172 ("firmware: dmi_scan: Fix dmi_len type").

Signed-off-by Jean Delvare <[email protected]>

Cc Matt Fleming <[email protected]>

Cc Ivan Khoronzhuk <[email protected]>

Cc <[email protected]>

Acked-by Ard Biesheuvel <[email protected]>

Signed-off-by Matt Fleming <[email protected]>

---
 drivers/firmware/dmi_scan.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

drivers/firmware/dmi_scan.c +7 -15

index 69fac068669f..2eebd28b4c40 100644
@@ -86,10 +86,13 @@ static void dmi_table(u8 *buf, u32 len, int num,
 	int i = 0;
 
 	/*
-	 *	Stop when we see all the items the table claimed to have
-	 *	OR we run off the end of the table (also happens)
+	 * Stop when we have seen all the items the table claimed to have
+	 * (SMBIOS < 3.0 only) OR we reach an end-of-table marker OR we run
+	 * off the end of the table (should never happen but sometimes does
+	 * on bogus implementations.)
 	 */
-	while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
+	while ((!num || i < num) &&
+	       (data - buf + sizeof(struct dmi_header)) <= len) {
 		const struct dmi_header *dm = (const struct dmi_header *)data;
 
 		/*

@@ -529,21 +532,10 @@ static int __init dmi_smbios3_present(const u8 *buf)
 	if (memcmp(buf, "_SM3_", 5) == 0 &&
 	    buf[6] < 32 && dmi_checksum(buf, buf[6])) {
 		dmi_ver = get_unaligned_be16(buf + 7);
+		dmi_num = 0;			/* No longer specified */
 		dmi_len = get_unaligned_le32(buf + 12);
 		dmi_base = get_unaligned_le64(buf + 16);
 
-		/*
-		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
-		 * containing the number of structures present in the table.
-		 * Instead, it defines the table size as a maximum size, and
-		 * relies on the end-of-table structure type (#127) to be used
-		 * to signal the end of the table.
-		 * So let's define dmi_num as an upper bound as well: each
-		 * structure has a 4 byte header, so dmi_len / 4 is an upper
-		 * bound for the number of structures in the table.
-		 */
-		dmi_num = dmi_len / 4;
-
 		if (dmi_walk_early(dmi_decode) == 0) {
 			pr_info("SMBIOS %d.%d present.\n",
 				dmi_ver >> 8, dmi_ver & 0xFF);

Signature · 1 line

2.1.0

Quoted 4 lines · Matt Fleming

Quoted 2 lines · Jean Delvare

I don't actually have a tree, so feel free to pick it.

Quoted 2 lines

OK will do, but this is a regression fix for a potential bug introduced in commit 6d9ff4733172 ("firmware: dmi_scan: Fix dmi_len type"), right?

Not really. I'd rather say it complements commit 6d9ff4733172. The actual bugs were introduced in commit fc43026278b2. But you can't really call it a regression as SMBIOS 3.0 entry points were not supported at all before that commit.

Still a reference to commit fc43026278b2 would certainly be appropriate, as anyone backporting that commit will also want to pick the two aforementioned fix-ups (plus ce204e9a4bd8 for that matter.)

From bfbaafae8519d82d10da6abe75f5766dd5b20475 Mon Sep 17 00:00:00 2001

Quoted 5 lines · Jean Delvare

Quoted 4 lines · Matt Fleming

Quoted 2 lines · Jean Delvare

I don't actually have a tree, so feel free to pick it.

Quoted 7 lines

Quoted 2 lines

OK will do, but this is a regression fix for a potential bug introduced in commit 6d9ff4733172 ("firmware: dmi_scan: Fix dmi_len type"), right?

Not really. I'd rather say it complements commit 6d9ff4733172. The actual bugs were introduced in commit fc43026278b2. But you can't really call it a regression as SMBIOS 3.0 entry points were not supported at all before that commit.

OK, thanks for clarifying.

Quoted 3 lines

Still a reference to commit fc43026278b2 would certainly be appropriate, as anyone backporting that commit will also want to pick the two aforementioned fix-ups (plus ce204e9a4bd8 for that matter.)

commit ce204e9a4bd8 and 6d9ff4733172 are already tagged for stable, so we should be good there.

This is what I've got queued up to send to tip today. Feel free to send me another version today if you want to word things differently.