[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [dmidecode] [PATCH v5] update dmidecode to parse Modern Management C
|
From: |
Jean Delvare |
|
Subject: |
Re: [dmidecode] [PATCH v5] update dmidecode to parse Modern Management Controller blocks |
|
Date: |
Mon, 10 Sep 2018 18:55:17 +0200 |
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 <address@hidden>
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
>
> ---
> Change Notes:
> V1->V2) Updated string formatting to print matching number of bytes
> for unsigned shorts (address@hidden)
>
> Adjusted string format for bDescriptor (address@hidden)
>
> Prefaced PCI id's with 0x (address@hidden)
> 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