Fix garbage handling in DMI strings

Previously we tried to replace garbage characters with <space> directly in
the read-only memory-mapped SMBIOS area(!). This could never have
worked for any DMI strings with garbage and results in a segfault on
machines with such strings.

Thanks to Brian Rak (Supermicro X10SLE-F) and John Pohlman (HP XW9400)
for reporting this issue.

With this patch the strings are duplicated within dmi_string() already,
just before we sanitize them. Also, the limit variable used everywhere
points to the first invalid byte address. Refine respective checks
accordingly.

Corresponding to flashrom svn r1824.

Signed-off-by: Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>

1 files changed, 35 insertions, 17 deletions

diff --git a/dmi.c b/dmi.c
index 25a4957..f329fab 100644
--- a/dmi.c
+++ b/dmi.c
@@ -23,6 +23,7 @@
 
 #include <strings.h>
 #include <string.h>
+#include <ctype.h>
 #include <stdio.h>
 #include <stdlib.h>
 
@@ -105,7 +106,16 @@ static bool dmi_checksum(const uint8_t * const buf, size_t len)
 	return (sum == 0);
 }
 
-static char *dmi_string(uint8_t *buf, uint8_t string_id, uint8_t *limit)
+/** Retrieve a DMI string.
+ *
+ * See SMBIOS spec. section 6.1.3 "Text strings".
+ * The table will be unmapped ASAP, hence return a duplicated & sanitized string that needs to be freed later.
+ *
+ * \param buf		the buffer to search through (usually appended directly to a DMI structure)
+ * \param string_id	index of the string to look for
+ * \param limit		pointer to the first byte beyond \em buf
+ */
+static char *dmi_string(const char *buf, uint8_t string_id, const char *limit)
 {
 	size_t i, len;
 
@@ -113,26 +123,33 @@ static char *dmi_string(uint8_t *buf, uint8_t string_id, uint8_t *limit)
 		return "Not Specified";
 
 	while (string_id > 1 && string_id--) {
-		if (buf > limit) {
+		if (buf >= limit) {
 			msg_perr("DMI table is broken (string portion out of bounds)!\n");
 			return "<OUT OF BOUNDS>";
 		}
-		buf += strnlen((char *)buf, limit - buf) + 1;
+		buf += strnlen(buf, limit - buf) + 1;
 	}
 
 	if (!*buf) /* as long as the current byte we're on isn't null */
 		return "<BAD INDEX>";
 
-	len = strnlen((char *)buf, limit - buf);
-	if (len > DMI_MAX_ANSWER_LEN)
-		len = DMI_MAX_ANSWER_LEN;
+	len = strnlen(buf, limit - buf);
+	char *newbuf = malloc(len + 1);
+	if (newbuf == NULL) {
+		msg_perr("Out of memory!\n");
+		return NULL;
+	}
 
 	/* fix junk bytes in the string */
-	for (i = 0; i < len; i++)
-		if (buf[i] < 32 || buf[i] >= 127)
-			buf[i] = ' ';
+	for (i = 0; i < len && buf[i] != '\0'; i++) {
+		if (isprint(buf[i]))
+			newbuf[i] = buf[i];
+		else
+			newbuf[i] = ' ';
+	}
+	newbuf[i] = '\0';
 
-	return (char *)buf;
+	return newbuf;
 }
 
 static void dmi_chassis_type(uint8_t code)
@@ -167,7 +184,7 @@ static void dmi_table(uint32_t base, uint16_t len, uint16_t num)
 	 *  - uint8_t length;	// data section w/ header w/o strings
 	 *  - uint16_t handle;
 	 */
-	while (i < num && data + 4 <= limit) {
+	while (i < num && data + 4 < limit) {
 		/* - If a short entry is found (less than 4 bytes), not only it
 		 *   is invalid, but we cannot reliably locate the next entry.
 		 * - If the length value indicates that this structure spreads
@@ -175,15 +192,16 @@ static void dmi_table(uint32_t base, uint16_t len, uint16_t num)
 		 * Better stop at this point, and let the user know his/her
 		 * table is broken.
 		 */
-		if (data[1] < 4 || data + data[1] > limit) {
+		if (data[1] < 4 || data + data[1] >= limit) {
 			msg_perr("DMI table is broken (bogus header)!\n");
 			break;
 		}
 
 		if(data[0] == 3) {
-			if (data + 5 <= limit)
+			if (data + 5 < limit)
 				dmi_chassis_type(data[5]);
-			/* else the table is broken, but laptop detection is optional, hence continue. */
+			else /* the table is broken, but laptop detection is optional, hence continue. */
+				msg_pwarn("DMI table is broken (chassis_type out of bounds)!\n");
 		} else
 			for (j = 0; j < ARRAY_SIZE(dmi_strings); j++) {
 				uint8_t offset = dmi_strings[j].offset;
@@ -192,13 +210,13 @@ static void dmi_table(uint32_t base, uint16_t len, uint16_t num)
 				if (data[0] != type)
 					continue;
 
-				if (data[1] <= offset || data+offset > limit) {
+				if (data[1] <= offset || data + offset >= limit) {
 					msg_perr("DMI table is broken (offset out of bounds)!\n");
 					goto out;
 				}
 
-				/* Table will be unmapped, hence fill the struct with duplicated strings. */
-				dmi_strings[j].value = strdup(dmi_string(data + data[1], data[offset], limit));
+				dmi_strings[j].value = dmi_string((const char *)(data + data[1]), data[offset],
+								  (const char *)limit);
 			}
 		/* Find next structure by skipping data and string sections */
 		data += data[1];