Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks

[Jean Delvare]

Hi Neil,

On Tue, 7 Aug 2018 12:00:13 -0400, Neil Horman wrote:
> On Tue, Aug 07, 2018 at 03:14:22PM +0200, Jean Delvare wrote:
> > Is it a "recent" addition? I am certain that it used to crash, back in
> > 2003 - I did not implement ALIGNMENT_WORKAROUND just for fun. The git
> > log also has evidence that gcc used to complain about unaligned
> > structure member copies. If you can point me to the kernel piece of
> > code which deals with that, I'm interested.  
> Look to arch/ia64/kernel/unaligned.c
> 
> Git history has it as far back as 2005, but thats the import to the current
> tree, so I'm guessing it goes back a bit farther than that.
> 
> > That being said, I am
> > afraid that it must be slower to do things wrong and let the kernel
> > cope with the mess, than do the right thing directly?
>
> It is slower of course, because you have to take a trap and fix up the access
> (and then the kernel logs it).  Its not something you want to happen, I'm just
> saying its no longer fatal.  That said, the right thing to use is the
> get_unaligned macro to avoid the misaligned access in the first place.

OK. Then maybe I remember wrong and the workaround in dmidecode was not
to prevent an actual crash but to prevent flooding the kernel log with
warnings. Either way, the workaround is there to stay, in one form or
another, for the reason you just explained.

> > (...)
> > get_unaligned() doesn't seem to be a standard user-space function.
>
> Its standard for linux, but no, its not part of any specification.

Where is it? I can't find it.

$ gcc -Wall -W -Wno-unused test-unaligned.c -o test-unaligned
test-unaligned.c: In function 'main':
test-unaligned.c:13:2: warning: implicit declaration of function 
'get_unaligned' [-Wimplicit-function-declaration]
  j = get_unaligned(i);
  ^
/tmp/ccJIuhvD.o: In function `main':
test-unaligned.c:(.text+0x69): undefined reference to `get_unaligned'
collect2: error: ld returned 1 exit status
$ grep -r get_unaligned /usr/include
$ 

> (...)
> I keep looking at the protocol count field, which the spec lists as being at
> 0x6+n bytes, where n is defined as the interface specific data length from
> earlier in the structure, but I keep finding it on this system at 0x7+n, so
> either they put the count 1 too many bytes in, or the spec if vague as to
> weather the interface specific data length includes the length byte itself.

Your dump says it complies with SMBIOS 3.0.0, not 3.2.0. The type 42
structure definition was different in 3.0.0, specifically there was no
length byte at offset 0x05, instead the interface type specific data
was immediately at 0x05. The length of that data had to be inferred from
the data itself, which was pretty fragile (and thus was later changed.)

I was thinking that maybe that could explain that off-by-one you are
seeing. Unfortunately my theory doesn't really fly. If your data was
actually following the old definition, then it means that the device
type would have value 0x06, which is not specified by DSP0270. If the
type is at offset 0x07, value is 0x02, which is USB, which is specified
and presumably what you expect.

Unfortunately (again), that means that 0x06 (at offset 0x05) is the
variable length, which is not sufficient to hold the device type (1
byte, USB) and the device type specific data (at least 6 bytes for a
USB device, more if you include the bString). Very fishy. Maybe the
manufacturer started with SMBIOS 3.0.0, then noticed the change in
3.2.0 and tried to adjust, but messed up.

To make things worse, it seems that the vendor added another length
byte which is not in the specification:

Handle 0x2E30, DMI type 42, 170 bytes
        Header and Data:
                2A AA 30 2E 40 06 02 00 10 40 B3 04 01 9C 04 9A
                                                    ^^ ^^ ^^ ^^
                16 76 7C B6 46 1F 11 E7 BA 4F C7 78 8C 80 94 22
                01 01 A9 FE 5F 78 00 00 00 00 00 00 00 00 00 00
                00 00 FF FF 00 00 00 00 00 00 00 00 00 00 00 00
                00 00 01 01 A9 FE 5F 76 00 00 00 00 00 00 00 00
                00 00 00 00 FF FF 00 00 00 00 00 00 00 00 00 00
                00 00 00 00 BB 01 00 00 00 00 09 58 43 43 2D 37
                58 31 32 2D 00 00 00 00 00 00 00 00 00 00 00 00
                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
                00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
                00 00 00 00 00 00 00 00 00 00

Assuming that 01 is the number of protocol records, and 04 is the type
of the first (and only) protocol (Redfish), 9A would be the length of
that protocol (which matches the specification and is consistent with
the overall DMI structure length). 9C would apparently be the total
length of ALL the protocols. It is consistent with the length of the DMI
structure, the problem is that it doesn't follow the SMBIOS
specification, neither version 3.2.0 nor previous versions.

So no matter how you look at it, your data is invalid. Is it a
production system? You'll have to talk to the manufacturer and/or look
for a BIOS update. They must follow the SMBIOS specification they claim
to implement. And if they are serious about that Redfish thing then
they really should implement SMBIOS 3.2.0.

-- 
Jean Delvare
SUSE L3 Support