linux-kernel - Re: [PATCH AUTOSEL for 4.15 142/189] firmware: dmi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Apr 2018 14:48:03 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     Parag Warudkar <parag.warudkar@...il.com>
Cc:     Sasha Levin <Alexander.Levin@...rosoft.com>,
        stable@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH AUTOSEL for 4.15 142/189] firmware: dmi_scan: Fix
 handling of empty DMI strings

Hi Parag,

On Sun, 8 Apr 2018 21:21:19 -0400, Parag Warudkar wrote:
> On Sun, Apr 8, 2018 at 8:18 PM, Sasha Levin <Alexander.Levin@...rosoft.com>
> wrote:
> 
> > From: Jean Delvare <jdelvare@...e.de>
> >
> > [ Upstream commit a7770ae194569e96a93c48aceb304edded9cc648 ]
> >
> > [snip]
> >  
> 
> 
> > * Strings starting with 8 spaces are all considered empty, even if
> >   non-space characters follow (sounds like a weird thing to do, but
> >   I have actually seen occurrences of this in DMI tables before.)
> >  
> 
> Unless I am misreading the patch we will now allocate memory for
> len(spaces)+len(string) for this case.

Correct.

> Have you seen BIOSes with lots of <space>string occurrences?

A fair number, yes, but most of them were thankfully not in strings we
are saving. Also note that most of them only have 1 or 2 leading
spaces, not 8+, so this commit makes no difference for them.

> If so what's the point of allocating memory for the leading spaces?

Presented like that, it sounds like we are silly. But look at things
the other way around: what's the point of storing these leading spaces
in the DMI table in the first place? Whenever this happens, the
hardware manufacturer is to blame, not us.

As much as possible, we should stick to the specification and assume
the other party does as well. Stripping the leading spaces would be
trivial, but hiding their mistakes does not help hardware manufacturers
in the long run anyway. If they can't see that it's wrong, it's harder
for them to fix it.

> IOW, what happens if we strip leading space before allocating?

At boot time, we save a few bytes of memory, on the affected systems
only. No code cost, but possibly some confusion as to why we strip
leading spaces (which are rare) but not trailing spaces (which in my
experience are more frequent.)

At run time, the exact matching of the affected DMI strings would be
less error-prone (although strings having leading spaces are likely to
also have trailing spaces, annihilating the benefits.) Non-exact
matching would be marginally faster, again only for the affected
systems.

As a conclusion, it's doable, but the benefit is very small and limited
to a few broken systems, and it has the downside of not discouraging
low-quality tables, so my position is that it's not worth it and not
desirable.

-- 
Jean Delvare
SUSE L3 Support