linux-kernel - Re: [RFCv2] string: Use faster alternatives when constant arguments are used

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-prev] [day] [month] [year] [list]

Date:   Tue, 2 Apr 2019 10:17:28 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Sultan Alsawaf <sultan@...neltoast.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        Nathan Chancellor <natechancellor@...il.com>
Subject: Re: [RFCv2] string: Use faster alternatives when constant arguments
 are used

On 02/04/2019 01.55, Sultan Alsawaf wrote:
> On Mon, Apr 01, 2019 at 10:43:13PM +0200, Rasmus Villemoes wrote:

>> No. First, these are concerns for all arches. Second, if you can find
>> some particular place where string parsing/matching is in any way
>> performance relevant and not just done once during driver init or
>> whatnot, maybe the maintainers of that file would take a patch
>> hand-optimizing some strcmps to memcmps, or, depending on what the code
>> does, perhaps replacing the whole *cmp logic with a custom hash table.
> 
> FWIW, I didn't have a specific place in the kernel in mind that heavily relied
> on str* operations and needed optimization. I just thought it could be a "free"
> optimization, but apparently not.

Well, it wouldn't be completely free; at the very least, .text size
would usually increase, and for the majority of sites that are
execute-at-most-once-per-full-moon that's actually a small overall
pessimization. But the point is moot, I think.

>> But a patch implicitly and silently touching thousands of lines of code,
>> without an analysis of why none of the above is a problem for any of
>> those lines, for any .config, arch, compiler version? No.
> 
> Well, I thought it could be proven to be correct regardless of the context,
> which would imply that making such a global change touching thousands lof lines
> of code would be safe. But sadly it isn't.
> 
> This does bring into question a greater problem though: usage of memcmp
> throughout the kernel where the size provided is not the size of the smaller
> container being compared. This usage of memcmp (which is quite easy to find all
> across the kernel) could run into the unaligned memory access across a page
> boundary that you gave as an example, no?

Yes, as I said, I think you may have identified some actual bugs. In
some cases, the runtime string is known to live in an array of
sufficient size, so we would at most be reading uninitialized bytes (but
not making decisions based on them) - that could cause a sanitizer
warning, but otherwise harmless (stuff like this is a rather common
source of false positives with valgrind). In other cases, there's no
such guarantee. It really needs to be decided on a case-by-case basis.
And if in doubt, I'd change the code to use strcmp(), strncmp(),
strstarts() as appropriate - that's _much_ more readable, and avoids
having to worry, and would usually avoid duplicating the literal string
(which is error-prone when the code gets copy-pasted to handle a new
option).

Rasmus