[PATCH v5] core, nfqueue, openvswitch: Orphan frags in skb

From: Zoltan Kiss <zoltan.kiss-Sxgqhf6Nn4DQT0dZR+***@public.gmane.org>
Date: Wed, 26 Mar 2014 22:37:45 +0000

Post by 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.

Fingers crossed :-) Applied, thanks Zoltan.

Luis Henriques

2014-04-21 11:26:01 UTC

Hi David,

Post by David Miller
Date: Wed, 26 Mar 2014 22:37:45 +0000

Fingers crossed :-) Applied, thanks Zoltan.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
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

Ben Hutchings

2014-04-22 15:38:38 UTC

Post by Luis Henriques
Hi David,

Post by David Miller
Date: Wed, 26 Mar 2014 22:37:45 +0000

Fingers crossed :-) Applied, thanks Zoltan.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
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/***@public.gmane.org> but it needs
testing/reviewing.

Ben.

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

Zoltan Kiss

2014-04-22 18:02:39 UTC

Post by Luis Henriques
Hi David,

Post by David Miller
Date: Wed, 26 Mar 2014 22:37:45 +0000

Fingers crossed :-) Applied, thanks Zoltan.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
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
testing/reviewing.

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

Zoli

Ben Hutchings

2014-04-22 19:18:04 UTC

From: Zoltan Kiss <zoltan.kiss-Sxgqhf6Nn4DQT0dZR+***@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+***@public.gmane.org>
Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/***@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+***@public.gmane.org>
---

Post by Luis Henriques
Hi David,

Post by David Miller
Date: Wed, 26 Mar 2014 22:37:45 +0000

Fingers crossed :-) Applied, thanks Zoltan.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
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
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

Luis Henriques

2014-04-23 08:57:20 UTC

Post by Ben Hutchings
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.
[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.]
---

Post by Luis Henriques
Hi David,

Post by David Miller
Date: Wed, 26 Mar 2014 22:37:45 +0000

Fingers crossed :-) Applied, thanks Zoltan.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
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
testing/reviewing.

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

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

Cheers,
--
Luís

Post by Ben Hutchings
---
--- 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;
+ 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

Zoltan Kiss

2014-04-23 12:34:10 UTC

Post by Luis Henriques
Hi David,

Post by David Miller
Date: Wed, 26 Mar 2014 22:37:45 +0000

Fingers crossed :-) Applied, thanks Zoltan.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
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
testing/reviewing.

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

Seems OK, thanks!

Post by Ben Hutchings
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;
+ skb_tx_error(entskb);
kfree_skb(skb);
net_err_ratelimited("nf_queue: error creating packet message\n");
return NULL;

Josh Boyer

2014-04-23 13:31:25 UTC

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
--
To unsubscribe from this list: send the line "unsubscribe netfilter" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Ben Hutchings

2014-06-06 16:01:07 UTC

[Trimmed cc list; added Greg and stable]

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

Ben.

Post by Ben Hutchings
---

Post by Luis Henriques
Hi David,

Post by David Miller
Date: Wed, 26 Mar 2014 22:37:45 +0000

Fingers crossed :-) Applied, thanks Zoltan.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
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
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;
+ 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?

Greg Kroah-Hartman

2014-07-29 23:36:15 UTC