netdev - Re: [patch net-next v2] net: sched: silence uninitialized parent variable warning in tc_dump

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]

Date:   Thu, 18 Jan 2018 17:02:17 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
        mlxsw@...lanox.com, sfr@...b.auug.org.au, arnd@...db.de
Subject: Re: [patch net-next v2] net: sched: silence uninitialized parent
 variable warning in tc_dump_tfilter

Thu, Jan 18, 2018 at 04:36:58PM CET, davem@...emloft.net wrote:
>From: Jiri Pirko <jiri@...nulli.us>
>Date: Thu, 18 Jan 2018 16:14:49 +0100
>
>> @@ -1317,6 +1317,13 @@ static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)
>>  		block = tcf_block_lookup(net, tcm->tcm_block_index);
>>  		if (!block)
>>  			goto out;
>> +		/* If we work with block index, q is NULL and parent value
>> +		 * will never be used in the following code. The check
>> +		 * in tcf_fill_node prevents it. However, compiler does not
>> +		 * see that far, so set parent to zero to silence the warning
>> +		 * about parent being uninitialized.
>> +		 */
>> +		parent = 0;
>>  	} else {
>
>Ugh....
>
>Jiri, if you need to add such a verbose comment to explain a compiler
>warning fix, then this code is too complicated for humans to
>understand and audit properly.
>
>And from this perspective I really don't blame the compiler.  Even
>I am still having trouble putting all of these invariants together,
>even considering the information in this comment, in order to see
>how this is "ok".
>
>And even if tcf_fill_node() doesn't access parent, tcf_chain_dump()
>does and stores this uninitialized value into the 'args' if we
>run out of space during the dump.
>
>Yes, I understand that this value will never be used, but wow that
>is propagating an uninitialized value across dump passes.
>
>I've applied this, but please look into restructuring this code
>so that it is a bit more sane in this regard.

Ack. Will try to figure out how to make this saner.

Thanks.