netdev - [PATCH RFC] solos-pci: Fix BUG() with shared skb

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]

Date:	Tue, 03 Sep 2013 16:45:25 +0100
From:	David Woodhouse <dwmw2@...radead.org>
To:	netdev@...r.kernel.org
Cc:	Simon Arlott <simon@...e.lp0.eu>,
	linux-atm-general@...ts.sourceforge.net, nathan@...verse.com.au
Subject: [PATCH RFC] solos-pci: Fix BUG() with shared skb

Simon reported this BUG():

 kernel BUG at net/core/skbuff.c:1065!
 Call Trace:
  [<f9b7c12c>] ? pppoatm_send+0x3f/0x1a0 [pppoatm]
  [<f8751797>] psend+0xa9/0x14a [solos_pci]
  [<f9b7c248>] pppoatm_send+0x15b/0x1a0 [pppoatm]
  [<f8a2f77d>] ppp_push+0x76/0x533 [ppp_generic]

(Rest of backtrace at http://s85.org/mn0aOxMN — the skb appears to be
IPv6, forwarded from another interface over PPPoATM.)

I wasn't expecting to see shared skbs in the ATM driver's ->send()
function. Is this the right fix?

I've included the whole function in the patch context... I appear to be
jumping through a lot of hand-coded hoops here, just to ensure that I
can safely prepend my own device's hardware header to the skb. This
makes me think that I'm doing something severely wrong. There ought to
be an easier way of doing this, surely? What am I missing?

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 32784d1..b15e475 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -1135,40 +1135,50 @@ static uint32_t fpga_tx(struct solos_card *card)
 static int psend(struct atm_vcc *vcc, struct sk_buff *skb)
 {
 	struct solos_card *card = vcc->dev->dev_data;
 	struct pkt_hdr *header;
 	int pktlen;
 
 	pktlen = skb->len;
 	if (pktlen > (BUF_SIZE - sizeof(*header))) {
 		dev_warn(&card->dev->dev, "Length of PDU is too large. Dropping PDU.\n");
 		solos_pop(vcc, skb);
 		return 0;
 	}
 
+	/* This is skb_share_check() except it uses solos_pop() instead of kfree_skb() */
+	if (skb_shared(skb)) {
+		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
+		if (unlikely(!nskb)) {
+			solos_pop(vcc, skb);
+			return 0;
+		}
+		consume_skb(skb);
+		skb = nskb;
+	}
 	if (!skb_clone_writable(skb, sizeof(*header))) {
 		int expand_by = 0;
 		int ret;
 
 		if (skb_headroom(skb) < sizeof(*header))
 			expand_by = sizeof(*header) - skb_headroom(skb);
 
 		ret = pskb_expand_head(skb, expand_by, 0, GFP_ATOMIC);
 		if (ret) {
 			dev_warn(&card->dev->dev, "pskb_expand_head failed.\n");
 			solos_pop(vcc, skb);
 			return ret;
 		}
 	}
 
 	header = (void *)skb_push(skb, sizeof(*header));
 
 	/* This does _not_ include the size of the header */
 	header->size = cpu_to_le16(pktlen);
 	header->vpi = cpu_to_le16(vcc->vpi);
 	header->vci = cpu_to_le16(vcc->vci);
 	header->type = cpu_to_le16(PKT_DATA);
 
 	fpga_queue(card, SOLOS_CHAN(vcc->dev), skb, vcc);
 
 	return 0;
 }



-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@...el.com                              Intel Corporation

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5745 bytes)