Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1601300 > unrolled thread

[PATCH 0/7] net, netfilter refcounter conversions

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] net, netfilter refcounter conversions Elena Reshetova <elena.reshetova@intel.com> - 2017-03-15 12:20 +0100
    [PATCH 4/7] net, netfilter: convert nf_acct.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-15 12:20 +0100
    Re: [PATCH 0/7] net, netfilter refcounter conversions Pablo Neira Ayuso <pablo@netfilter.org> - 2017-03-15 14:10 +0100
      RE: [PATCH 0/7] net, netfilter refcounter conversions "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-03-16 09:00 +0100
        Re: [PATCH 0/7] net, netfilter refcounter conversions Pablo Neira Ayuso <pablo@netfilter.org> - 2017-03-17 13:00 +0100

#1601300 — [PATCH 0/7] net, netfilter refcounter conversions

This series, for the netfilter subsystem, replaces atomic_t reference
counters with the new refcount_t type and API (see include/linux/refcount.h).
By doing this we prevent intentional or accidental
underflows or overflows that can led to use-after-free vulnerabilities.

Please take the series to your tree if there are no run-time issues.

Elena Reshetova (7):
  net, netfilter: convert ip_vs_conn.refcnt from atomic_t to refcount_t
  net, netfilter: convert ip_vs_dest.refcnt from atomic_t to refcount_t
  net, netfilter: convert ctnl_timeout.refcnt from atomic_t to
    refcount_t
  net, netfilter: convert nf_acct.refcnt from atomic_t to refcount_t
  net, netfilter: convert nf_conntrack_expect.use from atomic_t to
    refcount_t
  net, netfilter: convert nfulnl_instance.use from atomic_t to
    refcount_t
  net, netfilter: convert clusterip_config.refcount and
    clusterip_config.entries from atomic_t to refcount_t

 include/net/ip_vs.h                          | 16 +++++++++-------
 include/net/netfilter/nf_conntrack_expect.h  |  4 +++-
 include/net/netfilter/nf_conntrack_timeout.h |  3 ++-
 net/ipv4/netfilter/ipt_CLUSTERIP.c           | 19 ++++++++++---------
 net/netfilter/ipvs/ip_vs_conn.c              | 24 ++++++++++++------------
 net/netfilter/ipvs/ip_vs_core.c              |  4 ++--
 net/netfilter/ipvs/ip_vs_ctl.c               | 12 ++++++------
 net/netfilter/ipvs/ip_vs_lblc.c              |  2 +-
 net/netfilter/ipvs/ip_vs_lblcr.c             |  6 +++---
 net/netfilter/ipvs/ip_vs_nq.c                |  2 +-
 net/netfilter/ipvs/ip_vs_proto_sctp.c        |  2 +-
 net/netfilter/ipvs/ip_vs_proto_tcp.c         |  2 +-
 net/netfilter/ipvs/ip_vs_rr.c                |  2 +-
 net/netfilter/ipvs/ip_vs_sed.c               |  2 +-
 net/netfilter/ipvs/ip_vs_wlc.c               |  2 +-
 net/netfilter/ipvs/ip_vs_wrr.c               |  2 +-
 net/netfilter/nf_conntrack_expect.c          | 10 +++++-----
 net/netfilter/nf_conntrack_netlink.c         |  4 ++--
 net/netfilter/nfnetlink_acct.c               | 16 +++++++++-------
 net/netfilter/nfnetlink_cttimeout.c          | 12 ++++++------
 net/netfilter/nfnetlink_log.c                | 14 ++++++++------
 21 files changed, 85 insertions(+), 75 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1601301 — [PATCH 4/7] net, netfilter: convert nf_acct.refcnt from atomic_t to refcount_t

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 net/netfilter/nfnetlink_acct.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index d44d89b..f44cbd3 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
 #include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/netlink.h>
 #include <linux/rculist.h>
 #include <linux/slab.h>
@@ -32,7 +33,7 @@ struct nf_acct {
 	atomic64_t		bytes;
 	unsigned long		flags;
 	struct list_head	head;
-	atomic_t		refcnt;
+	refcount_t		refcnt;
 	char			name[NFACCT_NAME_MAX];
 	struct rcu_head		rcu_head;
 	char			data[0];
@@ -123,7 +124,7 @@ static int nfnl_acct_new(struct net *net, struct sock *nfnl,
 		atomic64_set(&nfacct->pkts,
 			     be64_to_cpu(nla_get_be64(tb[NFACCT_PKTS])));
 	}
-	atomic_set(&nfacct->refcnt, 1);
+	refcount_set(&nfacct->refcnt, 1);
 	list_add_tail_rcu(&nfacct->head, &net->nfnl_acct_list);
 	return 0;
 }
@@ -166,7 +167,7 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 			 NFACCT_PAD) ||
 	    nla_put_be64(skb, NFACCT_BYTES, cpu_to_be64(bytes),
 			 NFACCT_PAD) ||
-	    nla_put_be32(skb, NFACCT_USE, htonl(atomic_read(&acct->refcnt))))
+	    nla_put_be32(skb, NFACCT_USE, htonl(refcount_read(&acct->refcnt))))
 		goto nla_put_failure;
 	if (acct->flags & NFACCT_F_QUOTA) {
 		u64 *quota = (u64 *)acct->data;
@@ -325,11 +326,12 @@ static int nfnl_acct_get(struct net *net, struct sock *nfnl,
 static int nfnl_acct_try_del(struct nf_acct *cur)
 {
 	int ret = 0;
+	unsigned int refcount;
 
 	/* We want to avoid races with nfnl_acct_put. So only when the current
 	 * refcnt is 1, we decrease it to 0.
 	 */
-	if (atomic_cmpxchg(&cur->refcnt, 1, 0) == 1) {
+	if (refcount_dec_if_one(&cur->refcnt)) {
 		/* We are protected by nfnl mutex. */
 		list_del_rcu(&cur->head);
 		kfree_rcu(cur, rcu_head);
@@ -413,7 +415,7 @@ struct nf_acct *nfnl_acct_find_get(struct net *net, const char *acct_name)
 		if (!try_module_get(THIS_MODULE))
 			goto err;
 
-		if (!atomic_inc_not_zero(&cur->refcnt)) {
+		if (!refcount_inc_not_zero(&cur->refcnt)) {
 			module_put(THIS_MODULE);
 			goto err;
 		}
@@ -429,7 +431,7 @@ EXPORT_SYMBOL_GPL(nfnl_acct_find_get);
 
 void nfnl_acct_put(struct nf_acct *acct)
 {
-	if (atomic_dec_and_test(&acct->refcnt))
+	if (refcount_dec_and_test(&acct->refcnt))
 		kfree_rcu(acct, rcu_head);
 
 	module_put(THIS_MODULE);
@@ -502,7 +504,7 @@ static void __net_exit nfnl_acct_net_exit(struct net *net)
 	list_for_each_entry_safe(cur, tmp, &net->nfnl_acct_list, head) {
 		list_del_rcu(&cur->head);
 
-		if (atomic_dec_and_test(&cur->refcnt))
+		if (refcount_dec_and_test(&cur->refcnt))
 			kfree_rcu(cur, rcu_head);
 	}
 }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1601384

On Wed, Mar 15, 2017 at 01:10:38PM +0200, Elena Reshetova wrote:
> This series, for the netfilter subsystem, replaces atomic_t reference
> counters with the new refcount_t type and API (see include/linux/refcount.h).
> By doing this we prevent intentional or accidental
> underflows or overflows that can led to use-after-free vulnerabilities.
> 
> Please take the series to your tree if there are no run-time issues.

Could you collapse all of your patches into one single? They are all
part of the same logical change to me.

>  21 files changed, 85 insertions(+), 75 deletions(-)

The diffstat is small enough to do what I'm asking.

Thanks!

[toc] | [prev] | [next] | [standalone]

#1602040

> On Wed, Mar 15, 2017 at 01:10:38PM +0200, Elena Reshetova wrote:
> > This series, for the netfilter subsystem, replaces atomic_t reference
> > counters with the new refcount_t type and API (see include/linux/refcount.h).
> > By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> >
> > Please take the series to your tree if there are no run-time issues.
> 
> Could you collapse all of your patches into one single? They are all
> part of the same logical change to me.
> 
> >  21 files changed, 85 insertions(+), 75 deletions(-)
> 
> The diffstat is small enough to do what I'm asking.

Sure. The reason why they are separated is that it is easier to review them this way IMO and find mistakes (I found many after I split all networking patches into one per variable).
But I guess for merge, it is easier to have them collapsed, so I am going to send you a new version shortly. 

Best Regards,
Elena.

> 
> Thanks!

[toc] | [prev] | [next] | [standalone]

#1603240

On Thu, Mar 16, 2017 at 07:52:19AM +0000, Reshetova, Elena wrote:
> 
> > On Wed, Mar 15, 2017 at 01:10:38PM +0200, Elena Reshetova wrote:
> > > This series, for the netfilter subsystem, replaces atomic_t reference
> > > counters with the new refcount_t type and API (see include/linux/refcount.h).
> > > By doing this we prevent intentional or accidental
> > > underflows or overflows that can led to use-after-free vulnerabilities.
> > >
> > > Please take the series to your tree if there are no run-time issues.
> > 
> > Could you collapse all of your patches into one single? They are all
> > part of the same logical change to me.
> > 
> > >  21 files changed, 85 insertions(+), 75 deletions(-)
> > 
> > The diffstat is small enough to do what I'm asking.
> 
> Sure. The reason why they are separated is that it is easier to
> review them this way IMO and find mistakes (I found many after I
> split all networking patches into one per variable).  But I guess
> for merge, it is easier to have them collapsed, so I am going to
> send you a new version shortly.

In my particular case, collapsing them is good so the Netfilter batch
I pass up to David becomes smaller. Thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web