linux-kernel - Re: [RFC][PATCH 2/3] perf: Fix u16 overflows

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: <20150914093142.GA24362@gmail.com>
Date:	Mon, 14 Sep 2015 11:31:42 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	linux-kernel@...r.kernel.org, vincent.weaver@...ne.edu,
	acme@...nel.org, eranian@...gle.com, jolsa@...hat.com,
	alexander.shishkin@...ux.intel.com
Subject: Re: [RFC][PATCH 2/3] perf: Fix u16 overflows


* Peter Zijlstra <peterz@...radead.org> wrote:

> On Sat, Sep 12, 2015 at 10:11:20AM +0200, Ingo Molnar wrote:
> > 
> > * Peter Zijlstra <peterz@...radead.org> wrote:
> > 
> > > Vince reported that its possible to overflow the various size fields
> > > and get weird stuff if you stick too many events in a group.
> > > 
> > > Put a lid on this by requiring the fixed record size not exceed 16k.
> > > This is still a fair amount of events (silly amount really) and leaves
> > > plenty room for callchains and stack dwarves while also avoiding
> > > overflowing the u16 variables.
> > 
> > Does this leave a natural ABI extension route here, in case in the future it 
> > becomes a problem? We should take aside a value to mean 'larger record' or such?
> 
> So this all is a result of:
> 
> struct perf_event_header {
> 	__u32   type;
> 	__u16   misc;
> 	__u16   size;
> };
> 
> And we've not even done the 'sensible' thing of interpreting @size as
> @size*8 :/ That is, because entries must be u64 aligned, the lower 3
> bits of @size will always be 0.
> 
> Now there are of course ways we can 'grow' if we really have to. One
> would be to set aside a MISC bit to indicate we should do that *8 thing,
> which would allow up to 512 Kb records.
>
> 	__u32   type;
> 	__u16   misc;
> 	__u16   size;
> };

Makes sense!

Btw., it appears that header->type is using only about 4 bits at the moment, out 
of 32.

So future extensions could split it into two and use the other __u16 half as more 
header->misc fields, should we run out of them (we seem to be close to). Such 
user-space requesting extended misc bits would have to parse the new format 
records.

> That said, 64k is already quite a lot of data, and I'm not sure we want to have 
> records bigger than that. Certainly not for samples, copying that much data on 
> an interrupt is just not going to be fast.
> 
> And I'm not sure there's a sensible use-case for having this many events in a 
> group (and there's good reasons not to do it).
> 
> In any case, the patch only pokes at internal stuff, the ABI isn't affected 
> beyond refusing to create humongous groups.

Fair enough!

Thanks,

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