linux-kernel - Re: [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave

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: <20170122144144.bwgkh3esdcveim3q@pd.tnic>
Date:   Sun, 22 Jan 2017 15:41:44 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Nicolas Iooss <nicolas.iooss_linux@....org>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] EDAC: always return an initialized value in
 knl_show_interleave_mode

On Sun, Jan 22, 2017 at 02:51:15PM +0100, Nicolas Iooss wrote:
> When building drivers/edac/sb_edac.c with compiler warning flags which
> aim to detect the use of uninitialized values at compile time, the
> compiler reports that knl_show_interleave_mode() may return an
> uninitialized value. This function indeed uses a switch statement to set
> a local variable ("s"), which is not set to anything in the default
> statement. Anyway this would be never reached as
> knl_interleave_mode(reg) always returns a value between 0 and 3, but the
> compiler has no way of knowing this.
> 
> Silent the compiler warning by initializing variable "s" too in the
> default case. While at it, make knl_show_interleave_mode() and
> show_interleave_mode() return const char* values as the returned
> pointers refer to read-only memory.

No, this is trying to make pretty an already ugly pile of crap.

That ->show_interleave_mode() is called only once in a debug printk. Big
deal. But it is a function pointer which points to the same function
except for KNL.

Then, the default implementation returns bit slices: "8:6" :
"[8:6]XOR[18:16]" but the KNL one says "use address bits" like this is
the SDM. Yeah, right. I should've caught that when the KNL pile was
added.

So here's what I'd like you to do, instead:

Kill that ->show_interleave_mode() thing and use the default
show_interleave_mode() at the only call site. You can pass in a second
bool argument is_knl which is "pvt->info.type == KNIGHTS_LANDING".

And then add the KNL functionality from knl_show_interleave_mode() to
the default show_interleave_mode().

And get rid of that default: case - it won't be reached anyway.

You can even define a string array:

struct pair intlv_mode[] = {
	"[8:6]", "[10:8]", ...;
};

and then do:

	if (!is_knl && !interleave_mode(reg))
		return "[8:6]XOR[18:16]";
	else
		return intlv_mode[knl_interleave_mode()];

and be done with it.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.