linux-kernel - Re: [PATCH] firmware: declare __{start,end}_builtin

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: <4ae97e84-5a9a-c862-1324-82586cd44913@oracle.com>
Date:   Fri, 14 Oct 2016 08:25:46 +0200
From:   Vegard Nossum <vegard.nossum@...cle.com>
To:     Jiri Slaby <jslaby@...e.cz>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Vegard Nossum <vegard.nossum@...il.com>
Cc:     Ming Lei <ming.lei@...onical.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Steven Rostedt <srostedt@...hat.com>
Subject: Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers

On 10/14/2016 07:52 AM, Jiri Slaby wrote:
> On 06/26/2016, 07:17 PM, Linus Torvalds wrote:
>> On Sun, Jun 26, 2016 at 2:24 AM, Vegard Nossum <vegard.nossum@...il.com> wrote:
>>>
>>> This is the best I could come up with: assuming gcc is not allowed to
>>> reason about what's inside the asm(), this is the only way I could
>>> think of to lose the array information without incurring unnecessary
>>> overheads. It should also be relatively safe as there is no way to
>>> accidentally use the underlying arrays without explicitly declaring
>>> them.
>>
>> Ugh. I worry about the other places where we do things like this,
>> depending on the linker just assigning the addresses and us being able
>> to compare them.
>>
>> If there is a compiler option to disable this optimization, I would
>> almost prefer that.. Because we really do have a whole slew of these
>> things.
>
> Any update on this? Couple months later and I still hit this.

I didn't find any compiler flags or anything that would let us get by
with the current code.

I have a branch fixing a bunch of these (many of them in tracing code,
like you wrote) using my old approach (declaring the arrays with a
macro) but it looks so ugly I got discouraged from submitting any of it.
I also don't have a great way to actually test a lot of the fixes.

Could something like this work?

#define DECLARE_EXTERNAL_ARRAY(type, name) \
	extern type __##name##_start;
	static inline type name##_start(void) \
	{ \
		type tmp = __##name##_start; \
		OPTIMIZER_HIDE_VAR(tmp); \
		return tmp; \
	} \
	static inline type name##_end(void) \
	{ \
		type tmp = __##name##_end; \
		OPTIMIZER_HIDE_VAR(tmp); \
		return tmp; \
	} \
	static inline size_t name##_size(void) \
	{ \
		return name##_end() - name##_start(); \
	}

#define ext_for_each(var, name) \
	for (var = name##_start(); var != name##_end(); var++)

and then for the firmware case you would do

DECLARE_EXTERNAL_ARRAY(struct builtin_fw, builtin_fw);

struct builtin_fw *fw;
ext_for_each(fw, builtin_fw)
	...;

You'd also have to adjust the names in the linker script accordingly.

The points here being:

1) DECLARE_*() follows the kernel convention (you get what you expect,
more or less)

2) the real variables defined in the linker script are hidden behind
some generated names so you don't use them by accident

3) no need to sprinkle your code with OPTIMIZER_HIDE_VAR() or anything
else, but you do need to use function calls to access the variables
(e.g. firmware_start() and firmware_end()).

What do you think?


Vegard