linux-kernel - Re: Q: why didn't GCC warn about this uninitialized variable? (was: Re: [PATCH] perf tests: initialize sa.sa

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]

Date:	Thu, 3 Mar 2016 14:47:16 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Jakub Jelinek <jakub@...hat.com>
Cc:	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Colin King <colin.king@...onical.com>,
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
	Richard Henderson <rth@...ddle.net>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: Q: why didn't GCC warn about this uninitialized variable? (was:
 Re: [PATCH] perf tests: initialize sa.sa_flags)


* Ingo Molnar <mingo@...nel.org> 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 <peterz@...radead.org>
>   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