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

[Luis Henriques <luis.henriques-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>]

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