Re: [flashrom] [PATCH] internal dmi decoder

[Michael Karcher]

Am Dienstag, den 30.11.2010, 15:50 +0100 schrieb Carl-Daniel Hailfinger:
> I killed some duplicated code, but I'm sure there is more potential
> for cleanup.
>
> Sean, I remember you originally had some code which compared internal
> and external DMI decoder results. Do you still have that code
> somewhere?
>
> Add internal DMI decoder, default it to on.
>
> Signed-off-by: Sean Nelson
> -static const char *dmidecode_names[] = {
>   "system-manufacturer",
>   "system-product-name",
>   "system-version",
>   "baseboard-manufacturer",
>   "baseboard-product-name",
>   "baseboard-version",
>
> +static const struct {
>   const char *keyword;
>   unsigned char type;
>   unsigned char offset;
>
> +} flashrom_dmi_strings[] = {
>   { "system-manufacturer", 1, 0x04 },
>   { "system-product-name", 1, 0x05 },
>   { "system-version", 1, 0x06 },
>   { "baseboard-manufacturer", 2, 0x04 },
>   { "baseboard-product-name", 2, 0x05 },
>   { "baseboard-version", 2, 0x06 },
>   { "chassis-type", 3, 0x05 },

This hunk adds an entry for "chassis-type" to the DMI strings. This
seems to have unintended consequences:
1) The chassis type string is (in dmistrings[6]) also a match candidate
for DMI board enable entries, and
2) The internal DMI decoder never initializes dmistrings[6], while the
external one does.
3) If dmistrings[6] is NULL (by using the internal decoder), dmi_compare
crashes on it.
> +static char *dmi_string(char *buffer, unsigned char length, unsigned char string_id)
> +{
> size_t i, len;
> if (string_id == 0)
> return "Not Specified";

OK.
> buffer += length; /* skip to after the handle's data length byte */

Comment misleading/wrong: You don't skip until after the length byte,
but you skip "length" bytes to get past the (formatted) data.
> /* Continue till we hit a null which denotes end of string in dmi
>   or as long as we're not grabing the first string. The string 
>   should be no longer than 64 bytes. We continue looping because
>   we "jump" to the data string. */

It would be nice if you really checked that you don't hit the end of the
string table, which is indicated by a zero-length string. Currently, if
there is 1 entry in the table and the third entry is requested, this
loops skips the first valid entry in the first iteration, skips the end
marker in the second iteration and then continues past-the-structure to
get the third entry.
> for (; string_id > 1; string_id--) {
> buffer += strlen(buffer); /* skip previous data strings */

Comment misleading: This line skips only one string, of course. The
plural is obtained by looping that line, so that comment ("data
strings") should be before the loop, or changed to "skip current data
string" or "skip next data string".
> buffer++; /* skip the data string length byte */

Comment misleading/wrong: You are not skippin a length byte, you are
skipping the terminating zero. I also don't see the purpose of not just
adding "strlen(buffer)+1" in the previous line, as I consider this
expression to be a standard idiom for "size including NUL byte".
> }
> if (!*buffer) /* as long as the current byte we're on isn't null */
> return "<BAD INDEX>";

If you move this into the loop (as first instruction) you fix the
overrun-the-end-marker problem.
> len = strlen(buffer);
> if (len > 64)
> len = 64;

I don't find the limit of 64 bytes in the specs. Why don't you use
DMI_MAX_ANSWER_LEN here? Of course you may reduce it to 64, but using
the symbolic constant seems more clean than some arbitrary limit (even
if mentioned in the function header comment).
> +static int dmi_chassis_type(unsigned char code)
> +{
> int i;
> for (i = 0; i < ARRAY_SIZE(dmi_chassis_types); i++) {
> if (code == dmi_chassis_types[i].type) {
> 	break;
> }
> }

Hmm, if you don't match in this loop, you end up with i ==
ARRAY_SIZE(dmi_chassis_types), and access out-of-bound elements in the
following lines. I don't think you want that.
> msg_pdbg("DMI string chassis-type: "%s"\n", dmi_chassis_types[i].name );
> if (dmi_chassis_types[i].is_laptop) {
> msg_pdbg("Laptop detected via DMI\n");
> is_laptop = 1;
> }
> return 0;
>
> +}
> +static void dmi_table(unsigned int base, unsigned short len, unsigned short num)
> +{
> unsigned char *data;
> unsigned char *dmi_table_mem;
> int i = 0, j = 0;
> dmi_table_mem = physmap_try_ro("DMI Tables", base, len);
> if (!dmi_table_mem) {
> msg_perr("Unable to access DMI Tables\n");
> return;
> }
> data = dmi_table_mem;
> /* 4 is the length of an SMBIOS structure header */
> while (i < num && data+4 <= dmi_table_mem + len) {
> unsigned char *next;
> /*
>  * If a short entry is found (less than 4 bytes), not only it
>  * is invalid, but we cannot reliably locate the next entry.
>  * Better stop at this point, and let the user know his/her
>  * table is broken.
>  */

indeed.
> if (data[1] < 4) {
> 	msg_perr("Invalid entry length (%u). DMI table is "
> 		"broken! Stop.\n\n", (unsigned int)data[1]);
> 	break;
> }
> /* Stop decoding after chassis segment */
> if (data[0] == 4)
> 	break;

I can find no statement that DMI entries *have* to be sorted in the DMI
spec. If it's not specified, it's dangerous to rely on that (even though
the basic BIOS entries usually are sorted). If I missed the
specification about order, please give a reference.
> /* look for the next handle */
> next = data + data[1];

better comment: Skip the string section / unformatted section or
something like that
> while (next - dmi_table_mem + 1 < len && (next[0] != 0 || next[1] != 0))
> 	next++;
> next += 2;

Abort if the limit dmi_table_mem has been hit!
> for (j = 0; j < ARRAY_SIZE(flashrom_dmi_strings); j++)
> {
> 	unsigned char offset = flashrom_dmi_strings[j].offset;
> 	unsigned char type = flashrom_dmi_strings[j].type;

check data[0] == type at this point, and continue if not.
> 	if (offset >= data[1])
> 		return;

Add error message about bad DMI structure. It also is important to check
the type before, because this aborts if an encountered entry doesn't
include the offset we look for, even if that entry is for something
completely different, which is legitimately shorter.
> 	switch ((type << 8)|offset)
> 	{
> 	case 0x0305: /* detect if laptop */
> 		if (type == data[0]) {
> 			dmi_chassis_type(data[offset]);
> 		}
> 		break;
> 	default:
> 		if (type == data[0]) {
> 			dmistrings[j] = dmi_string((char*)data, data[1], data[offset]);
> 			msg_pdbg("DMI string %s: \"%s\"\n",
> 				flashrom_dmi_strings[j].keyword, dmistrings[j]);
> 		}
> 	}

This is a single case switch merging two values into one switch value.
As I explained in the beginning, adding the chassis type to the
flashrom_dmi_strings array seems like a bad idea (the chassis type is no
string, this is one root of the problem), so I would suggest to move
handling of that entry out of this loop. You can just use
  if(data[0] == 3) dmi_chassis_type(data[5]);
As the type check has already been moved to the beginning of the loop
body, just the assignment and the the msg_pdbg of this loop stay.
> }
> data = next;
> i++;
> }
> physunmap(dmi_table, len);
>
> +}
> +static int smbios_decode(unsigned char *buf)
> +{
> if (!dmi_checksum(buf, buf[0x05])
> || (memcmp(buf + 0x10, "_DMI_", 5) != 0) 
> || !dmi_checksum(buf + 0x10, 0x0F))
> 	return 0;
> dmi_table(mmio_readl(buf + 0x18), mmio_readw(buf + 0x16), mmio_readw(buf + 0x1C));
> return 1;
>
> +}
>
> +static int legacy_decode(unsigned char *buf)
> +{
> if (!dmi_checksum(buf, 0x0F))
> return 0;
> dmi_table(mmio_readl(buf + 0x08), mmio_readw(buf + 0x06), mmio_readw(buf + 0x0C));
> return 1;
>
> +}

To me, it looks like we don't care about the extra data of the more
modern "_SM_" structure and the second 16 bytes are exactly the legacy
structure, so just searching for the legacy table should be enough.
> +void dmi_init(void)
> +{
> int found = 0;
> size_t fp;
> unsigned char *dmi_mem = NULL;
> has_dmi_support = 1;
> msg_pdbg("Trying Internal DMI decoder.\n");
> dmi_mem = physmap_try_ro("DMI", 0xF0000, 0x10000);
> if (!dmi_mem)
> goto func_exit;
> for (fp = 0; fp <= 0xFFF0; fp += 16) {
> if (memcmp(dmi_mem + fp, "_SM_", 4) == 0 && fp <= 0xFFE0) {
> 	if (smbios_decode(dmi_mem+fp)) {
> 		found++;
> 		fp += 16;
> 	}
> }
> else if (memcmp(dmi_mem + fp, "_DMI_", 5) == 0)
> 	if (legacy_decode(dmi_mem + fp))
> 		found++;
> }

Do we want to handle multiple tables? Otherwise, we can just skip to
unmapping as soon as the first table was found.
> msg_pdbg("Trying External DMI decoder.\n");

s/Trying/Using/

Regards,
  Michael Karcher