netdev - [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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <1367590924-8594-1-git-send-email-kirr@mns.spb.ru>
Date:	Fri,  3 May 2013 18:22:04 +0400
From:	Kirill Smelkov <kirr@....spb.ru>
To:	"David S. Miller" <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, Kirill Smelkov <kirr@....spb.ru>,
	Patrick McHardy <kaber@...sh.net>,
	Stephen Hemminger <stephen@...workplumber.org>,
	Mirko Lindner <mlindner@...vell.com>
Subject: [PATCH] sky2: Fix crash on receiving VLAN frames

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>
---
 drivers/net/ethernet/marvell/sky2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index 256ae78..d175bbd 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -2496,10 +2496,12 @@ static struct sk_buff *receive_copy(struct sky2_port *sky2,
 		skb->ip_summed = re->skb->ip_summed;
 		skb->csum = re->skb->csum;
 		skb->rxhash = re->skb->rxhash;
+		skb->vlan_proto = re->skb->vlan_proto;
 		skb->vlan_tci = re->skb->vlan_tci;
 
 		pci_dma_sync_single_for_device(sky2->hw->pdev, re->data_addr,
 					       length, PCI_DMA_FROMDEVICE);
+		re->skb->vlan_proto = 0;
 		re->skb->vlan_tci = 0;
 		re->skb->rxhash = 0;
 		re->skb->ip_summed = CHECKSUM_NONE;
-- 
1.8.3.rc0.361.g39d8700

--
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