netdev - Re: [PATCH net-next] 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: <20161024081736.GB2781@nanopsycho>
Date:   Mon, 24 Oct 2016 10:17:36 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Tom Herbert <tom@...bertland.com>,
        "David S. Miller" <davem@...emloft.net>,
        Alexander Duyck <aduyck@...antis.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Hadar Hen Zion <hadarh@...lanox.com>,
        Gao Feng <fgao@...vckh6395k16k5.yundunddos.com>,
        Amir Vadai <amir@...ai.me>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH net-next] flow_dissector: fix vlan tag handling

Sat, Oct 22, 2016 at 10:30:08PM 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 as the stack may have been
>  overwritten.
>
>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.")
>Signed-off-by: Arnd Bergmann <arnd@...db.de>
>---
>
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 44e6ba9d3a6b..17be1b66cc41 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -246,13 +246,10 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 	case htons(ETH_P_8021AD):
> 	case htons(ETH_P_8021Q): {
> 		const struct vlan_hdr *vlan;
>+		struct vlan_hdr _vlan;
>+		bool vlan_tag_present = (skb && skb_vlan_tag_present(skb));

Drop the unnecessary "()"


> 
>-		if (skb && skb_vlan_tag_present(skb))
>-			proto = skb->protocol;

This does not look correct. I believe that you need to set proto for
further processing.


>-
>-		if (eth_type_vlan(proto)) {
>-			struct vlan_hdr _vlan;
>-
>+		if (!vlan_tag_present || eth_type_vlan(skb->protocol)) {
> 			vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan),
> 						    data, hlen, &_vlan);
> 			if (!vlan)
>@@ -270,7 +267,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 							     FLOW_DISSECTOR_KEY_VLAN,
> 							     target_container);
> 
>-			if (skb_vlan_tag_present(skb)) {
>+			if (vlan_tag_present) {
> 				key_vlan->vlan_id = skb_vlan_tag_get_id(skb);
> 				key_vlan->vlan_priority =
> 					(skb_vlan_tag_get_prio(skb) >> VLAN_PRIO_SHIFT);
>