linux-kernel - Re: [PATCH 1/3 RFC] ARM: mvebu: at least warn on kzalloc failure

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: <20190414232443.GB1215@osadl.at>
Date:   Mon, 15 Apr 2019 01:24:43 +0200
From:   Nicholas Mc Guire <der.herr@...r.at>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Nicholas Mc Guire <hofrat@...dl.org>,
        Jason Cooper <jason@...edaemon.net>,
        Andrew Lunn <andrew@...n.ch>,
        Gregory Clement <gregory.clement@...tlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3 RFC] ARM: mvebu: at least warn on kzalloc failure

On Sun, Apr 14, 2019 at 06:26:02PM +0100, Russell King - ARM Linux admin wrote:
> On Sun, Apr 14, 2019 at 06:49:49AM +0200, Nicholas Mc Guire wrote:
> > Although it is very unlikely that the allocation during init would
> > fail any such failure should point to the original cause rather
> > than waiting for a null-pointer dereference to splat.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@...dl.org>
> > ---
> > 
> > Problem located with experimental coccinelle script
> > 
> > While this will not really help much - but kzalloc failures should not
> > go unhandled. 
> 
> Sorry, no, not like this.
>

ok - well I wsa not sure about it either - it just seems wrong
to leave a possible allocation failure without any response.

The issue of generating excessive outout make sense - so will fix
it up to a pr_err() and resend.

thx!
hofrat
 
> With this patch, rather than getting an oops and a stacktrace which
> people can capture and email, we instead end up getting a warning
> line, a stack trace, followed by an oops containing another stack
> trace.
> 
> We _already_ have problems getting people to send us kernel message
> debug information without editing out what they deem to be "unnecessary
> verbage", like all those numbers and function names that comprise a
> stack trace.  We don't need yet more of that stuff, especially when it
> is redundant.
> 
> So, I think throwing WARN_ON() at this case is way too excessive, and
> will only have a detrimental effect on the reports we receive - and
> that is extremely important.
> 
> IMHO, A better solution would be to just print a warning, rather than
> causing the kernel to print several kB of needless messages.
> 
> 	if (!new_compat)
> 		pr_err("new_compat allocation failure in %s()\n",
> 		       __func__);
> 
> > 
> > Patch was compile-tested: mvebu_v7_defconfig (implies MACH_MVEBU_ANY=y)
> > (with some unrelated sparse warnings about missing syscalls)
> > 
> > Patch is against 5.1-rc4 (localversion-next is 20190412)
> > 
> >  arch/arm/mach-mvebu/board-v7.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
> > index 0b10acd..37f8cb6 100644
> > --- a/arch/arm/mach-mvebu/board-v7.c
> > +++ b/arch/arm/mach-mvebu/board-v7.c
> > @@ -128,6 +128,7 @@ static void __init i2c_quirk(void)
> >  		struct property *new_compat;
> >  
> >  		new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
> > +		WARN_ON(!new_compat);
> >  
> >  		new_compat->name = kstrdup("compatible", GFP_KERNEL);
> >  		new_compat->length = sizeof("marvell,mv78230-a0-i2c");
> > -- 
> > 2.1.4
> > 
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up