linux-kernel - Re: [PATCH v6 3/4] printk: Userspace format indexing support

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: <bdbcf438-50d3-7429-36e8-c077e31bc695@rasmusvillemoes.dk>
Date:   Wed, 19 May 2021 08:59:06 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Chris Down <chris@...isdown.name>
Cc:     linux-kernel@...r.kernel.org, Petr Mladek <pmladek@...e.com>,
        Jessica Yu <jeyu@...nel.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        John Ogness <john.ogness@...utronix.de>,
        Steven Rostedt <rostedt@...dmis.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Kees Cook <keescook@...omium.org>, kernel-team@...com
Subject: Re: [PATCH v6 3/4] printk: Userspace format indexing support

On 18/05/2021 18.00, Andy Shevchenko wrote:
> On Tue, May 18, 2021 at 03:07:44PM +0100, Chris Down wrote:
>> Andy Shevchenko writes:
> 
> ...
> 
>>>> +	return mod ? mod->name : "vmlinux";
>>>
>>> First of all, you have several occurrences of the "vmlinux" literal.
>>> Second, can't you get it from somewhere else? Is it even guaranteed that the
>>> name is always the same?
>>
>> Hmm, I don't know if it's guaranteed, but we already have similar logic in
>> (as one example) livepatch, which seems to suggest it's not obviously wrong:
>>
>>     % grep -R '"vmlinux"' kernel/livepatch/
>>     kernel/livepatch/core.c:                       sympos, name, objname ? objname : "vmlinux");
>>     kernel/livepatch/core.c:        bool sec_vmlinux = !strcmp(sec_objname, "vmlinux");
>>     kernel/livepatch/core.c:                sym_vmlinux = !strcmp(sym_objname, "vmlinux");
>>     kernel/livepatch/core.c:        if (strcmp(objname ? objname : "vmlinux", sec_objname))
>>     kernel/livepatch/core.c:        name = klp_is_module(obj) ? obj->name : "vmlinux";
>>     kernel/livepatch/core.c:                                klp_is_module(obj) ? obj->name : "vmlinux");
>>     kernel/livepatch/core.c:                                klp_is_module(obj) ? obj->name : "vmlinux");
>>     kernel/livepatch/core.c:        if (!strcmp(mod->name, "vmlinux")) {
>>
>> Is there another name or method you'd prefer? :-)
>>
>> As for the literals, are you saying that you prefer that it's symbolised as
>> a macro or static char, or do you know of an API where this kind of name can
>> be canonically accessed?
> 
> I have heard that modern GCC (at least) can utilize same constant literals in a
> single compilation unit, so it won't be duplicated.

Yes, except it's not gcc but ld, string deduplication happens across
compilation units, and "modern" isn't required, SHF_STRINGS and
SHF_MERGE have been part of the ELF spec for decades, with support in
binutils landing around 2001-04-13 AFAICT.

IOW, don't uglify the code by introducing macros or const char[]
objects. Using string literals is just fine.

> 
>>>> +static int __init pi_init(void)
> 
>>> No __exit? (There is a corresponding call for exit)
>>
>> Hmm, can't printk only be built in to the kernel, so it can't be unloaded?
>> At least it looks that way from Kconfig. Maybe I'm missing something and
>> there's some other way that might be invoked?
> 
> While it's true, it may help in these cases:
>  1) getting things done in a clean way

Huh?

>  2) finding bugs during boot cycle

What bugs would code that doesn't get executed find?

>  3) (possibly) making better debugging in virtual environments

How?

>  4) (also possibly) clean up something which shouldn't be seen by the next
>     (unsecure) kernel, like kexec.

Tearing down a few debugfs files wouldn't touch a lot of memory, the
printk format strings are very unlikely to be sensitive, and I highly
doubt __exit code is kept around and run at kexec time anyway.

IOW, please do not bloat the kernel image with __exit code in things
which cannot be built modular.

Rasmus