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]

Date:   Sun, 22 Jan 2017 16:50:31 +0100
From:   Nicolas Iooss <nicolas.iooss_linux@....org>
To:     Borislav Petkov <bp@...en8.de>
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 22/01/17 15:41, Borislav Petkov wrote:
> 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.

Thanks for your quick reply! I agree with your proposal and will send an
other patch which implements it.

In your reply, there is one point I have not understood. When doing "if
(!is_knl && !interleave_mode(reg))", the condition assumes that
interleave mode 1 has the same meaning on all kinds of CPUs. However the
current code prints this mode as "[10:8]" on KNL and "8:6" anywhere
else. So I would rather modify the code this way:

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

Would this be good for you?

Thanks