Message-ID: <22cb7c93-1160-b181-5702-1d7bfcee5b18@rasmusvillemoes.dk>
Date:   Fri, 2 Nov 2018 21:09:21 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Kees Cook <keescook@...omium.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 0/7] runtime format string checking

[trimming cc list]

On 2018-11-01 23:57, Kees Cook wrote:
> On Thu, Nov 1, 2018 at 3:06 PM, Rasmus Villemoes
> <linux@...musvillemoes.dk> wrote:
>> referring to an anonymous object in .rodata; one gets code gen like
>>
>> +:      31 c0                   xor    %eax,%eax
>> +:      48 b8 61 63 70 69 2d    movabs $0x7570632d69706361,%rax # "acpi-cpu"
>> +:      63 70 75
>> +:      c7 44 24 0b 66 72 65    movl   $0x71657266,0xb(%rsp) # "freq"
>> +:      71
>> +:      c6 44 24 0f 00          movb   $0x0,0xf(%rsp) "\0"
>> +:      48 89 44 24 03          mov    %rax,0x3(%rsp)
> 
> Oh that is nasty. Ugh. I hate the "const but not really ha ha" optimizations. :(
> 
>> It's not the-end-of-the-world-horrible, but it's better avoided,
>> especially for patches that are not supposed to change anything. And
>> longer strings would of course produce even more gunk like the above.
>> A better fix which also silences -Wformat-security is to declare the
>> variable itself const, i.e.
>>
>> const char *const drv = "acpi-cpufreq".
> 
> Yes, that would be much better. Seems like we could do a really easy
> Coccinelle script to fix all of those?
> 
> @@
> identifier VAR;
> expression STRING;
> @@
> 
> - const char VAR[]
> + const char * const VAR
>   = STRING;
> 
> yields:
>  517 files changed, 890 insertions(+), 891 deletions(-)
> 
> Worth doing at the end of -rc2?

That's a bit too naive. At the very least, you must exclude static
stuff, i.e. restrict to actual auto variables. Otherwise you're making
things worse (a "static const char []" just occupies some space in
.rodata, a "static const char * const" occupies the same space for the
anonymous literal, plus space for a pointer). Furthermore, you must
ensure that nobody does sizeof() on VAR. With a trivial extension of
your script to exclude the "static const char" places, I get

 97 files changed, 151 insertions(+), 151 deletions(-)

but that includes a number of places at file level where VAR actually
has external linkage. Which is most likely not intentional, but those
places would need different fixes. Actually, a lot of them are of the
'version = "1.2 (Feb 3 1995)"' kind which are utterly useless, so should
simply be removed (possibly left in a comment).

There's not a whole lot of difference between

const char *const foo = "read";

and

static const char foo[] = "read";

The former allows the linker to share "read" with other identical
literals (or reuse the tail of "thread"), but the actual strings in
these cases are likely to be unique and not suffixes of others. The
latter is probably more readable (at least it's more common), and in
some cases one can slap on an __initconst, making the memory footprint
go away entirely. And when sizeof() is used,

So I think it's better to take the above 151 cases and do them in small
batches.

>>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/format-security&id=b7dcfc8f48caaafcc423e5793f7ef61b9bb5c458
>>> This one covers cases where the pointer is pointing to a const string,
>>> so really there's no sense in injecting the "%s", but I was collecting
>>> them to make real ones stand out.
>>
>> I don't agree. [...]
> 
> Okay, then I'll forward this to akpm maybe?

Yes, if all they do is replace f(..., s) by f(..., "%s", s) that should
never hurt. Maybe check if there's a ..._puts() variant that can be used
instead, e.g. seq_puts().

Rasmus

Powered by blists - more mailing lists