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.
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. Signed-off-by: Stefan Tauner <[email protected]> --- dmi.c | 49 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/dmi.c b/dmi.c index 25a4957..cd6e434 100644 --- a/dmi.c +++ b/dmi.c @@ -105,7 +105,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 +122,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++) + for (i = 0; i < len && buf[i] != '\0'; i++) { if (buf[i] < 32 || buf[i] >= 127) - buf[i] = ' '; + newbuf[i] = ' '; + else + newbuf[i] = buf[i]; + } + newbuf[i] = '\0'; - return (char *)buf; + return newbuf; } static void dmi_chassis_type(uint8_t code) @@ -167,7 +183,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 +191,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 +209,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]; -- Kind regards, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
