Hi Neil, We are getting there, thanks for your persistence. Comments inline.
On Fri, 7 Sep 2018 14:00:32 -0400, Neil Horman wrote: > Starting with version 3.2.0 the SMBIOS specification defined in more > detail the contents of the management controller type. DMTF further > reserved values to define the Redfish host interface specification. > Update dmidecode to properly parse and present that information. > > Signed-off-by: Neil Horman <[email protected]> > CC: [email protected] > CC: [email protected] > CC: [email protected] > CC: [email protected] > > --- > Change Notes: > V1->V2) Updated string formatting to print matching number of bytes > for unsigned shorts ([email protected]) > > Adjusted string format for bDescriptor ([email protected]) > > Prefaced PCI id's with 0x ([email protected]) > V2->V3) Updated word and dword accesses to do appropriate endian > conversion > > Updated Interface type and protocol type lists to reflect > overall SMBIOS spec rather than just RedFish host spec, and stay > more compatible with pre version 3 SMBIOS layouts > > Adjusted IFC_PROTO_RECORD_BASE to be 6 rather than 7, as this is > in keeping with the spec, and is validated against the overall > type 42 record length in his dmidecode dump. I'm convinced that > the layout of the system I'm testing on has an extra byte > inserted between the protocol record count and the start of the > protocol records. > V3->V4) Moved type 42 defines to more appropriate section > > Removed defines to avoid namespace clashes > > Renamed function to dmi_parse_controller_structure > > Renamed PCI[e] to PCI/PCIe > > Added check to make sure structure length is sane > > Consolidated Host Interface parsing to > dmi_management_controller_host_type > > Restrict the controller parsing structure to only decode network > interface types > > Validate that strucure recorded length is enough to decode ifc > specific data > > Removed bString comment > > Corrected protocol count comment > > Separated protocol parsing to its own function > > Adjusted hlen size > > Fixed Ip/IP casing > > Test each protocol record for containment in overall struct > length > > Use dmi_system_uuid > > Converted ennumeration string lookups to their own function > > Guaranteed the non-null-ness of inet_ntop (note, inet_ntop is > POSIX-2001 compliant and so should be very portable) > > Cleaned up host name copy using printf string precision > specifier > > Cleaned up smbios version check > > Fixed record cursor advancement > > Misc cleanups > > V4->V5) Removed references to DSP0134 > > Removed else statements that were not strictly needed > > Moved type 42 helpers to a better location > > Changed naming of some ennumerations > > Changed helper string check order to match spec > > Added some comments to dmi_parse_protocol_record > > Break up some long lines > > Spelling fixes > > Convert aval (assign_val) to be more clear > > Add hostname length check in proto record print > > Removed USB serial number display (its not useful) > > Make OEM IANA access safe on arches that can't handle unaligned > access > > Created a variable to track the running length of data we've > read > --- > dmidecode.c | 390 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 376 insertions(+), 14 deletions(-) > > diff --git a/dmidecode.c b/dmidecode.c > index 76faed9..46f39cb 100644 > --- a/dmidecode.c > +++ b/dmidecode.c > @@ -56,6 +56,8 @@ > * - "PC Client Platform TPM Profile (PTP) Specification" > * Family "2.0", Level 00, Revision 00.43, January 26, 2015 > * > https://trustedcomputinggroup.org/pc-client-platform-tpm-profile-ptp-specification/ > + * - "RedFish Host Interface Specification" (DMTF DSP0270) > + * https://www.dmtf.org/sites/default/files/DSP0270_1.0.1.pdf > */ > > #include <stdio.h> > @@ -63,6 +65,7 @@ > #include <strings.h> > #include <stdlib.h> > #include <unistd.h> > +#include <arpa/inet.h> > > #ifdef __FreeBSD__ > #include <errno.h> > @@ -3391,11 +3394,81 @@ static const char > *dmi_management_controller_host_type(u8 code) > > if (code >= 0x02 && code <= 0x08) > return type[code - 0x02]; > + if (code <= 0x3F) > + return "MCTP"; > + if (code == 0x40) > + return "Network"; > if (code == 0xF0) > return "OEM"; > return out_of_spec; > } > > +/* > + * 7.43.2: Protocol Record Types > + */ > +static const char *dmi_protocol_record_type(u8 type) > +{ > + const char *protocol[] = { > + "Reserved", /* 0x0 */ > + "Reserved", > + "IPMI", > + "MCTP", > + "Redfish over IP", /* 0x4 */ > + }; > + > + if (type <= 0x4) > + return protocol[type]; > + if (type == 0xF0) > + return "OEM"; > + return out_of_spec; > +} > + > +/* > + * DSP0270: 8.6: Protocol IP Assignment types > + */ > +static const char *dmi_protocol_assignment_type(u8 type) > +{ > + const char *assignment[] = { > + "Unknown", /* 0x0 */ > + "Static", > + "DHCP", > + "AutoConf", > + "Host Selected", /* 0x4 */ > + }; > + > + if (type <= 0x4) > + return assignment[type]; > + return out_of_spec; > +} > + > +/* > + * DSP0270: 8.6: Protocol IP Address type > + */ > +static const char *dmi_address_type(u8 type) > +{ > + const char *addressformat[] = { > + "Unknown", /* 0x0 */ > + "IPv4", > + "IPv6", /* 0x2 */ > + }; > + > + if (type <= 0x2) > + return addressformat[type]; > + return out_of_spec; > +} > + > +/* > + * DSP0270: 8.6 Protocol Address decode > + */ > +static const char *dmi_address_decode(u8 *data, char *storage, u8 addrtype) > +{ > + if (addrtype == 0x1) /* IPv4 */ > + return inet_ntop(AF_INET, data, storage, 64); > + if (addrtype == 0x2) /*IPv6 */ Space after /* > + return inet_ntop(AF_INET6, data, storage, 64); > + return out_of_spec; > +} > + > /* > * 7.44 TPM Device (Type 43) > */ > @@ -3447,6 +3520,290 @@ static void dmi_tpm_characteristics(u64 code, const > char *prefix) > prefix, characteristics[i - 2]); > } > > +/* > + * DSP0270: 8.5: Parse the protocol record format > + */ > +static void dmi_parse_protocol_record(const char *prefix, u8 *rec) > +{ > + u8 rid; > + u8 rlen; > + u8 *rdata; > + char buf[64]; > + u8 assign_val; > + u8 addrtype; > + u8 hlen; > + const char *hname; > + > + /* DSP0270: 8.5: Protocol Identifier */ > + rid = rec[0x0]; > + /* DSP0270: 8.5: Protocol Record Length */ > + rlen = rec[0x1]; > + /* DSP0270: 8.5: Protocol Record Data */ > + rdata = &rec[0x2]; > + > + printf("%s\tProtocol ID: %02x (%s)\n", prefix, rid, > + dmi_protocol_record_type(rid)); > + > + /* > + * Don't decode anything other than Redfish for now > + * Note 0x4 is Redfish over IP in 7.43.2 > + * and DSP0270: 8.5 > + */ > + if (rid != 0x4) > + return; You are still missing a length check here: rlen must be at least 91 for Redfish. > + > + /* > + * DSP0270: 8.6: Redfish Over IP Service UUID > + * Note: ver is hardcoded to 0x311 here just for > + * convenience. It could get passed from the SMBIOS > + * header, but that's a lot of passing of pointers just > + * to get that info, and the only thing it is used for is > + * to determine the endianess of the field. Since we only > + * do this parsing on versions of smbios after 3.1.1, and the Spelling: SMBIOS (uppercase). > + * endianess of the field is always little after version 2.6.0 > + * we can just pick a sufficiently recent version here. > + */ > + printf("%s\t\tService UUID: ", prefix); > + dmi_system_uuid(&rdata[0], 0x311); > + printf("\n"); > + > + /* > + * DSP0270: 8.6: Redfish Over IP Host IP Assignment Type > + * Note, using decimal indicies here, as the DSP0270 > + * uses decimal, so as to make it more comparable > + */ > + assign_val = rdata[16]; > + printf("%s\t\tHost IP Assignment Type: %s\n", prefix, > + dmi_protocol_assignment_type(assign_val)); > + > + /* DSP0270: 8.6: Redfish Over IP Host Address format */ > + addrtype = rdata[17]; > + Blank line not needed. > + printf("%s\t\tHost IP Address Format: %s\n", prefix, > + dmi_address_type(addrtype)); > + > + /* DSP0270: 8.6 IP Assignment types */ > + /* We only use the Host IP Address and Mask if the assignment type is > static */ > + if (assign_val == 0x1 || assign_val == 0x3) > + { > + /* DSP0270: 8.6: the Host IPv[4|6] Address */ > + printf("%s\t\t%s Address: %s\n", prefix, > + (addrtype == 0x1 ? "IPv4" : "IPv6"), It just occurred to me that you should call dmi_address_type() instead of open-coding it here. Even better would be to call it once above and store the result in a local const char * variable. It avoids duplication and also avoids presenting an unknown address format as "IPv6". > + dmi_address_decode(&rdata[18], buf, addrtype)); > + > + /* DSP0270: 8.6: Prints the Host IPv[4|6] Mask */ > + printf("%s\t\t%s Mask: %s\n", prefix, > + (addrtype == 0x1 ? "IPv4" : "IPv6"), Ditto. > + dmi_address_decode(&rdata[34], buf, addrtype)); > + } > + > + /* DSP0270: 8.6: Get the Redfish Service IP Discovery Type */ > + assign_val = rdata[50]; > + /* Redfish Service IP Discovery type mirrors Host IP Assignment type */ > + printf("%s\t\tRedfish Service IP Discovery Type: %s\n", prefix, > + dmi_protocol_assignment_type(assign_val)); > + > + /* DSP0270: 8.6: Get the Redfish Service IP Address Format */ > + addrtype = rdata[51]; > + printf("%s\t\tRedfish Service IP Address Format: %s\n", prefix, > + dmi_address_type(addrtype)); > + > + if (assign_val == 0x1 || assign_val == 0x3) > + { > + u16 port; > + u32 vlan; > + > + /* DSP0270: 8.6: Prints the Redfish IPv[4|6] Service Address */ > + printf("%s\t\t%s Redfish Service Address: %s\n", prefix, > + (addrtype == 0x1 ? "IPv4" : "IPv6"), Ditto. > + dmi_address_decode(&rdata[52], buf, addrtype)); > + > + /* DSP0270: 8.6: Prints the Redfish IPv[4|6] Service Mask */ > + printf("%s\t\t%s Redfish Service Mask: %s\n", prefix, > + (addrtype == 0x1 ? "IPv4" : "IPv6"), Ditto. > + dmi_address_decode(&rdata[68], buf, addrtype)); > + > + /* DSP0270: 8.6: Redfish vlan and port info */ > + port = WORD(&rdata[84]); > + vlan = DWORD(&rdata[86]); > + printf("%s\t\tRedfish Service Port: %hu\n", prefix, port); > + printf("%s\t\tRedfish Service Vlan: %u\n", prefix, vlan); > + } > + > + /* DSP0270: 8.6: Redfish host length and name */ > + hlen = rdata[90]; > + > + /* > + * DSP0270: 8.6: The the length of the host string + 91 (the minimum Extra "the". > + * size of a protocol record) cannot exceeed the record length Spelling: exceed. > + * (rec[0x1] Missing closing parenthesis. > + */ > + hname = (const char *)&rdata[91]; > + if (hlen + 91 > rlen) { Curly brace goes on separate line for consistency. > + hname = out_of_spec; > + hlen = strlen(out_of_spec); > + } > + printf("%s\t\tRedfish Service Hostname: %*s\n", prefix, hlen, hname); > +} > + > +/* > + * DSP0270: 8.3: Device type ennumeration > + */ > +static const char *dmi_parse_device_type(u8 type) > +{ > + const char *devname[] = { > + "USB", /* 0x2 */ > + "PCI/PCIe", /* 0x3 */ > + }; > + > + if (type <= 0x3 && type >= 0x2) Traditionally tested the other way around. Result is the same of course, but I like consistency. > + return devname[type-2]; Spaces around "-", and please use hexadecimal for consistency. > + if (type >= 0x80) > + return "OEM"; > + return out_of_spec; > +} > + > +static void dmi_parse_controller_structure(const struct dmi_header *h, > + const char *prefix) > +{ > + int i; > + u8 *data = h->data; > + /* Host interface type */ > + u8 type; > + /* Host Interface specific data length */ > + u8 len; > + u8 count; > + u32 total_read; > + > + /* > + * Minimum length of this struct is 0xB bytes > + */ > + if (h->length < 0xB) > + return; > + > + /* > + * Also need to ensure that the interface specific data length > + * plus the size of the structure to that point don't exceed > + * the defined length of the structure, or we will overrun its > + * bounds > + */ > + len = data[0x5]; > + total_read = len + 0x6; > + > + if (total_read > h->length) > + return; > + > + type = data[0x4]; > + printf("%sHost Interface Type: %s\n", prefix, > + dmi_management_controller_host_type(type)); > + > + /* > + * The following decodes are code for Network interface host types only > + * As defined in DSP0270 > + */ > + if (type != 0x40) > + return; > + > + if (len != 0) > + { > + /* DSP0270: 8.3 Table 2: Device Type */ > + type = data[0x6]; > + > + printf("%sDevice Type: %s\n", prefix, > dmi_parse_device_type(type)); > + if (type == 0x2 && len >= 6) > + { > + /* USB Device Type - need at least 6 bytes */ Something I missed during the previous review... len is the whole length of the Interface Specific data. This data is composed of the device type (1 byte) followed by the type-specific device descriptors (variable number of bytes). Your len checks do NOT take into account the 1 byte used for the device type. So all your checks are off by one. About USB, technically we only need to check for len >= 1 + 4 bytes, as we are ignoring the serial number now (which by the way is wrong in the sample you shared with me). > + u8 *usbdata = &data[0x7]; > + /* USB Device Descriptor: idVendor */ > + printf("%s\tidVendor: 0x%04x\n", prefix, > WORD(&usbdata[0x0])); > + /* USB Device Descriptor: idProduct */ > + printf("%s\tidProduct: 0x%04x\n", prefix, > WORD(&usbdata[0x2])); > + /* > + * USB Serial number is here, but its useless, don't > + * bother decoding it > + */ > + } > + else if (type == 0x3 && len >= 8) > + { > + /* PCI Device Type - Need at least 8 bytes */ > + u8 *pcidata = &data[0x7]; > + /* PCI Device Descriptor: VendorID */ > + printf("%s\tVendorID: 0x%04x\n", prefix, > WORD(&pcidata[0x0])); > + /* PCI Device Descriptor: DeviceID */ > + printf("%s\tDeviceID: 0x%04x\n", prefix, > WORD(&pcidata[0x2])); > + /* PCI Device Descriptor: PCI SubvendorID */ > + printf("%s\tSubVendorID: 0x%04x\n", prefix, > WORD(&pcidata[0x4])); > + /* PCI Device Descriptor: PCI SubdeviceID */ > + printf("%s\tSubDeviceID: 0x%04x\n", prefix, > WORD(&pcidata[0x6])); > + } > + else if ((type == 0x4) && (len >= 4)) Parentheses not needed. > + { > + /* OEM Device Type - Need at least 4 bytes */ > + u8 *oemdata = (u8 *)&data[0x7]; Useless cast. > + /* OEM Device Descriptor: IANA */ > + printf("%s\tVendor ID: 0x%02x:0x%02x:0x%02x:0x%02x\n", > + prefix, oemdata[0x0], oemdata[0x1], > + oemdata[0x2], oemdata[0x3]); > + } > + /* Don't mess with unknown types for now */ > + } > + > + /* > + * DSP0270: 8.2 and 8.5: Protcol record count and protocol records Spelling: Protocol. > + * Move to the Protocol Count Record. "Protocol Count Record" is ambiguous. I think you really mean Protocol Count. > + * Protocol record count starts len bytes + 0x6, where len represents > + * the interface specific data length from above > + */ > + data = &data[0x6+len]; 0x6+len is already known as total_read, so data += total_read; would work fine (or data = &data[total_read] if you really prefer that syntax). This would also clear the need for the explanation above, in my opinion. > + > + /* > + * we've validated up to 0x6 + len bytes, but we need to validate Start with capital W for consistency. > + * the next byte below, the count value > + */ > + total_read++; > + if (total_read > h->length) > + { > + printf("%s\tWARN: Protocol rec length %d > total length %d\n", Please spell out "Warning" and "record" in full. Abbreviations do not help understanding. > + prefix, total_read, h->length); > + return; > + } > + > + /* Get the protocol records count */ > + count = data[0x0]; > + if (count) > + { > + u8 *rec = &data[0x1]; > + for (i = 0; i < count; i++) > + { > + /* > + * Need to ensure that this record doesn't overrun > + * the total length of the type 42 struct. Note the +2 > + * is added for the two leading bytes of a protocol > + * record representing the type and length bytes Missing dot. > + */ > + if (total_read + rec[1] + 2 > h->length) Too many spaces before > > + { > + printf("%s\tWARN: Protocol rec length %d > > total length %d\n", Same comment as above. > + prefix, len, h->length); I think you mean "total_read + rec[1] + 2" instead of "len" here. Maybe it would make sense to move the "total_read += rec[1] + 2;" from the end of this block to before the test, so that you only do the operation once instead of 3 times? > + return; > + } > + > + dmi_parse_protocol_record(prefix, rec); > + > + /* > + * DSP0270: 8.6 > + * Each record is rec[1] bytes long, starting at the > + * data byte immediately following the length field Dot. > + * That means we need to add the byte for the rec id, > + * the byte for the length field, and the value of the > + * length field itself Dot. > + */ > + total_read += rec[1] + 2; > + rec += rec[1] + 2; > + } > + } > +} > + > /* > * Main > */ > @@ -4582,22 +4939,27 @@ static void dmi_decode(const struct dmi_header *h, > u16 ver) > > case 42: /* 7.43 Management Controller Host Interface */ > printf("Management Controller Host Interface\n"); > - if (h->length < 0x05) break; > - printf("\tInterface Type: %s\n", > - > dmi_management_controller_host_type(data[0x04])); > - /* > - * There you have a type-dependent, variable-length > - * part in the middle of the structure, with no > - * length specifier, so no easy way to decode the > - * common, final part of the structure. What a pity. > - */ > - if (h->length < 0x09) break; > - if (data[0x04] == 0xF0) /* OEM */ > + if (ver < 0x0302) > { > - printf("\tVendor ID: 0x%02X%02X%02X%02X\n", > - data[0x05], data[0x06], data[0x07], > - data[0x08]); > + if (h->length < 0x05) break; > + printf("\tInterface Type: %s\n", > + > dmi_management_controller_host_type(data[0x04])); > + /* > + * There you have a type-dependent, > variable-length > + * part in the middle of the structure, with no > + * length specifier, so no easy way to decode > the > + * common, final part of the structure. What a > pity. > + */ > + if (h->length < 0x09) break; > + if (data[0x04] == 0xF0) /* OEM */ > + { > + printf("\tVendor ID: > 0x%02X%02X%02X%02X\n", > + data[0x05], data[0x06], > data[0x07], > + data[0x08]); > + } > } > + else > + dmi_parse_controller_structure(h, "\t"); > break; > > case 43: /* 7.44 TPM Device */ -- Jean Delvare SUSE L3 Support _______________________________________________ https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
