Re: [PATCH v3 net] flow_dissector: fix vlan tag handling
On Mon, Oct 24, 2016 at 11:40:30PM +0200, Arnd Bergmann 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
Acked-by: Eric Garver
Re: [PATCH] flow_dissector: avoid uninitialized variable access
On Sat, Oct 22, 2016 at 12:16:29AM +0200, Arnd Bergmann wrote: > On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote: > > > > Can you explain why "dissector_uses_key(flow_dissector, > > FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies > > "eth_type_vlan(proto))"? > > > > If I add uninitialized_var() here, I would at least put that in > > a comment here. > > Found it now myself: if skb_vlan_tag_present(skb), then we don't > access 'vlan', otherwise we know it is initialized because > eth_type_vlan(proto) has to be true. > > > On a related note, I also don't see how > > "dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)" > > implies that skb is non-NULL. I guess this is related to the > > first one. > > I'm still unsure about this one. Only skb_flow_dissect_flow_keys_buf() calls this function with skb == NULL. It uses flow_keys_buf_dissector_keys which does not specify FLOW_DISSECTOR_KEY_VLAN, so the if statement is false. A similar assumption is made for FLOW_DISSECTOR_KEY_ETH_ADDRS higher up. > I also found something else that is suspicious: 'vlan' points > to the local _vlan variable, but that has gone out of scope > by the time we access the pointer, which doesn't seem safe. I see no harm in moving _vlan to the same scope as vlan.
