From: Colin Ian King <[email protected]>
The sa_flags field is not being initialized, so a garbage value is
being passed to sigaction. Initialize it to zero.
Signed-off-by: Colin Ian King <[email protected]>
---
tools/perf/arch/x86/tests/rdpmc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/perf/arch/x86/tests/rdpmc.c b/tools/perf/arch/x86/tests/rdpmc.c
index 7bb0d13..7945462 100644
--- a/tools/perf/arch/x86/tests/rdpmc.c
+++ b/tools/perf/arch/x86/tests/rdpmc.c
@@ -103,6 +103,7 @@ static int __test__rdpmc(void)
sigfillset(&sa.sa_mask);
sa.sa_sigaction = segfault_handler;
+ sa.sa_flags = 0;
sigaction(SIGSEGV, &sa, NULL);
fd = sys_perf_event_open(&attr, 0, -1, -1,
--
2.7.0
On Wed, Mar 02, 2016 at 12:55:22PM +0000, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> The sa_flags field is not being initialized, so a garbage value is
> being passed to sigaction. Initialize it to zero.
>
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> tools/perf/arch/x86/tests/rdpmc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/arch/x86/tests/rdpmc.c b/tools/perf/arch/x86/tests/rdpmc.c
> index 7bb0d13..7945462 100644
> --- a/tools/perf/arch/x86/tests/rdpmc.c
> +++ b/tools/perf/arch/x86/tests/rdpmc.c
> @@ -103,6 +103,7 @@ static int __test__rdpmc(void)
>
> sigfillset(&sa.sa_mask);
> sa.sa_sigaction = segfault_handler;
> + sa.sa_flags = 0;
Would not something like:
sa = (struct sigaction){
.sa_sigaction = segfault_handler,
};
sigfillset(&sa.sa_mask);
Be better?
> sigaction(SIGSEGV, &sa, NULL);
>
> fd = sys_perf_event_open(&attr, 0, -1, -1,
> --
> 2.7.0
>
Em Wed, Mar 02, 2016 at 12:55:22PM +0000, Colin King escreveu:
> From: Colin Ian King <[email protected]>
>
> The sa_flags field is not being initialized, so a garbage value is
> being passed to sigaction. Initialize it to zero.
>
> Signed-off-by: Colin Ian King <[email protected]>
Thanks, applied.
- Arnaldo
Em Wed, Mar 02, 2016 at 01:59:01PM +0100, Peter Zijlstra escreveu:
> On Wed, Mar 02, 2016 at 12:55:22PM +0000, Colin King wrote:
> > From: Colin Ian King <[email protected]>
> >
> > The sa_flags field is not being initialized, so a garbage value is
> > being passed to sigaction. Initialize it to zero.
> >
> > Signed-off-by: Colin Ian King <[email protected]>
> > ---
> > tools/perf/arch/x86/tests/rdpmc.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/perf/arch/x86/tests/rdpmc.c b/tools/perf/arch/x86/tests/rdpmc.c
> > index 7bb0d13..7945462 100644
> > --- a/tools/perf/arch/x86/tests/rdpmc.c
> > +++ b/tools/perf/arch/x86/tests/rdpmc.c
> > @@ -103,6 +103,7 @@ static int __test__rdpmc(void)
> >
> > sigfillset(&sa.sa_mask);
> > sa.sa_sigaction = segfault_handler;
> > + sa.sa_flags = 0;
>
> Would not something like:
>
> sa = (struct sigaction){
> .sa_sigaction = segfault_handler,
> };
> sigfillset(&sa.sa_mask);
>
> Be better?
I thought about that, but isn't that set in stone? This would be a 4
liner, while his is a one' :-)
- Arnaldo
On Wed, Mar 02, 2016 at 10:03:50AM -0300, Arnaldo Carvalho de Melo wrote:
> > Would not something like:
> >
> > sa = (struct sigaction){
> > .sa_sigaction = segfault_handler,
> > };
> > sigfillset(&sa.sa_mask);
> >
> > Be better?
>
> I thought about that, but isn't that set in stone? This would be a 4
> liner, while his is a one' :-)
Dunno, you're right that its rather unlikely struct sigaction is going
to grow another member, but I like the above pattern better in general,
makes it harder to end up with uninitalized bits.
When performance matters the above pattern isn't ideal, but that should
not be a concern here.
Em Wed, Mar 02, 2016 at 02:21:27PM +0100, Peter Zijlstra escreveu:
> On Wed, Mar 02, 2016 at 10:03:50AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Would not something like:
> > >
> > > sa = (struct sigaction){
> > > .sa_sigaction = segfault_handler,
> > > };
> > > sigfillset(&sa.sa_mask);
> > >
> > > Be better?
> >
> > I thought about that, but isn't that set in stone? This would be a 4
> > liner, while his is a one' :-)
>
> Dunno, you're right that its rather unlikely struct sigaction is going
> to grow another member, but I like the above pattern better in general,
> makes it harder to end up with uninitalized bits.
>
> When performance matters the above pattern isn't ideal, but that should
> not be a concern here.
Right, I also always use :
struct foo bar = {
.baz = 1,
.name = "whatever",
};
Even more compact than using that cast. But didn't bother changing in
this case.
- Arnaldo
* Arnaldo Carvalho de Melo <[email protected]> wrote:
> Em Wed, Mar 02, 2016 at 02:21:27PM +0100, Peter Zijlstra escreveu:
> > On Wed, Mar 02, 2016 at 10:03:50AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Would not something like:
> > > >
> > > > sa = (struct sigaction){
> > > > .sa_sigaction = segfault_handler,
> > > > };
> > > > sigfillset(&sa.sa_mask);
> > > >
> > > > Be better?
> > >
> > > I thought about that, but isn't that set in stone? This would be a 4
> > > liner, while his is a one' :-)
> >
> > Dunno, you're right that its rather unlikely struct sigaction is going
> > to grow another member, but I like the above pattern better in general,
> > makes it harder to end up with uninitalized bits.
> >
> > When performance matters the above pattern isn't ideal, but that should
> > not be a concern here.
>
> Right, I also always use :
>
>
> struct foo bar = {
> .baz = 1,
> .name = "whatever",
> };
>
> Even more compact than using that cast. But didn't bother changing in
> this case.
So the source of the bug was:
struct sigaction sa;
...
sigfillset(&sa.sa_mask);
sa.sa_sigaction = segfault_handler;
sigaction(SIGSEGV, &sa, NULL);
... which uninitialized sa.sa_flags field GCC merrily accepted as proper C code,
despite us turning on essentially _all_ GCC warnings for the perf build that exist
under the sun:
gcc -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k \
-Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs \
-Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls \
-Wshadow -Wstrict-aliasing=3 -Wstrict-prototypes -Wswitch-default -Wswitch-enum \
-Wundef -Wwrite-strings -Wformat \
-Werror -O6 -fno-omit-frame-pointer -ggdb3 -funwind-tables -Wall -Wextra -std=gnu99 -fstack-protector-all -D_FORTIFY_SOURCE=2
This is a _trivial_ uninitialized variable bug, yet GCC never warned about it.
Why?
People build perf with a wide range of GCC versions, from old ones to trunk. I
cannot believe it that none of those GCC versions warned about this trivial
looking bug!
And yes, I know that unitialized structures on the stack are valid C code, yet
it's one of the most fragile aspects of C and it was the source of countless
security holes in the past...
Thanks,
Ingo
On 03/03/16 12:19, Ingo Molnar wrote:
>
> * Arnaldo Carvalho de Melo <[email protected]> wrote:
>
>> Em Wed, Mar 02, 2016 at 02:21:27PM +0100, Peter Zijlstra escreveu:
>>> On Wed, Mar 02, 2016 at 10:03:50AM -0300, Arnaldo Carvalho de Melo wrote:
>>>>> Would not something like:
>>>>>
>>>>> sa = (struct sigaction){
>>>>> .sa_sigaction = segfault_handler,
>>>>> };
>>>>> sigfillset(&sa.sa_mask);
>>>>>
>>>>> Be better?
>>>>
>>>> I thought about that, but isn't that set in stone? This would be a 4
>>>> liner, while his is a one' :-)
>>>
>>> Dunno, you're right that its rather unlikely struct sigaction is going
>>> to grow another member, but I like the above pattern better in general,
>>> makes it harder to end up with uninitalized bits.
>>>
>>> When performance matters the above pattern isn't ideal, but that should
>>> not be a concern here.
>>
>> Right, I also always use :
>>
>>
>> struct foo bar = {
>> .baz = 1,
>> .name = "whatever",
>> };
>>
>> Even more compact than using that cast. But didn't bother changing in
>> this case.
>
> So the source of the bug was:
>
> struct sigaction sa;
>
> ...
>
> sigfillset(&sa.sa_mask);
> sa.sa_sigaction = segfault_handler;
> sigaction(SIGSEGV, &sa, NULL);
>
> ... which uninitialized sa.sa_flags field GCC merrily accepted as proper C code,
> despite us turning on essentially _all_ GCC warnings for the perf build that exist
> under the sun:
>
> gcc -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k \
> -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs \
> -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls \
> -Wshadow -Wstrict-aliasing=3 -Wstrict-prototypes -Wswitch-default -Wswitch-enum \
> -Wundef -Wwrite-strings -Wformat \
> -Werror -O6 -fno-omit-frame-pointer -ggdb3 -funwind-tables -Wall -Wextra -std=gnu99 -fstack-protector-all -D_FORTIFY_SOURCE=2
>
> This is a _trivial_ uninitialized variable bug, yet GCC never warned about it.
> Why?
>
> People build perf with a wide range of GCC versions, from old ones to trunk. I
> cannot believe it that none of those GCC versions warned about this trivial
> looking bug!
I'm only finding these kind of bugs through use of various tools such as
CoverityScan, cppcheck, smatch, etc. It is quite amazing how such bugs
don't get picked up by GCC. The downside is that there are quite a few
false positives to work through, so this is tedious work to separate out
the wheat from the chaff.
>
> And yes, I know that unitialized structures on the stack are valid C code, yet
> it's one of the most fragile aspects of C and it was the source of countless
> security holes in the past...
>
> Thanks,
>
> Ingo
>
Ingo Molnar <[email protected]> writes:
> * Arnaldo Carvalho de Melo <[email protected]> wrote:
>
>> Em Wed, Mar 02, 2016 at 02:21:27PM +0100, Peter Zijlstra escreveu:
>> > On Wed, Mar 02, 2016 at 10:03:50AM -0300, Arnaldo Carvalho de Melo wrote:
>> > > > Would not something like:
>> > > >
>> > > > sa = (struct sigaction){
>> > > > .sa_sigaction = segfault_handler,
>> > > > };
>> > > > sigfillset(&sa.sa_mask);
>> > > >
>> > > > Be better?
>> > >
>> > > I thought about that, but isn't that set in stone? This would be a 4
>> > > liner, while his is a one' :-)
>> >
>> > Dunno, you're right that its rather unlikely struct sigaction is going
>> > to grow another member, but I like the above pattern better in general,
>> > makes it harder to end up with uninitalized bits.
>> >
>> > When performance matters the above pattern isn't ideal, but that should
>> > not be a concern here.
>>
>> Right, I also always use :
>>
>>
>> struct foo bar = {
>> .baz = 1,
>> .name = "whatever",
>> };
>>
>> Even more compact than using that cast. But didn't bother changing in
>> this case.
>
> So the source of the bug was:
>
> struct sigaction sa;
>
> ...
>
> sigfillset(&sa.sa_mask);
> sa.sa_sigaction = segfault_handler;
> sigaction(SIGSEGV, &sa, NULL);
>
> ... which uninitialized sa.sa_flags field GCC merrily accepted as
> proper C code, despite us turning on essentially _all_ GCC warnings
> for the perf build that exist under the sun:
>
> gcc -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k \
> -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs \
> -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls \
> -Wshadow -Wstrict-aliasing=3 -Wstrict-prototypes -Wswitch-default -Wswitch-enum \
> -Wundef -Wwrite-strings -Wformat \
> -Werror -O6 -fno-omit-frame-pointer -ggdb3 -funwind-tables -Wall -Wextra -std=gnu99 -fstack-protector-all -D_FORTIFY_SOURCE=2
>
> This is a _trivial_ uninitialized variable bug, yet GCC never warned
> about it. Why?
>
> People build perf with a wide range of GCC versions, from old ones to
> trunk. I cannot believe it that none of those GCC versions warned
> about this trivial looking bug!
>
> And yes, I know that unitialized structures on the stack are valid C
> code, yet it's one of the most fragile aspects of C and it was the
> source of countless security holes in the past...
Passing a pointer to an uninitialised object is typically not warned
about since the purpose of the call might be to initialise it in the
first place. Now the second argument of sigaction() is a pointer to
const, so the compiler should be able to see that this isn't the case.
Maybe it's not warning because some fields in the struct are initialised
and the function, as far as the compiler knows, might only be accessing
those. (There's certainly code out there using that pattern.) If this
is the case here, a flag to warn unless the object is fully initialised
would be useful to catch bugs like this.
--
M?ns Rullg?rd
* M?ns Rullg?rd <[email protected]> wrote:
> > So the source of the bug was:
> >
> > struct sigaction sa;
> >
> > ...
> >
> > sigfillset(&sa.sa_mask);
> > sa.sa_sigaction = segfault_handler;
> > sigaction(SIGSEGV, &sa, NULL);
> >
> > ... which uninitialized sa.sa_flags field GCC merrily accepted as
> > proper C code, despite us turning on essentially _all_ GCC warnings
> > for the perf build that exist under the sun:
> >
> > gcc -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k \
> > -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs \
> > -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls \
> > -Wshadow -Wstrict-aliasing=3 -Wstrict-prototypes -Wswitch-default -Wswitch-enum \
> > -Wundef -Wwrite-strings -Wformat \
> > -Werror -O6 -fno-omit-frame-pointer -ggdb3 -funwind-tables -Wall -Wextra -std=gnu99 -fstack-protector-all -D_FORTIFY_SOURCE=2
> >
> > This is a _trivial_ uninitialized variable bug, yet GCC never warned
> > about it. Why?
> >
> > People build perf with a wide range of GCC versions, from old ones to
> > trunk. I cannot believe it that none of those GCC versions warned
> > about this trivial looking bug!
> >
> > And yes, I know that unitialized structures on the stack are valid C
> > code, yet it's one of the most fragile aspects of C and it was the
> > source of countless security holes in the past...
>
> Passing a pointer to an uninitialised object is typically not warned about since
> the purpose of the call might be to initialise it in the first place. Now the
> second argument of sigaction() is a pointer to const, so the compiler should be
> able to see that this isn't the case.
>
> Maybe it's not warning because some fields in the struct are initialised and the
> function, as far as the compiler knows, might only be accessing those. (There's
> certainly code out there using that pattern.) If this is the case here, a flag
> to warn unless the object is fully initialised would be useful to catch bugs
> like this.
So it would be absolutely fantastic if one of these solutions existed on GCC:
- emit a warning if a structure is passed around uninitialized. A new GCC
__attribute__((struct_fully_initialized)) could be used to annotate extern
function arguments which fully initialize input arguments.
(I'd personally migrate both tools/perf and kernel side code to use it, module
by module.)
- or memset() to zero all on-stack structures that GCC cannot prove are
initialized fully.
The first solution takes extra work on the source level, the latter takes extra
runtime profiling to find where the extra memset()s matter to performance. Any of
these would be fantastic tools for C robustness and security.
Thanks,
Ingo
On Thu, 2016-03-03 at 13:43 +0100, Ingo Molnar wrote:
> it would be absolutely fantastic if one of these solutions existed on GCC:>
>
> ?- emit a warning if a structure is passed around uninitialized. A new GCC
> ???__attribute__((struct_fully_initialized)) could be used to annotate extern
> ???function arguments which fully initialize input arguments.
>
> ???(I'd personally migrate both tools/perf and kernel side code to use it, module
> ????by module.)
>
> ?- or memset() to zero all on-stack structures that GCC cannot prove are
> ???initialized fully.
>
> The first solution takes extra work on the source level, the latter takes extra?
> runtime profiling to find where the extra memset()s matter to performance. Any of?
> these would be fantastic tools for C robustness and security.
Maybe memset any alignment padding between automatic variables too.
On Thu, Mar 03, 2016 at 01:19:44PM +0100, Ingo Molnar wrote:
> struct sigaction sa;
>
> ...
>
> sigfillset(&sa.sa_mask);
> sa.sa_sigaction = segfault_handler;
> sigaction(SIGSEGV, &sa, NULL);
>
> ... which uninitialized sa.sa_flags field GCC merrily accepted as proper C code,
> despite us turning on essentially _all_ GCC warnings for the perf build that exist
> under the sun:
GCC -W{,maybe-}uninitialized warning works only on SSA form, so in order to
get that warning, either it needs to be some scalar that is uninitialized,
or Scalar Replacement of Aggregates needs to be able to turn the structure
into independent scalars. Neither is the case here, as you take address of
the struct when passing its address to sigaction, and that call can't be
inlined nor is in any other way visible to the compiler, so that it could
optimize both the caller and sigaction itself at the same time.
Even if GCC added infrastructure for tracking which bits/bytes in
aggregates are or might be uninitialized at which point, generally,
struct XYZ abc;
foo (&abc);
is so common pattern that warning here that the fields are uninitialized
would have extremely high false positive ratio.
Even if as somebody mentioned that the argument is const struct sigaction *
rather than struct sigaction *, that doesn't change really anything,
you can cast away the constness and still write into it in the other
function.
Furthermore, in many APIs, only a subset of fields need to be initialized
unconditionally, and other fields might need to be initialized only
conditionally, depending on those always initialized fields, other
parameters to functions, etc.
So, in order to warn here, we'd need some assurance (new attribute on
sigaction function?) that when it is called, it has to have all named fields in
the pointed to structures initialized (perhaps attribute like nonnull, which
can either apply to all pointer arguments, or selected ones) at the entry of
the function and not initializing them all is a bug.
So, this really isn't as trivial as you might think it is.
Jakub
* Jakub Jelinek <[email protected]> wrote:
> On Thu, Mar 03, 2016 at 01:19:44PM +0100, Ingo Molnar wrote:
> > struct sigaction sa;
> >
> > ...
> >
> > sigfillset(&sa.sa_mask);
> > sa.sa_sigaction = segfault_handler;
> > sigaction(SIGSEGV, &sa, NULL);
> >
> > ... which uninitialized sa.sa_flags field GCC merrily accepted as proper C code,
> > despite us turning on essentially _all_ GCC warnings for the perf build that exist
> > under the sun:
>
> GCC -W{,maybe-}uninitialized warning works only on SSA form, so in order to
> get that warning, either it needs to be some scalar that is uninitialized,
> or Scalar Replacement of Aggregates needs to be able to turn the structure
> into independent scalars. Neither is the case here, as you take address of
> the struct when passing its address to sigaction, and that call can't be
> inlined nor is in any other way visible to the compiler, so that it could
> optimize both the caller and sigaction itself at the same time.
>
> Even if GCC added infrastructure for tracking which bits/bytes in
> aggregates are or might be uninitialized at which point, generally,
> struct XYZ abc;
> foo (&abc);
> is so common pattern that warning here that the fields are uninitialized
> would have extremely high false positive ratio.
So at least for the kernel, people rely on external tools that do something like
this anyway, and which are essentially annotated manually that duplicates much of
the effort it would take to make a simple GCC solution work.
So in the aggregate, we already have this overhead _anyway_, except that:
- some of the best tools are closed (so the techniques never enter the free
software world)
- the fashion we get the feedback is per tool and inefficient
- that there's also an inevitable lag between code added upstream and tools
finding uninitialized variables bugs.
So it's all highly inefficient and fragile.
There's also another cost, the cost of finding the bugs themselves - for example
here's a recent upstream kernel fix:
commit e01d8718de4170373cd7fbf5cf6f9cb61cebb1e9
Author: Peter Zijlstra <[email protected]>
Date: Wed Jan 27 23:24:29 2016 +0100
perf/x86: Fix uninitialized value usage
When calling intel_alt_er() with .idx != EXTRA_REG_RSP_* we will not
initialize alt_idx and then use this uninitialized value to index an
array.
When that is not fatal, it can result in an infinite loop in its
caller __intel_shared_reg_get_constraints(), with IRQs disabled.
Alternative error modes are random memory corruption due to the
cpuc->shared_regs->regs[] array overrun, which manifest in either
get_constraints or put_constraints doing weird stuff.
Only took 6 hours of painful debugging to find this. Neither GCC nor
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Smatch warnings flagged this bug.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -1960,7 +1960,8 @@ intel_bts_constraints(struct perf_event *event)
static int intel_alt_er(int idx, u64 config)
{
- int alt_idx;
+ int alt_idx = idx;
+
if (!(x86_pmu.flags & PMU_FL_HAS_RSP_1))
return idx;
6 hours of PeterZ time translates to quite a bit of code restructuring overhead to
eliminate false positive warnings...
So it would scale far better if we could do this kind of checking and annotation
in the kernel code, C module by C module, interface by interface. We could also
push the detection to the stage where such bugs are introduced: when new code is
written - which scales a lot better than the current method of a handful of people
looking at static analysis tools.
If GCC could warn in some really simplistic fashion (accepting tons of false
positives), I'd definitely try to wade through the warnings, eliminate them step
by step and make it all work for a couple of key subsystems I maintain. Most
on-stack structures in the kernel are small, so there's very little reason to be
overly clever with not initializing them.
Thanks,
Ingo
On Thu, Mar 03, 2016 at 02:24:34PM +0100, Ingo Molnar wrote:
> 6 hours of PeterZ time translates to quite a bit of code restructuring overhead to
> eliminate false positive warnings...
I'll file a bugzilla enhancement request for this (with new attribute),
perhaps we could do it in FRE that is able to see through memory
stores/loads even in addressable structures in some cases.
Though, certainly GCC 7 material.
And, in this particular case it couldn't do anything anyway, because
the sigfillset call is not inlined, and takes address of a field in the
structure. The compiler can't know if it doesn't cast it back to struct
sigaction and initialize the other fields.
BTW, valgrind should be able to detect this.
Jakub
* Ingo Molnar <[email protected]> wrote:
> So it's all highly inefficient and fragile.
>
> There's also another cost, the cost of finding the bugs themselves - for example
> here's a recent upstream kernel fix:
>
> commit e01d8718de4170373cd7fbf5cf6f9cb61cebb1e9
> Author: Peter Zijlstra <[email protected]>
> Date: Wed Jan 27 23:24:29 2016 +0100
>
> perf/x86: Fix uninitialized value usage
>
> When calling intel_alt_er() with .idx != EXTRA_REG_RSP_* we will not
> initialize alt_idx and then use this uninitialized value to index an
> array.
>
> When that is not fatal, it can result in an infinite loop in its
> caller __intel_shared_reg_get_constraints(), with IRQs disabled.
>
> Alternative error modes are random memory corruption due to the
> cpuc->shared_regs->regs[] array overrun, which manifest in either
> get_constraints or put_constraints doing weird stuff.
>
> Only took 6 hours of painful debugging to find this. Neither GCC nor
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Smatch warnings flagged this bug.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -1960,7 +1960,8 @@ intel_bts_constraints(struct perf_event *event)
>
> static int intel_alt_er(int idx, u64 config)
> {
> - int alt_idx;
> + int alt_idx = idx;
> +
> if (!(x86_pmu.flags & PMU_FL_HAS_RSP_1))
> return idx;
>
> 6 hours of PeterZ time translates to quite a bit of code restructuring overhead to
> eliminate false positive warnings...
Btw., here's the wider context of that bug:
static int intel_alt_er(int idx, u64 config)
{
int alt_idx;
if (!(x86_pmu.flags & PMU_FL_HAS_RSP_1))
return idx;
if (idx == EXTRA_REG_RSP_0)
alt_idx = EXTRA_REG_RSP_1;
if (idx == EXTRA_REG_RSP_1)
alt_idx = EXTRA_REG_RSP_0;
if (config & ~x86_pmu.extra_regs[alt_idx].valid_mask)
return idx;
return alt_idx;
}
so it's a straightforward uninitialized variable bug.
I tried to distill a testcase out of it, and the following silly hack seems to
trigger it:
------------------------------->
#include <stdio.h>
#define PMU_FL_HAS_RSP_1 1
#define EXTRA_REG_RSP_1 2
#define EXTRA_REG_RSP_0 4
int global_flags = -1;
static int intel_alt_er(int idx, unsigned long long config)
{
int alt_idx;
int uninitialized = 1;
printf("idx: %d, config: %Ld\n", idx, config);
if (!(global_flags & PMU_FL_HAS_RSP_1))
return idx;
if (idx == EXTRA_REG_RSP_0) {
alt_idx = EXTRA_REG_RSP_1;
uninitialized = 0;
}
if (idx == EXTRA_REG_RSP_1) {
alt_idx = EXTRA_REG_RSP_0;
uninitialized = 0;
}
if (config & ~0xff)
return idx;
if (uninitialized)
printf("ugh, using uninitialized alt_idx (%d)!\n", alt_idx);
return alt_idx;
}
int main(int argc, char **argv)
{
argv++;
return intel_alt_er(argc, argc);
}
<-------------------------------
built with:
gcc -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k \
-Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs \
-Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls \
-Wshadow -Wstrict-aliasing=3 -Wstrict-prototypes -Wswitch-default -Wswitch-enum \
-Wundef -Wwrite-strings -Wformat \
-Werror -O6 -fno-omit-frame-pointer -ggdb3 -funwind-tables -Wall -Wextra -std=gnu99 -fstack-protector-all -D_FORTIFY_SOURCE=2 \
-o uninitialized uninitialized.c
gives:
triton:~> ./uninitialized 1
idx: 2, config: 2
triton:~> ./uninitialized 0 0
idx: 3, config: 3
ugh, using uninitialized alt_idx (2)!
I.e. I cannot get GCC to warn about this seemingly trivial bug, using:
gcc version 5.2.1 20151010 (Ubuntu 5.2.1-22ubuntu2)
Thanks,
Ingo
* Jakub Jelinek <[email protected]> wrote:
> On Thu, Mar 03, 2016 at 02:24:34PM +0100, Ingo Molnar wrote:
> > 6 hours of PeterZ time translates to quite a bit of code restructuring overhead to
> > eliminate false positive warnings...
>
> I'll file a bugzilla enhancement request for this (with new attribute),
> perhaps we could do it in FRE that is able to see through memory
> stores/loads even in addressable structures in some cases.
> Though, certainly GCC 7 material.
> And, in this particular case it couldn't do anything anyway, because
> the sigfillset call is not inlined, and takes address of a field in the
> structure. The compiler can't know if it doesn't cast it back to struct
> sigaction and initialize the other fields.
That's true - but I think in the typical case it's a pretty fragile pattern to go
outside the bounds of a on-stack structure you get passed, so I wouldn't mind a
(default-disabled) warning for it, even if it generates false positives that have
to be annotated for the few cases where it's a legitimate technique.
I am 99% sure that a fair number of security critical projects would migrate to
the usage of such a warning, combined with -Werror. I'm 100% sure that perf would
migrate to it.
> BTW, valgrind should be able to detect this.
Yes - assuming the uninitialized value gets used. Often they are in rarely used
code and error paths, only triggered by exploits.
It would be far better if GCC allowed a (non-default) C variant that makes it
impossible to introduce uninitialized values via on-stack variables. The
maintenance cost of the false positives is the price paid for that (very valuable)
guarantee.
Thanks,
Ingo
On Thu, Mar 03, 2016 at 02:47:16PM +0100, Ingo Molnar wrote:
> I tried to distill a testcase out of it, and the following silly hack seems to
> trigger it:
...
This is a known issue, which we don't have a solution for yet.
The thing is, GCC has 2 uninitialized warning passes, one is done
very early, on fairly unoptimized code, which warns for -O and above
only about must be uninitialized cases in code that is executed
unconditionally (if the containing function is executed, and doesn't
have PHI handling code), and then a very late uninitialized pass,
that warns also about maybe-uninitialized cases, has predicate aware
handling in it, etc.; but this warns only about the cases where the
uninitialized uses survived through the optimizations until that phase.
In the testcase, the conditional uninitialized uses got optimized away,
passes seeing that you can get alt_idx initialized say to 2 from one branch
and uninitialized from another one just optimize it into 2.
Warning right away at that spot when the optimization pass performs this
might not be the right thing, as it could warn for stuff in dead code,
or couldn't be backed up by the predicate aware uninit analysis which is
costly and couldn't be done in every pass that just happens to optimize away
some uninitialized stuff. Not to mention that it doesn't have to be always
even so obvious to the optimizing pass. Say, when computing value ranges,
the uninitialized uses should be ignored, because they can't be used in
valid paths, so if say you have value range [2, 34] from one branch and
uninitialized use from another branch, the resulting value range will be
[2, 34]. Then later on, you just optimize based on this value range and
perhaps the uninitialized use will go away because of that.
We could handle the uninitialized uses pessimistically, by not optimizing
PHI <initialized_2, uninited_3(D)> into just initialized_2, etc., by
considering uninitialized uses as VARYING ([min, max] range) rather than
something that doesn't happen, etc., and then the late uninitialized pass
would warn here. But then we'd trade the warning for less optimized code.
GCC is primarily an optimizing compiler, rather than static analyzer, so
that is why GCC chooses to do what it does. Do you want us introduce
-Ow mode, which will prefer warnings over generated code quality?
BTW, as for false positives and new warnings, my experience is that
in the kernel generally such warnings are just disabled, even if they
helped discover severe errors in other packages.
Jakub
* Jakub Jelinek <[email protected]> wrote:
> On Thu, Mar 03, 2016 at 02:47:16PM +0100, Ingo Molnar wrote:
> > I tried to distill a testcase out of it, and the following silly hack seems to
> > trigger it:
>
> ...
>
> This is a known issue, which we don't have a solution for yet.
> The thing is, GCC has 2 uninitialized warning passes, one is done
> very early, on fairly unoptimized code, which warns for -O and above
> only about must be uninitialized cases in code that is executed
> unconditionally (if the containing function is executed, and doesn't
> have PHI handling code), and then a very late uninitialized pass,
> that warns also about maybe-uninitialized cases, has predicate aware
> handling in it, etc.; but this warns only about the cases where the
> uninitialized uses survived through the optimizations until that phase.
> In the testcase, the conditional uninitialized uses got optimized away,
> passes seeing that you can get alt_idx initialized say to 2 from one branch
> and uninitialized from another one just optimize it into 2.
> Warning right away at that spot when the optimization pass performs this
> might not be the right thing, as it could warn for stuff in dead code,
> or couldn't be backed up by the predicate aware uninit analysis which is
> costly and couldn't be done in every pass that just happens to optimize away
> some uninitialized stuff. Not to mention that it doesn't have to be always
> even so obvious to the optimizing pass. Say, when computing value ranges,
> the uninitialized uses should be ignored, because they can't be used in
> valid paths, so if say you have value range [2, 34] from one branch and
> uninitialized use from another branch, the resulting value range will be
> [2, 34]. Then later on, you just optimize based on this value range and
> perhaps the uninitialized use will go away because of that.
> We could handle the uninitialized uses pessimistically, by not optimizing
> PHI <initialized_2, uninited_3(D)> into just initialized_2, etc., by
> considering uninitialized uses as VARYING ([min, max] range) rather than
> something that doesn't happen, etc., and then the late uninitialized pass
> would warn here. But then we'd trade the warning for less optimized code.
> GCC is primarily an optimizing compiler, rather than static analyzer, so
> that is why GCC chooses to do what it does. Do you want us introduce
> -Ow mode, which will prefer warnings over generated code quality?
>
> BTW, as for false positives and new warnings, my experience is that in the
> kernel generally such warnings are just disabled, even if they helped discover
> severe errors in other packages.
That's true to a certain degree, especially when the warning is about something
that can often be used in 'healthy' patterns, and if the workaround for the
warning generates _worse_ code.
One such example was -Wtype-limits.
We tend to disable new GCC warnings if they got enabled indiscriminately, and if
they trigger many false positives that get worked around in the wrong fashion,
with no sane way to write the code to avoid the warning.
Note that the usage in my suggested usecase would be different: we'd not enable
the warning by default (a separate kernel config option would enable it, default
disabled), and we'd not enable it for all C files, but would opt in gradually.
This would remove much of the pressure to hack around annoying warnings in default
kernel builds. For example we had and have a _lot_ of false positive warnings from
Sparse for example, still we gradually eliminated them, because Sparse checking
builds are opt-in.
Thanks,
Ingo
* Jakub Jelinek <[email protected]> wrote:
> On Thu, Mar 03, 2016 at 02:47:16PM +0100, Ingo Molnar wrote:
> > I tried to distill a testcase out of it, and the following silly hack seems to
> > trigger it:
>
> ...
>
> This is a known issue, which we don't have a solution for yet.
> The thing is, GCC has 2 uninitialized warning passes, one is done
> very early, on fairly unoptimized code, which warns for -O and above
> only about must be uninitialized cases in code that is executed
> unconditionally (if the containing function is executed, and doesn't
> have PHI handling code), and then a very late uninitialized pass,
> that warns also about maybe-uninitialized cases, has predicate aware
> handling in it, etc.; but this warns only about the cases where the
> uninitialized uses survived through the optimizations until that phase.
> In the testcase, the conditional uninitialized uses got optimized away,
> passes seeing that you can get alt_idx initialized say to 2 from one branch
> and uninitialized from another one just optimize it into 2.
> Warning right away at that spot when the optimization pass performs this
> might not be the right thing, as it could warn for stuff in dead code,
> or couldn't be backed up by the predicate aware uninit analysis which is
> costly and couldn't be done in every pass that just happens to optimize away
> some uninitialized stuff. Not to mention that it doesn't have to be always
> even so obvious to the optimizing pass. Say, when computing value ranges,
> the uninitialized uses should be ignored, because they can't be used in
> valid paths, so if say you have value range [2, 34] from one branch and
> uninitialized use from another branch, the resulting value range will be
> [2, 34]. Then later on, you just optimize based on this value range and
> perhaps the uninitialized use will go away because of that.
> We could handle the uninitialized uses pessimistically, by not optimizing
> PHI <initialized_2, uninited_3(D)> into just initialized_2, etc., by
> considering uninitialized uses as VARYING ([min, max] range) rather than
> something that doesn't happen, etc., and then the late uninitialized pass
> would warn here. But then we'd trade the warning for less optimized code.
> GCC is primarily an optimizing compiler, rather than static analyzer, so
> that is why GCC chooses to do what it does. Do you want us introduce
> -Ow mode, which will prefer warnings over generated code quality?
Yes, -Ow would be very useful, if it can 'guarantee' that no false negatives slip
through:
It could be combined with the following 'safe' runtime behavior: when built with
-Ow then all uninitialized values are initialized to 0. This should be relatively
easy to implement, as it does not depend on any optimization. After all is said
and done, there's two cases:
- a 0-initialization gets optimized out by an optimization pass. This is the
common case.
- a variable gets initialized to 0 unnecessarily. (If a warning got ignored.)
having some runtime overhead for zero initialization is much preferred for many
projects.
The warning could even be generated at this late stage: i.e. the warning would
simply warn about remaining 0-initializations that previous passes were unable to
eliminate.
This way no undeterministic, random, uninitialized (and worst-case: attacker
controlled) values can ever enter the program flow (from the stack) - in the worst
case (where a warning was ignored) a 0 value is set implicitly - which is still
deterministic behavior.
This is one of the big plusses of managed languages - and we could bring it to C
as well.
Thanks,
Ingo
* Ingo Molnar <[email protected]> wrote:
> Yes, -Ow would be very useful, if it can 'guarantee' that no false negatives slip
> through:
> [...]
> This way no undeterministic, random, uninitialized (and worst-case: attacker
> controlled) values can ever enter the program flow (from the stack) [...]
Note that mainstream Linux distro kernels already enable various options that
cause noticeable runtime overhead: such as stackprotector, or -pg.
So if GCC could simply warn about _all_ uninitialized variables that it cannot
prove are initialized before use, and implicitly initialize them to 0 in that
case, that would be really valuable. (Combined with a function argument attribute
mechanism that tells the compiler that an object pointed to by a pointer gets
fully initialized by the function.)
The runtime overhead can be eliminated by addressing the warnings. If no warnings
are emitted then the generated code should be equivalent to regularly optimized
code, right?
Thanks,
Ingo
Commit-ID: e17a0e16ca3a63d1bafbcba313586cf137418f45
Gitweb: http://git.kernel.org/tip/e17a0e16ca3a63d1bafbcba313586cf137418f45
Author: Colin Ian King <[email protected]>
AuthorDate: Wed, 2 Mar 2016 12:55:22 +0000
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 3 Mar 2016 11:10:39 -0300
perf tests: Initialize sa.sa_flags
The sa_flags field is not being initialized, so a garbage value is being
passed to sigaction. Initialize it to zero.
Signed-off-by: Colin Ian King <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/arch/x86/tests/rdpmc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/perf/arch/x86/tests/rdpmc.c b/tools/perf/arch/x86/tests/rdpmc.c
index 7bb0d13..7945462 100644
--- a/tools/perf/arch/x86/tests/rdpmc.c
+++ b/tools/perf/arch/x86/tests/rdpmc.c
@@ -103,6 +103,7 @@ static int __test__rdpmc(void)
sigfillset(&sa.sa_mask);
sa.sa_sigaction = segfault_handler;
+ sa.sa_flags = 0;
sigaction(SIGSEGV, &sa, NULL);
fd = sys_perf_event_open(&attr, 0, -1, -1,