linux-kernel - Re: [PATCH] firmware: dmi_scan: Prevent dmi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150327121243.GA5801@codeblueprint.co.uk>
Date:	Fri, 27 Mar 2015 12:12:43 +0000
From:	Matt Fleming <matt@...eblueprint.co.uk>
To:	Jean Delvare <jdelvare@...e.de>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Matt Fleming <matt.fleming@...el.com>,
	Ard Biesheuvel <ard.biesheuvel@...aro.org>,
	Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>
Subject: Re: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow

On Thu, 26 Mar, at 04:21:53PM, Jean Delvare wrote:
> Le Thursday 26 March 2015 à 14:47 +0000, Matt Fleming a écrit :
> > On Thu, 26 Mar, at 02:15:05PM, Jean Delvare wrote:
> > > 
> > > 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?
> 
> 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.

> 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.

---

>From bfbaafae8519d82d10da6abe75f5766dd5b20475 Mon Sep 17 00:00:00 2001
From: Jean Delvare <jdelvare@...e.de>
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 bug was introduced with the initial SMBIOS 3.0 support in commit
fc43026278b2 ("dmi: add support for SMBIOS 3.0 64-bit entry point").

Signed-off-by: Jean Delvare <jdelvare@...e.de>
Cc: Matt Fleming <matt.fleming@...el.com>
Cc: Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>
Cc: <stable@...r.kernel.org>
Acked-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>
Signed-off-by: Matt Fleming <matt.fleming@...el.com>
---
 drivers/firmware/dmi_scan.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 69fac068669f..2eebd28b4c40 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -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);
-- 
2.1.0

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/