linux-kernel - Re: [PATCH V2 1/2] string: Add stracpy and stracpy

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

Message-ID: <dab1b433-93c0-09ab-cceb-3db91b6ef353@rasmusvillemoes.dk>
Date:   Wed, 24 Jul 2019 08:53:52 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Joe Perches <joe@...ches.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org
Cc:     Jonathan Corbet <corbet@....net>, Stephen Kitt <steve@....org>,
        Kees Cook <keescook@...omium.org>,
        Nitin Gote <nitin.r.gote@...el.com>, jannh@...gle.com,
        kernel-hardening@...ts.openwall.com,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

On 23/07/2019 17.39, Joe Perches wrote:
> On Tue, 2019-07-23 at 16:37 +0200, Rasmus Villemoes wrote:
>> On 23/07/2019 15.51, Joe Perches wrote:
>>>
>>> These mechanisms verify that the dest argument is an array of
>>> char or other compatible types like u8 or s8 or equivalent.
>> Sorry, but "compatible types" has a very specific meaning in C, so
>> please don't use that word.
> 
> I think you are being overly pedantic here but
> what wording do you actually suggest?

I'd just not support anything other than char[], but if you want,
perhaps say "related types", or some other informal description.

>>  And yes, the kernel disables -Wpointer-sign,
>> so passing an u8* or s8* when strscpy() expects a char* is silently
>> accepted, but does such code exist?
> 
> u8 definitely, s8 I'm not sure.

Example (i.e. of someone passing an u8* as destination to some string
copy/formatting function)? I believe you, I'd just like to see the context.

> I don't find via grep a use of s8 foo[] = "bar";
> or "signed char foo[] = "bar";
> 
> I don't think it bad to allow it.

Your patch.

>> count is just as bad as size in terms of "the expression src might
>> contain that identifier". But there's actually no reason to even declare
>> a local variable, just use ARRAY_SIZE() directly as the third argument
>> to strscpy().
> 
> I don't care about that myself.
> It's a macro local identifier and shadowing in a macro
> is common.  I'm not a big fan of useless underscores.

shadowing is not the problem. The identifier "count" appearing in one of
the "dest" or "src" expressions is. For something that's supposed to
help eliminate bugs, such a hidden footgun seems to be a silly thing to
include. No need for some hideous triple-underscore variable, just make
the whole thing

BUILD_BUG_ON(!__same_type())
strscpy(dst, src, ARRAY_SIZE(dst))

Rasmus