[PATCH v5] core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors

[PATCH v5] core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors

[Zoltan Kiss]

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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
---
v2: orphan the frags right before touching the frags

v3:
- orphan 'from' instead of 'to'
- call skb_tx_error() in the callers if something went wrong

v4: correctly use error path in queue_userspace_packet

v5: remove const qualifier of "from" in skb_zerocopy parameters

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 03db95a..0db91fa 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2508,8 +2508,8 @@ int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
 		    unsigned int flags);
 void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to);
 unsigned int skb_zerocopy_headlen(const struct sk_buff *from);
-void skb_zerocopy(struct sk_buff *to, const struct sk_buff *from,
-		  int len, int hlen);
+int skb_zerocopy(struct sk_buff *to, struct sk_buff *from,
+		 int len, int hlen);
 void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len);
 int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
 void skb_scrub_packet(struct sk_buff *skb, bool xnet);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3f14c63..908ad98 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2127,25 +2127,31 @@ EXPORT_SYMBOL_GPL(skb_zerocopy_headlen);
  *
  *	The `hlen` as calculated by skb_zerocopy_headlen() specifies the
  *	headroom in the `to` buffer.
+ *
+ *	Return value:
+ *	0: everything is OK
+ *	-ENOMEM: couldn't orphan frags of @from due to lack of memory
+ *	-EFAULT: skb_copy_bits() found some problem with skb geometry
  */
-void
-skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
+int
+skb_zerocopy(struct sk_buff *to, 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;
 
 	BUG_ON(!from->head_frag && !hlen);
 
 	/* 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);
@@ -2163,6 +2169,11 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
 	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;
@@ -2173,6 +2184,8 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
 		j++;
 	}
 	skb_shinfo(to)->nr_frags = j;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(skb_zerocopy);
 
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index f072fe8..108120f 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -354,13 +354,16 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 
 	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;
 	}
@@ -488,13 +491,15 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 		nla->nla_type = NFQA_PAYLOAD;
 		nla->nla_len = nla_attr_size(data_len);
 
-		skb_zerocopy(skb, entskb, data_len, hlen);
+		if (skb_zerocopy(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;
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index c53fe0c..9230223 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -464,7 +464,9 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
 	}
 	nla->nla_len = nla_attr_size(skb->len);
 
-	skb_zerocopy(user_skb, skb, skb->len, hlen);
+	err = skb_zerocopy(user_skb, skb, skb->len, hlen);
+	if (err)
+		goto out;
 
 	/* Pad OVS_PACKET_ATTR_PACKET if linear copy was performed */
 	if (!(dp->user_features & OVS_DP_F_UNALIGNED)) {
@@ -478,6 +480,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
 
 	err = genlmsg_unicast(ovs_dp_get_net(dp), user_skb, upcall_info->portid);
 out:
+	if (err)
+		skb_tx_error(skb);
 	kfree_skb(nskb);
 	return err;
 }

Re: [PATCH v5] core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors

[David Miller]

From: Zoltan Kiss <zoltan.kiss-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>

Fingers crossed :-)  Applied, thanks Zoltan.

Re: [PATCH v5] core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors

[Luis Henriques]

Hi David,

On Thu, Mar 27, 2014 at 03:29:56PM -0400, David Miller wrote:
> From: Zoltan Kiss <zoltan.kiss-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> 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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> 
> Fingers crossed :-)  Applied, thanks Zoltan.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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.

Cheers,
--
Luís

Re: [PATCH v5] core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 1468 bytes --]

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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> > 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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> > 
> > Fingers crossed :-)  Applied, thanks Zoltan.
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-nDn/Rdv9kqW9Jme8/bJn5UCKIB8iOfG2tUK59QYPAWc@public.gmane.org> but it needs
testing/reviewing.

Ben.

-- 
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5] core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors

[Zoltan Kiss]

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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
>>> 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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
>>>
>>> Fingers crossed :-)  Applied, thanks Zoltan.
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-nDn/Rdv9kqW9Jme8/bJn5UCKIB8iOfG2tUK59QYPAWc@public.gmane.org> but it needs
> testing/reviewing.

This one is a different issue, although it is very similar.

Zoli

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

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 5097 bytes --]

From: Zoltan Kiss <zoltan.kiss-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>

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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
[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-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org>
---
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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> >>> 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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> >>>
> >>> Fingers crossed :-)  Applied, thanks Zoltan.
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-nDn/Rdv9kqW9Jme8/bJn5UCKIB8iOfG2tUK59QYPAWc@public.gmane.org> 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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Luis Henriques]

On Tue, Apr 22, 2014 at 08:18:04PM +0100, Ben Hutchings wrote:
> From: Zoltan Kiss <zoltan.kiss-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> 
> 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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> [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-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org>
> ---
> 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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> > >>> 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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> > >>>
> > >>> Fingers crossed :-)  Applied, thanks Zoltan.
> > >>> --
> > >>> To unsubscribe from this list: send the line "unsubscribe netdev" in
> > >>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-nDn/Rdv9kqW9Jme8/bJn5UCKIB8iOfG2tUK59QYPAWc@public.gmane.org> 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.

Thank you for this backport Ben.  It looks correct to me and it seems
to be applicable to 3.10+ kernels

Cheers,
--
Luís


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

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

[Zoltan Kiss]

On 22/04/14 20:18, Ben Hutchings wrote:
> From: Zoltan Kiss <zoltan.kiss-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
>
> 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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> [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-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org>
> ---
> 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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
>>>>> 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-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> Fingers crossed :-)  Applied, thanks Zoltan.
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-nDn/Rdv9kqW9Jme8/bJn5UCKIB8iOfG2tUK59QYPAWc@public.gmane.org> 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.
Seems OK, thanks!

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

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

[Josh Boyer]

On Tue, Apr 22, 2014 at 3:18 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> From: Zoltan Kiss <zoltan.kiss@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@citrix.com>
> Signed-off-by: David S. Miller <davem@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@decadent.org.uk>

Deja-vu.  I sent almost the same patch to netdev a while ago and you
acked it there.  So I guess I'll ack your copy as well :).  Not sure
why either version wasn't picked up by Dave.

josh

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 5372 bytes --]

[Trimmed cc list; added Greg and stable]

On Tue, 2014-04-22 at 20:18 +0100, Ben Hutchings wrote:
> From: Zoltan Kiss <zoltan.kiss@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@citrix.com>
> Signed-off-by: David S. Miller <davem@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@decadent.org.uk>

I noticed this is still not applied or queued for the 3.10.y branch, but
it does seem to be needed.

Ben.

> ---
> 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@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@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@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@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
You can't have everything.  Where would you put it?


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

[Greg Kroah-Hartman]

On Fri, Jun 06, 2014 at 05:01:07PM +0100, Ben Hutchings wrote:
> [Trimmed cc list; added Greg and stable]
> 
> On Tue, 2014-04-22 at 20:18 +0100, Ben Hutchings wrote:
> > From: Zoltan Kiss <zoltan.kiss@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@citrix.com>
> > Signed-off-by: David S. Miller <davem@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@decadent.org.uk>
> 
> I noticed this is still not applied or queued for the 3.10.y branch, but
> it does seem to be needed.

Thanks, I've queued this up for 3.10-stable.

greg k-h