linux-kernel - Re: [PATCH v3] oprofile, x86: Convert memory allocation to static array

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: <20110801221625.GF11795@erda.amd.com>
Date:	Tue, 2 Aug 2011 00:16:25 +0200
From:	Robert Richter <robert.richter@....com>
To:	Andi Kleen <andi@...stfloor.org>
CC:	Maarten Lankhorst <m.b.lankhorst@...il.com>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	"x86@...nel.org" <x86@...nel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] oprofile, x86: Convert memory allocation to static
 array

On 01.08.11 17:41:30, Andi Kleen wrote:
> On Mon, Aug 01, 2011 at 11:31:42PM +0200, Robert Richter wrote:
> > On 01.08.11 11:08:59, Maarten Lankhorst wrote:
> > > On -rt, allocators don't work from atomic context any more,
> > > and the maximum size of the array is known at compile time.
> > > 
> > > Signed-off-by: Maarten Lankhorst <m.b.lankhorst@...il.com>
> > 
> > Applied to oprofile/core. Thanks, Maarten.
> 
> And what happens when the CPU reports more than 32 counters?
> You have a silent buffer overflow then.

>From the layout of IA32_PERF_GLOBAL_CTRL MSR it seems to be limited to
32. Anyway, if there might be cpus out soon with more than 32 counters
we can either extend the array to 256 or limit the counters used to 32.

> Besides I bet there are other cases like this all over the tree.
> 
> And the whole thing is tasteless. 
> 
> Nacked-by: Andi Kleen <ak@...ux.intel.com>

This dynamic allocation is causing trouble from the beginning. Using a
static array removes a lot of NULL pointer checks:

 arch/x86/oprofile/op_model_ppro.c |   24 +-----------------------

The previous implementation silently dropped counters during setup on
failure. Also we have had many bugs caused by this dynamic allocation
(I counted 3 fixes). This all isn't it worth and you did not yet
explain the concerns you have (besides the potentially buffer overflow
which is fixable and probably of academic nature).

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

--
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/