linux-kernel - Re: [PATCH] ASoC: cs35l32: avoid uninitialized variable access

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]

Message-ID: <20160104165220.GL19062@n2100.arm.linux.org.uk>
Date:	Mon, 4 Jan 2016 16:52:20 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
	Brian Austin <brian.austin@...rus.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Paul Handrigan <Paul.Handrigan@...rus.com>,
	linux-kernel@...r.kernel.org, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] ASoC: cs35l32: avoid uninitialized variable access

On Mon, Jan 04, 2016 at 05:41:05PM +0100, Arnd Bergmann wrote:
> That reminds of a different problem that has been bugging me for a
> while: We frequently have a pattern like
> 
> #ifdef CONFIG_FOO
> static int function(void)
> {
> 	...
> }
> #endif
> 
> struct operations = {
> 	...
> #ifdef CONFIG_FOO
> 	.function = function;
> #endif
> 	...
> };
> 
> Except that people constantly get it wrong, e.g. by using the
> wrong ifdef, forgetting one of the two ifdefs, or by leaving
> unused static functions that only get called indirectly from the
> other one that is built conditionally.

We already have a solution to that.  __maybe_unused against the
function, and use the correct #ifdef in the structure initialiser.
We just need reviewers to be better at picking that up.

> We could add a macro like
> 
> #define COND_PTR(config, ptr) (IS_ENABLED(config) ? (ptr) : NULL)
> 
> and then let the compiler figure out that "function" is unused even
> without an explicit __maybe_unused annotation.  The function above
> can be simplied to
> 
> static inline struct device_node *dev_of_node(struct device *dev)
> {
> 	return COND_PTR(CONFIG_OF, dev->of_node);
> }
> 
> with that, which is another benefit.

You're just inventing another way for people to get it wrong though.
Instead of having mismatched #ifdefs, we can now have a mismatched
#ifdef around the function and the COND_PTR config - and people will
add #ifdef's because they won't realise they don't need them.

You're reliant on reviewers to spotting the pattern, and suggesting
using COND_PTR() without #ifdefs around the function.  It's the same
problem with spotting the existing pattern and suggesting dropping
the #ifdef around the function and annotating the function with
__maybe_unused.

So, I don't see the benefit.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/