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

[Neil Horman]

On Wed, Aug 08, 2018 at 09:54:22AM +0200, Jean Delvare wrote:
> 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.
> 
Thats fine, get_aligned does the same thing that your macro does regardless.

> > > (...)
> > > 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
> $ 
> 
Hmm, thats odd.  It was in linux/unaligned.h as part of the kernel headers, and
I thought that got exported as part of the user space set, but it doesn't seem
to be, so perhaps I'm the one misremembering.

> > (...)
> > 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.
> 
That could well be, the system is an engineering sample.  Regardless, do you
think we need to add some sanity check for SMBIOS version 3 where that length
isn't specified, and refuse to decode it for the ambiguity it provides?

> 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.
> 
Yeah, its just broken.

> 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.
> 
No, its pre-production, and pretty old.  I'm getting a newer system this week,
and should be spec compliant.  If not, i'm bringing it up with the manufacturer.

Best
Neil

> -- 
> Jean Delvare
> SUSE L3 Support
>