[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [dmidecode] [PATCH v4] update dmidecode to parse Modern Management C
|
From: |
Jean Delvare |
|
Subject: |
Re: [dmidecode] [PATCH v4] update dmidecode to parse Modern Management Controller blocks |
|
Date: |
Fri, 7 Sep 2018 12:09:26 +0200 |
Hi Neil,
On Mon, 13 Aug 2018 12:02:41 -0400, Neil Horman wrote:
> Starting with version 0x300 the SMBIOS specification defined in more
0x300 is an internal representation. SMBIOS specification versions
should be referred to using their string representation as used on the
DMTF web site, like 3.0.0.
By the way, from our previous discussion, the first version for which
type 42 records can actually be decoded is 3.2.0, not 3.0.0. You have
adjusted it in the code already, and I think the patch description
should be updated accordingly, to avoid confusion.
> 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
Missing final dot.
We still don't have any real-world implementation to test your code, do
we? The whole thing is way too tricky to commit it without testing and
hope that it will work. If you crafted a modified DMI table dump file
for the purpose of testing your code, please save me the work and share
it with me. If not, I'll do it.
Don't be afraid by the number of my comments below. Most of them are
minor things and overall this version of your patch is much better than
the previous one.
>
> 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
> ---
> dmidecode.c | 360 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 345 insertions(+), 15 deletions(-)
>
> diff --git a/dmidecode.c b/dmidecode.c
> index 76faed9..f7f1a9e 100644
> --- a/dmidecode.c
> +++ b/dmidecode.c
> @@ -56,6 +56,10 @@
> * - "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
> + * - "DMTF SMBIOS Specification" (DMTF DSP0134)
> + *
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.1.1.pdf
> */
DSP0134 is the main SMBIOS specification, and it is already referenced
earlier in the header comment. No need to add it again in the
"Additional references" section. Likewise, there is no need to mention
DSP0134 later in the code, as it is implied (there is even an explicit
comment stating that).
>
> #include <stdio.h>
> @@ -63,6 +67,7 @@
> #include <strings.h>
> #include <stdlib.h>
> #include <unistd.h>
> +#include <arpa/inet.h>
>
> #ifdef __FreeBSD__
> #include <errno.h>
> @@ -3379,6 +3384,7 @@ static void dmi_additional_info(const struct dmi_header
> *h, const char *prefix)
> static const char *dmi_management_controller_host_type(u8 code)
> {
> /* DMTF DSP0239 (MCTP) version 1.1.0 */
> + /* DMTF DSP0134 (SMBIOS version 3.1.1) */
> static const char *type[] = {
> "KCS: Keyboard Controller Style", /* 0x02 */
> "8250 UART Register Compatible",
> @@ -3391,7 +3397,11 @@ static const char
> *dmi_management_controller_host_type(u8 code)
>
> if (code >= 0x02 && code <= 0x08)
> return type[code - 0x02];
> - if (code == 0xF0)
> + else if (code <= 0x3F)
> + return "MCTP";
> + else if (code == 0x40)
> + return "Network";
> + else if (code == 0xF0)
The "else" statements are not needed after a "return" statement.
> return "OEM";
> return out_of_spec;
> }
> @@ -3447,6 +3457,321 @@ static void dmi_tpm_characteristics(u64 code, const
> char *prefix)
> prefix, characteristics[i - 2]);
> }
>
> +/*
> + * DSP0134: 7.43.2: Protocol Record Types
> + */
> +static const char *dmi_protocol_record_type(u8 type)
All this should go right after dmi_management_controller_host_type, to
avoid scattering type 42 helpers before and after type 43 helpers.
> +{
> + const char *protocoltypes[] = {
I'd drop the trailing "types".
> + "Reserved",
> + "Reserved",
> + "IPMI",
> + "MCTP",
> + "Redfish over IP",
It is good practice to add a comment after the first and last lines of
an enumeration with the corresponding number. It makes it easier to
check for code correctness.
> + };
> +
> + if (type == 0xF0)
> + return "OEM";
> + else if (type <= 0x4)
> + return protocoltypes[type];
"else" not needed. There is also no technical reason to check for OEM
type first, so you could use the spec order.
> + return out_of_spec;
> +}
> +
> +/*
> + * DSP0270: 8.6: Protocol IP Assignment types
> + */
> +static const char *dmi_protocol_assignment_type(u8 type)
> +{
> + const char *assigntype[] = {
I'd drop the trailing "type" and go with "assignment", more readable.
> + "Unknown",
> + "Static",
> + "DHCP",
> + "AutoConf",
> + "Host Selected",
Same comment as above.
> + };
> +
> + if (type <= 0x4)
> + return assigntype[type];
> + return out_of_spec;
> +}
> +
> +/*
> + * DSP0270: 8.6: Protocol IP Address type
> + */
> +static const char *dmi_address_type(u8 type)
> +{
> + const char *addressformat[] = {
> + "Unknown",
> + "IPv4",
> + "IPv6",
Same comment as above.
> + };
> +
> + if (type <= 0x2)
> + return addressformat[type];
> + return out_of_spec;
> +}
> +
> +static void dmi_parse_protocol_record(const char *prefix, u8 *rec)
A reference to DSP0270 8.5 would make sense.
> +{
> + u8 rid;
> + u8 *rdata;
> + u8 buf[64];
> + u8 aval;
> + u8 addrtype;
> + u8 hlen;
> + const char *addr;
> + int af;
> +
> + /* DSP0270: 8.5: Protocol Identifier*/
Space before end of comment.
> + rid = rec[0x0];
> + /* DSP0270: 8.5: Protocol Record Data */
> + rdata = &rec[0x2];
You are not making use of rec[0x1] in this function. You should ensure
that the record data length is big enough to contain everything you are
going to decode below. Failing to do so could result in out-of-bound
reads. In practice, you must ensure that rec[0x1] is at least 91, if
I read the specification right. Then you will need another check for
the variable size part of the record (Redfish Service Hostname), see
below.
> +
> + /*
> + * DSP0134: 7.43.2: If the protocol type is OEM defined,
> + * Map it down to the OEM index in our prtocol types
No uppercase M to Map. Spelling: protocol.
> + * array. Otherwise, if its larger than the last
> + * defined type on the list, index it as unknnown
Spelling: unknown. Missing final dot.
To be honest, I can't really make sense of this comment anyway. It is
basically paraphrasing the code in dmi_protocol_record_type().
> + */
> + printf("%s\tProtocol ID: %02x (%s)\n", prefix, rid,
> dmi_protocol_record_type(rid));
This long line is easy to split.
> +
> + /*
> + * Don't decode anything other than redfish for now
Redfish (uppercase R).
> + * Note 0x4 is Redfish over IP in DSP0134: 7.43.2
> + * and DSP0270: 8.5
> + */
> + if (rid != 0x4)
> + return;
> +
> + /*
> + * DSP0270: 8.6: Redfish Over IP Service UUID
> + * Note: ver is hardcoded to 0x311 here just for
> + * convienience. It could get passed from the smbios
Spelling: convenience. SMBIOS (uppercase).
> + * header, but thats alot of passing of pointers just
Spelling: that's a lot.
> + * to get that info, and the only thing its used for is
Spelling: it is.
> + * to determine the endianess of the field. since we only
Spelling: Since (capital).
> + * do this parsing on versions of smbios after 3.1.1, and the
Spelling: SMBIOS (uppercase).
> + * endianess of the field is alway little after version 2.6.0
Spelling: always.
> + * we can just pick a sufficiently recent version here
Missing final dot.
> + */
> + 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
> + */
> + aval = rdata[16];
Strange variable name, I have no idea what it means.
> +
Blank line not needed.
> + printf("%s\t\tHost IP Assignment Type: %s\n", prefix,
> + dmi_protocol_assignment_type(aval));
Second line should be aligned like that:
printf("%s\t\tHost IP Assignment Type: %s\n", prefix,
dmi_protocol_assignment_type(aval));
And same several times below. Makes things consistent while avoiding
long lines.
> +
> + /*
> + * DSP0270: 8.6: Redfish Over IP Host Address format
> + */
Fits on a single line, so you could make the comment more compact.
> + addrtype = (u8)rdata[17];
Useless cast.
> +
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 (aval == 0x1 || aval == 0x3)
> + {
You should also ensure that addrtype is 0x1 or 0x2. If it has another
value then the code below will format random data as if it was an IPv6
address.
> + /* DSP0270: 8.6: the Host IPV[4|6] Address */
Lowercase v.
> + af = addrtype == 0x1 ? AF_INET : AF_INET6;
> + addr = inet_ntop(af, &rdata[18], (char *)buf, 64);
> + addr = addr ? addr: out_of_spec;
Space before ":".
> + printf("%s\t\t%s Address: %s\n", prefix,
> + (addrtype == 0x1 ? "IPv4" : "IPv6"), addr);
Second line needs at least once more space for proper alignment (or
just use a tab as is done in the rest of the file).
> +
> + /* DSP0270: 8.6: Prints the Host IPV[4|6] Mask */
Lowercase v.
> + addr = inet_ntop(af, &rdata[34], (char *)buf, 64);
> + addr = addr ? addr : out_of_spec;
> + printf("%s\t\t%s Mask: %s\n", prefix,
> + (addrtype == 0x1 ? "IPv4" : "IPv6"), addr);
Alignment (see above).
> + }
> +
> + /* DSP0270: 8.6: Get the Redfish Service IP Discovery Type */
> + aval = rdata[50];
> +
Blank line not needed.
> + /* 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(aval));
> +
> + /* DSP0270: 8.6: Get the Redfish Service IP Address Format */
> + addrtype = rdata[51];
> +
Blank line not needed.
> + printf("%s\t\tRedfish Service IP Address Format: %s\n", prefix,
> +
> dmi_address_type(addrtype));
If you really like blank lines, this is where I would put one ;-)
> + if (aval == 0x1 || aval == 0x3)
All the comments of previous block apply to this one as well so I'm not
repeating them.
> + {
> + u16 port;
> + u32 vlan;
> +
> + af = addrtype == 0x1 ? AF_INET : AF_INET6;
> + addr = inet_ntop(af, &rdata[52], (char *)buf, 64);
> + addr = addr ? addr : out_of_spec; /* Ensure the string is
> non-null */
> +
> + /* DSP0270: 8.6: Prints the Redfish IPV[4|6]Service Address */
Missing space after "]".
> + printf("%s\t\t%s Redfish Service Address: %s\n", prefix,
> + (addrtype == 0x1 ? "IPv4" : "IPv6"), addr);
> +
> + /* DSP0270: 8.6: Prints the Redfish IPV[4|6] Service Mask */
> + addr = inet_ntop(af, &rdata[68], (char *)buf, 64);
> + addr = addr ? addr : out_of_spec;
> + printf("%s\t\t%s Redfish Service Mask: %s\n", prefix,
> + (addrtype == 0x1 ? "IPv4" : "IPv6"), addr);
> +
> + /* DSP0270: 8.6: Redfish vlan and port info */
> + port = WORD(&rdata[84]);
> + vlan = DWORD(&rdata[86]);
> + printf("%s\t\tRedfish Service Port: %u\n", prefix, port);
%hu
> + printf("%s\t\tRedfish Service Vlan: %u\n", prefix, vlan);
> + }
> +
> + /* DSP0270: 8.6: Redfish host length and name */
> + hlen = rdata[90];
> + printf("%s\t\tRedfish Service Hostname: %*s\n", prefix, hlen,
> &rdata[91]);
This is where you must have a second length check. 91 + hlen can't
exceed rec[1] (exact check may need to take a few offsets into account)
otherwise you are reading beyond the record.
> +}
> +
> +/*
> + * DSP0270: 8.3: Device type ennumeration
> + */
> +static const char *dmi_parse_device_type(u8 type)
> +{
> + const char *devname[] = {
> + "Unknown",
> + "Unknown",
Technically these are not defined in the datasheet. If they are used,
you should return out_of_spec, not "Unknown". Just start your array
with "USB" and apply a "- 2" offset below, as is done in other places.
> + "USB",
> + "PCI/PCIe",
> + };
Please add comments with values for first and last entry.
> +
> + if (type >= 0x80)
> + return "OEM";
> +
> + if (type <= 0x3)
> + return devname[type];
Again there is no reason to not perform the checks in the datasheet
order.
> + 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 = data[0x4];
You can't do that before checking for the structure length.
> + /* Host Interface specific data length */
> + u8 len = data[0x5];
Ditto.
> + u8 count;
> +
> + /*
> + * DSP0134: 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
> + */
> + if ((len + 0x7) > h->length)
Parentheses not needed. I think it's 0x06 + len, not 0x07.
> + return;
> +
> + 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))
Parentheses not needed (same below, twice).
> + {
> + /* USB Device Type - need at least 6 bytes */
> + 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]));
> + printf("%s\tSerialNumber:\n", prefix);
> + /* USB Device Descriptor: bDescriptor */
> + printf("%s\t\tbDescriptor: 0x%02x\n", prefix,
> usbdata[0x5]);
Makes no sense to me. Either you know how to decode the Serial Number
properly, and you do. Or you don't know, and you just skip the whole
thing. The Descriptor byte, alone, is useless.
According to
https://www.beyondlogic.org/usbnutshell/usb5.shtml
I think that bDescriptorType should always be 0x3. bString should
contain the serial number in "Unicode format", but that doesn't give us
the encoding. Further research suggests UTF-16LE, how convenient. Not
sure if we want to go through the pain of decoding UTF-16LE in
dmidecode... Depends how portable it can be, I suppose.
> + }
> + else if ((type == 0x3) && (len >= 8))
> + {
> + /* PCI Device Type - Need at least 8 bytes*/
Space before end of comment.
> + 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))
> + {
> + /* OEM Device Type - Need at least 4 bytes */
> + u32 *oemdata = (u32 *)&data[0x7];
This is not a safe thing to do. Remember the discussions we had about
unaligned memory access on exotic architectures.
> + /* OEM Device Descriptor: IANA */
> + printf("%s\t Vendor ID: 0x%08x\n", prefix,
> DWORD(&oemdata[0x0]));
Stray space before "Vendor". The datasheet says the data is MSB first,
so using DWORD is not possible here (DWORD assumes LSB first). That's
why I was printing it byte-by-byte originally.
> + }
> + /* Don't mess with unknown types for now */
> + }
> +
> + /*
> + * DSP0270: 8.2 and 8.5: Protcol record count and protocol records
> + * Move to the Protocol Count Record
> + * data[0x6] points to the start of the interface specific
> + * data, and len is the length of that region
> + */
> + data = &data[0x6+len];
> + len = len+0x6;
I find this construct confusing. Until now, len was actually
representing a length. Now it is really representing an offset into the
structure, basically telling what the "data" pointer (and later on the
"rec" pointer ?) is aiming at. I'd rather have a separate variable with
an explicit name for that purpose.
And apparently I'm not the only one confused, as the comment above does
NOT match the code ;-)
> +
> + /* Get the protocol records count */
> + count = (u8)data[0x0];
Useless cast. You also need to check that this byte is within
h->length. At this point you are walking in a minefield really, and
will have to do that check for every byte you read, see below.
> + 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
> + */
> + if (len + rec[1] > h->length)
Worth printing a warning in my opinion. Being silent on things we don't
know how to decode or interpret is fine, but in this case it would mean
inconsistent data. We can help BIOS authors by being vocal about it.
You must first ensure that rec[1] itself is within h->length.
And I believe that len + rec[1] isn't the right quantity to check,
because rec[1] is only the variable part of the record. You are missing
the 2 bytes of the fixed part before that variable part. Additionally,
len is where data it pointing, NOT where rec is pointing. There's a one
byte difference between these two pointers originally.
The whole thing is horribly tricky, I have to say, so it's easy to get
it wrong :-( We need good variable names and good comments (which you
tried to do and I appreciate that).
> + return;
> + len += rec[1];
If rec below needs a "+2", then I bet that len needs the same "+2". You
may want to increment both side by side to make it more obvious that
they are related.
> +
> + 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
> + * 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
> + */
> + rec += rec[1] + 2;
> + }
> + }
> +}
> +
> /*
> * Main
> */
> @@ -4582,22 +4907,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