Skip to content

The 'mc guid' command processes GUIDs incorrectly #25

Closed
AlexanderAmelkin opened this issue Jul 27, 2018 · 11 comments
Closed

The 'mc guid' command processes GUIDs incorrectly #25

AlexanderAmelkin opened this issue Jul 27, 2018 · 11 comments
Assignees
Labels
bug Something isn't working improvement Improves an existing feature

Comments

@AlexanderAmelkin
Copy link
Contributor

AlexanderAmelkin commented Jul 27, 2018

Commit 4787c3d (also see SourceForge discussion) back from 2008 looks to have been incorrect.
It reversed the byte order of the time low field of GUID justifying it by SMBIOS specification and overlooking the fact that IPMI specification directly states that IPMI-compliant devices must send bytes for that field in the order opposite to RFC4122 (and based on it SMBIOS):

Here is an excerpt from IPMI v2.0 specification rev 1.1:

20.8 Get Device GUID Command

...
Note that the individual fields within the GUID are stored least-significant byte first,
and in the order illustrated in the following table. This is the reverse of convention
described in [RFC4122] where GUID bytes are transmitted in ‘network order’
(most-significant byte first) starting with the time low field.

Table 20-, GUID Format

GUID Byte Field MSbyte
1 node
... ...
6 node MSbyte
... ... ...
7 clock seq and reserved
8 clock seq and reserved MSbyte
9 time high and version
10 time high and version MSbyte
11 time mid
12 time mid MSbyte
13 time low
... ... ...
16 time low MSbyte

However for the past 10 years since that commit nobody seems to have complained about that, which indicates to me that either:

  1. Nobody needs this GUID functionality at all
  2. All BMC software implementations on the market bear a symmetric bug and return GUID according to SMBIOS and violating IPMI.

Comments are welcome.

@vmauery
Copy link
Member

vmauery commented Jul 27, 2018

The openbmc implementation agrees with the IPMI standard. Those are implemented with bona-fide GUIDs though. For some other implementations of BMC firmware I have seen, it is simply 16 random bytes, so order doesn't make sense unless you are trying to actually interpret the values in the rfc4122 format.

I would lean toward making ipmitool correct. Urge the BMC implementations to fix themselves.

@AlexanderAmelkin
Copy link
Contributor Author

Ok, I now see that I wasn't completely correct in my analysis of the problem.
The mentioned commit 4787c3d indeed reversed the order of bytes but did it for the node field, not for time_low. It did though justify it by SMBIOS (see https://sourceforge.net/p/ipmitool/patches/27/), ignoring the IPMI spec. So my conclusion regarding the wrong byte order is correct.

As for the time_low field, the commit removed the unconditional byte swapping that was indeed unneeded for x86 as both that architecture and and IPMI use little-endian encoding. However, for compatibility with big-endian targets, there must be a conditional byte-swapping done instead (using ipmi32toh()), and I'm going to add it in context of this issue. However, even after I add it, it won't work for big-endian targets due to multiple other places breaking compatibility. There is issue #26 for that.

@AlexanderAmelkin
Copy link
Contributor Author

Comparison of GUID/UUID representation in IPMI/SMBIOS/RFC4122:

Textual GUID: {00112233-4455-6677-8899-AABBCCDDEEFF}

Standard First <- Byte order -> Last
RFC4122 00 11 22 33 44 55 66 77 88 99 AA BB CC DD EE FF
SMBIOS 33 22 11 00 55 44 77 66 88 99 AA BB CC DD EE FF
IPMI FF EE DD CC BB AA 99 88 77 66 55 44 33 22 11 00

This brings up yet another question of the same nature ("does anyone use it at all?").
ipmitool currently defines struct ipmi_guid_t in an SMBIOS way.
That is, byte 0..3 is time_low, while for IPMI bytes 0..5 must be node.

@vmauery, does the node part sent by OpenBMC gets displayed as such by ipmitool ?

@AlexanderAmelkin
Copy link
Contributor Author

I'm leaning towards adding an argument to ipmitool mc guid command to specify the UUID encoding. This is obviously against the IPMI specification but it looks like if anyone is using this functionality, is using it with broken BMCs, and since it is not always possible to get an updated version of a BMC for particular piece of hardware, I think that having an option to change GUID encoding may come in handy.

What I haven't decided on yet is whether I should force default behavior to IPMI-compliant (to give users a hint that there is something wrong with the BMCs) or to leave it as is and only add an option to switch to IPMI-compliant behavior (to make it more transparent for the current installations).

At least some quite recent versions of AMI MegaRAC implement GUIDs in such a way that they are 'properly' displayed by ipmitool, which is, as I've shown above, is a broken way as of now.

@AlexanderAmelkin
Copy link
Contributor Author

@bparthiban, I'd like to invite you and any of your engineers to join this discussion.

@AlexanderAmelkin
Copy link
Contributor Author

Another interesting observation is that ipmiutil also doesn't follow IPMI specification, but unlike ipmitool that does it the SMBIOS way, the former decodes GUIDs as if they were encoded according to RFC4122.

@AlexanderAmelkin
Copy link
Contributor Author

AlexanderAmelkin commented Aug 1, 2018

I'm going to add an optional argument to ipmitool mc guid command. The argument will let the user specify how to decode the GUID. By default I'm planning to have the current SMBIOS-way decoding. It will also be available with smbios option. Other options will be: ipmi, rfc4122 and dump. The latter will not process bytes in any way and will just dump them in hex from first to last as received from the BMC.

@AlexanderAmelkin
Copy link
Contributor Author

Timestamp decoding is also done incorrectly in ipmitool as of 1.8.18:

  • Timestamp is shown for all GUID versions, even for not time-based
  • Only the time_low field is used and it is used as if it was seconds since UNIX Epoch,
    while in fact it is in 100ns intervals since adoption of the Gregorian calendar and requires also time_mid and time_hi_and_version fields.

@AlexanderAmelkin
Copy link
Contributor Author

I've decided to implement automatic detection of GUID encoding as the default behavior.
See the ipmitool.1 man update for details.

For Lenovo IMM the output now looks like this:

$ src/ipmitool mc guid
System GUID   : bb5f86ae-7d72-11e6-8a80-02e0ec40a0d1
GUID Encoding : SMBIOS (WARNING: IPMI Specification violation!)
GUID Version  : Time-based
Timestamp     : 09/18/2016 10:37:20

@AlexanderAmelkin
Copy link
Contributor Author

@devenrao, I thought you may be interested as it addresses the ticket you raised back on SourceForge.

@AlexanderAmelkin
Copy link
Contributor Author

Ok, I guess this is now mature enough. So now it will be merged into master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working improvement Improves an existing feature
Projects
None yet
Development

No branches or pull requests

2 participants