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>
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 @@
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 @@
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 @@
* - 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 @@
* 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 @@
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];