[ovs-dev] [PATCH 3.13] core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors

[Ben Hutchings]

From: Zoltan Kiss <zoltan.kiss at citrix.com>

commit 36d5fe6a000790f56039afe26834265db0a3ad4c upstream.

skb_zerocopy can copy elements of the frags array between skbs, but it doesn't
orphan them. Also, it doesn't handle errors, so this patch takes care of that
as well, and modify the callers accordingly. skb_tx_error() is also added to
the callers so they will signal the failed delivery towards the creator of the
skb.

Signed-off-by: Zoltan Kiss <zoltan.kiss at citrix.com>
Signed-off-by: David S. Miller <davem at davemloft.net>
[bwh: Backported to 3.13: skb_zerocopy() is new in 3.14, but was moved from a
 static function in nfnetlink_queue.  We need to patch that and its caller, but
 not openvswitch.]
Signed-off-by: Ben Hutchings <ben at decadent.org.uk>
---
On Tue, 2014-04-22 at 19:02 +0100, Zoltan Kiss wrote:
> On 22/04/14 16:38, Ben Hutchings wrote:
> > On Mon, 2014-04-21 at 12:26 +0100, Luis Henriques wrote:
> >> Hi David,
> >>
> >> On Thu, Mar 27, 2014 at 03:29:56PM -0400, David Miller wrote:
> >>> From: Zoltan Kiss <zoltan.kiss at citrix.com>
> >>> Date: Wed, 26 Mar 2014 22:37:45 +0000
> >>>
> >>>> skb_zerocopy can copy elements of the frags array between skbs, but it doesn't
> >>>> orphan them. Also, it doesn't handle errors, so this patch takes care of that
> >>>> as well, and modify the callers accordingly. skb_tx_error() is also added to
> >>>> the callers so they will signal the failed delivery towards the creator of the
> >>>> skb.
> >>>>
> >>>> Signed-off-by: Zoltan Kiss <zoltan.kiss at citrix.com>
> >>>
> >>> Fingers crossed :-)  Applied, thanks Zoltan.
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>> the body of a message to majordomo at vger.kernel.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >> Could you please queue this for stable as well?  It has CVE-2014-2568
> >> assigned to it and I believe it is applicable to some of the trees.
> >
> > It was applied to 3.13, but needs backporting to earlier versions.  I
> > posted my attempt in
> > <1397429860.10849.86.camel at deadeye.wl.decadent.org.uk> but it needs
> > testing/reviewing.
> 
> This one is a different issue, although it is very similar.

Sorry for mixing these up.  I believe this bug has been present since
zerocopy was added to nfqueue in Linux 3.10 (commit ae08ce002108).  This
is the version I used for Debian's 3.13 branch, which might be usable
for older stable branches too.

Ben.

---
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -235,22 +235,23 @@ nfqnl_flush(struct nfqnl_instance *queue
 	spin_unlock_bh(&queue->lock);
 }
 
-static void
+static int
 nfqnl_zcopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
 {
 	int i, j = 0;
 	int plen = 0; /* length of skb->head fragment */
+	int ret;
 	struct page *page;
 	unsigned int offset;
 
 	/* dont bother with small payloads */
-	if (len <= skb_tailroom(to)) {
-		skb_copy_bits(from, 0, skb_put(to, len), len);
-		return;
-	}
+	if (len <= skb_tailroom(to))
+		return skb_copy_bits(from, 0, skb_put(to, len), len);
 
 	if (hlen) {
-		skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
+		ret = skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
+		if (unlikely(ret))
+			return ret;
 		len -= hlen;
 	} else {
 		plen = min_t(int, skb_headlen(from), len);
@@ -268,6 +269,11 @@ nfqnl_zcopy(struct sk_buff *to, const st
 	to->len += len + plen;
 	to->data_len += len + plen;
 
+	if (unlikely(skb_orphan_frags(from, GFP_ATOMIC))) {
+		skb_tx_error(from);
+		return -ENOMEM;
+	}
+
 	for (i = 0; i < skb_shinfo(from)->nr_frags; i++) {
 		if (!len)
 			break;
@@ -278,6 +284,8 @@ nfqnl_zcopy(struct sk_buff *to, const st
 		j++;
 	}
 	skb_shinfo(to)->nr_frags = j;
+
+	return 0;
 }
 
 static int
@@ -374,13 +382,16 @@ nfqnl_build_packet_message(struct net *n
 
 	skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
 				  GFP_ATOMIC);
-	if (!skb)
+	if (!skb) {
+		skb_tx_error(entskb);
 		return NULL;
+	}
 
 	nlh = nlmsg_put(skb, 0, 0,
 			NFNL_SUBSYS_QUEUE << 8 | NFQNL_MSG_PACKET,
 			sizeof(struct nfgenmsg), 0);
 	if (!nlh) {
+		skb_tx_error(entskb);
 		kfree_skb(skb);
 		return NULL;
 	}
@@ -504,13 +515,15 @@ nfqnl_build_packet_message(struct net *n
 		nla->nla_type = NFQA_PAYLOAD;
 		nla->nla_len = nla_attr_size(data_len);
 
-		nfqnl_zcopy(skb, entskb, data_len, hlen);
+		if (nfqnl_zcopy(skb, entskb, data_len, hlen))
+			goto nla_put_failure;
 	}
 
 	nlh->nlmsg_len = skb->len;
 	return skb;
 
 nla_put_failure:
+	skb_tx_error(entskb);
 	kfree_skb(skb);
 	net_err_ratelimited("nf_queue: error creating packet message\n");
 	return NULL;

-- 
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 811 bytes
Desc: This is a digitally signed message part
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140422/7125755e/attachment-0005.sig>