Re: [dmidecode] Bug: dmidecode -ut4 segfaults

[Jean Delvare]

Hi Jerry,

On Mon, 18 Jan 2021 01:26:10 -0700, Jerry Hoemann wrote:
> On current dmidecode I build, I am seeing core dumps (SEGFAULT) when running:
> 
>       ./dmidecode -ut4 --from-dump _file_
> 
> 
> For most, but not all, of my saved HPE dmi binary dumps.
> 
> e.g. the DL385G10.dmi I sent you earlier segfaults.
> 
> The segfault is coming from dmidecode.c:5239
> 
>                 display = ((opt.type == NULL || opt.type[h.type])
>                         && (opt.handle == ~0U || opt.handle == h.handle)
>                         && !((opt.flags & FLAG_QUIET) && (h.type == 126 || 
> h.type == 127))
>                         && !opt.string);
> 
> When run under gdb, it appears that the "opt" structure was over
> written by ascii characters.
> 
> I bisected the submits and the issue first appears in:
> 
>       da06888 dmidecode: Use the print helpers in dump mode too
> 
> 
> I haven't had a chance to dig into submit(da06888) so have not RCA'd
> the issue.
> 
> Can you reproduce in your environment?

Thanks for reporting. Yes, I can reproduce it on my systems and with
various dumps too. I confirm that reverting commit da06888 fixes the
problem.

Valrgind complains about the following:

==13097== Invalid read of size 1
==13097==    at 0x40B0B6: dmi_table_decode (dmidecode.c:5239)
==13097==    by 0x40B5A7: dmi_table (dmidecode.c:5397)
==13097==    by 0x40BAE8: smbios_decode (dmidecode.c:5522)
==13097==    by 0x40C02D: main (dmidecode.c:5714)

which matches the lines of code you quoted above.

I can't really see how this specific line can crash, as the only
dereferencing done in that statement, as far as I can see, is
opt.type[h.type]. Given that h.type is an 8-bit entity and opt.type is
a 256-byte array, that should not possibly crash even if the values are
wrong.

That being said, there is one very obvious bug in the output of
dmidecode -u before the crash happens. The binary form of the string
part of the structure is messed up:

Handle 0x0001, DMI type 4, 42 bytes
        Header and Data:
                04 2A 01 00 04 03 CD 02 A9 06 03 00 FF FB EB BF
                01 89 64 00 28 0A 28 0A 41 21 03 00 04 00 05 00
                03 05 06 02 02 04 04 00 CD 00
        Strings:
                49 6E 74 65 6C 28 52 29 20 43 6F 72 65 28 54 4D
                49 6E 74 65 6C 28 52 29 20 43 6F 72 65 28 54 4D29 20 69 35 2D 
33 33 32 30 4D 20 43 50 55 20 40
                49 6E 74 65 6C 28 52 29 20 43 6F 72 65 28 54 4D29 20 69 35 2D 
33 33 32 30 4D 20 43 50 55 20 4020 32 2E 36 30 47 48 7A 00
                Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz

As you can see, for the second and third lines, the hexadecimal values
are appended to the values from the previous line, instead of replacing
them. As these values are printed into a 48-byte buffer before being
displayes, we are obviously over-runing the buffer and thus trashing the
stack. A trashed stack can lead to pretty much anything bad, including
what we are seeing. Fixing that should solve the problem.

What this means is that I really need to extend my test suite to cover
the -u option. Such a regression should really have been caught before
being committed :(

Working on a fix.

-- 
Jean Delvare
SUSE L3 Support