netdev - Re: [PATCH] sky2: Fix crash on receiving VLAN frames

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130503080440.73032f54@nehalam.linuxnetplumber.net>
Date:	Fri, 3 May 2013 08:04:40 -0700
From:	Stephen Hemminger <stephen@...workplumber.org>
To:	Kirill Smelkov <kirr@....spb.ru>
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Patrick McHardy <kaber@...sh.net>,
	Mirko Lindner <mlindner@...vell.com>
Subject: Re: [PATCH] sky2: Fix crash on receiving VLAN frames

On Fri,  3 May 2013 18:22:04 +0400
Kirill Smelkov <kirr@....spb.ru> wrote:

> After recent 86a9bad3 (net: vlan: add protocol argument to packet
> tagging functions) my sky2 started to crash on receive of tagged
> frames, with backtrace similar to
> 
>     #CRASH!!!
>     vlan_do_receive
>     __netif_receive_skb_core
>     __netif_receive_skb
>     netif_receive_skb
>     sky2_poll
>     ...
>     __net_rx_action
>     __do_softirq
> 
> The problem turned out to be:
> 
>     1) sky2 copies small packets from ring on RX, and in its
>        receive_copy() skb header is copied manually field, by field, and
>        only for some fields;
> 
>     2) 86a9bad3  added skb->vlan_proto, which vlan_untag() or
>        __vlan_hwaccel_put_tag() set, and which is later used in
>        vlan_do_receive().
> 
>        That patch updated copy_skb_header() for newly introduced
>        skb->vlan_proto, but overlooked the need to also copy it in sky2's
>        receive_copy().
> 
> Because of 2, we have the following scenario:
> 
>     - frame is received and tagged in a ring, by sky2_rx_tag(). Both
>       skb->vlan_proto and skb->vlan_tci are set;
> 
>     - later skb is decided to be copied, but skb->vlan_proto is
>       forgotten and becomes 0.
> 
>     - in the beginning of vlan_do_receive() we call
> 
>         __be16 vlan_proto = skb->vlan_proto;
>         vlan_dev = vlan_find_dev(skb->dev, vlan_proto, vlan_id);
> 
>       which eventually invokes
> 
>         vlan_proto_idx(vlan_proto)
> 
>       and that routine BUGs for everything except ETH_P_8021Q and
>       ETH_P_8021AD.
> 
>       Oops.
> 
> Fix it.
> 
> P.S.
> 
> Stephen, I wonder, why copy_skb_header() is not used in
> sky2.c::receive_copy() ? Problems, where receive_copy was updated field
> by field showed several times already, e.g.
> 
>     3f42941b    (sky2: propogate rx hash when packet is copied)
>     e072b3fa    (sky2: fix receive length error in mixed non-VLAN/VLAN traffic)
> 
> Cc: Patrick McHardy <kaber@...sh.net>
> Cc: Stephen Hemminger <stephen@...workplumber.org>
> Cc: Mirko Lindner <mlindner@...vell.com>
> Signed-off-by: Kirill Smelkov <kirr@....spb.ru>

Acked-by: Stephen Hemminger <stephen@...workplumber.org>

I wonder what other drivers have same issue?
Looking again, copy_skb_header is overkill, it clones a lot of other values
which is not needed on a freshly received skb. The skb at that point has
not had all the other properties set.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html