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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 18 Jan 2018 10:36:58 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     jiri@...nulli.us
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

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.

Thank you.