| Message ID | 1398194284.7767.101.camel@deadeye.wl.decadent.org.uk |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Apr 22, 2014 at 08:18:04PM +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> > --- > 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. 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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/04/14 20:18, 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> > --- > 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. 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; > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[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; >
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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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;