netdev - Re: [PATCH v3 net] flow_dissector: fix vlan tag handling

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: <20161025081112.GB1672@nanopsycho.orion>
Date:   Tue, 25 Oct 2016 10:11:12 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     "David S. Miller" <davem@...emloft.net>, Eric Garver <e@...g.me>,
        Hadar Hen Zion <hadarh@...lanox.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Alexander Duyck <aduyck@...antis.com>,
        Tom Herbert <tom@...bertland.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 net] flow_dissector: fix vlan tag handling

Mon, Oct 24, 2016 at 11:40:30PM CEST, arnd@...db.de wrote:
>gcc warns about an uninitialized pointer dereference in the vlan
>priority handling:
>
>net/core/flow_dissector.c: In function '__skb_flow_dissect':
>net/core/flow_dissector.c:281:61: error: 'vlan' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
>As pointed out by Jiri Pirko, the variable is never actually used
>without being initialized first as the only way it end up uninitialized
>is with skb_vlan_tag_present(skb)==true, and that means it does not
>get accessed.
>
>However, the warning hints at some related issues that I'm addressing
>here:
>
>- the second check for the vlan tag is different from the first one
>  that tests the skb for being NULL first, causing both the warning
>  and a possible NULL pointer dereference that was not entirely fixed.
>- The same patch that introduced the NULL pointer check dropped an
>  earlier optimization that skipped the repeated check of the
>  protocol type
>- The local '_vlan' variable is referenced through the 'vlan' pointer
>  but the variable has gone out of scope by the time that it is
>  accessed, causing undefined behavior
>
>Caching the result of the 'skb && skb_vlan_tag_present(skb)' check
>in a local variable allows the compiler to further optimize the
>later check. With those changes, the warning also disappears.
>
>Fixes: 3805a938a6c2 ("flow_dissector: Check skb for VLAN only if skb specified.")
>Fixes: d5709f7ab776 ("flow_dissector: For stripped vlan, get vlan info from skb->vlan_tci")
>Signed-off-by: Arnd Bergmann <arnd@...db.de>

Acked-by: Jiri Pirko <jiri@...lanox.com>