[PATCH] netfilter: ipset: Fix race between dump and swap

[PATCH] netfilter: ipset: Fix race between dump and swap

[Ross Lagerwall]

Fix a race between ip_set_dump_start() and ip_set_swap().
The race is as follows:
* Without holding the ref lock, ip_set_swap() checks ref_netlink of the
  set and it is 0.
* ip_set_dump_start() takes a reference on the set.
* ip_set_swap() does the swap (even though it now has a non-zero
  reference count).
* ip_set_dump_start() gets the set from ip_set_list again which is now a
  different set since it has been swapped.
* ip_set_dump_start() calls __ip_set_put_netlink() and hits a BUG_ON due
  to the reference count being 0.

Fix this race by extending the critical region in which the ref lock is
held to include checking the ref counts.

The race can be reproduced with the following script:
  while :; do
    ipset destroy hash_ip1
    ipset destroy hash_ip2
    ipset create hash_ip1 hash:ip family inet hashsize 1024 \
        maxelem 500000
    ipset create hash_ip2 hash:ip family inet hashsize 300000 \
        maxelem 500000
    ipset create hash_ip3 hash:ip family inet hashsize 1024 \
        maxelem 500000
    ipset save &
    ipset swap hash_ip3 hash_ip2
    ipset destroy hash_ip3
    wait
  done

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 net/netfilter/ipset/ip_set_core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index ba6a551..cae3e4a 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1185,14 +1185,17 @@ static int ip_set_swap(struct net *net, struct sock *ctnl, struct sk_buff *skb,
 	      from->family == to->family))
 		return -IPSET_ERR_TYPE_MISMATCH;
 
-	if (from->ref_netlink || to->ref_netlink)
+	write_lock_bh(&ip_set_ref_lock);
+
+	if (from->ref_netlink || to->ref_netlink) {
+		write_unlock_bh(&ip_set_ref_lock);
 		return -EBUSY;
+	}
 
 	strncpy(from_name, from->name, IPSET_MAXNAMELEN);
 	strncpy(from->name, to->name, IPSET_MAXNAMELEN);
 	strncpy(to->name, from_name, IPSET_MAXNAMELEN);
 
-	write_lock_bh(&ip_set_ref_lock);
 	swap(from->ref, to->ref);
 	ip_set(inst, from_id) = to;
 	ip_set(inst, to_id) = from;
-- 
2.9.5

Re: [PATCH] netfilter: ipset: Fix race between dump and swap

[Jozsef Kadlecsik]

On Wed, 27 Sep 2017, Ross Lagerwall wrote:

> Fix a race between ip_set_dump_start() and ip_set_swap().
> The race is as follows:
> * Without holding the ref lock, ip_set_swap() checks ref_netlink of the
>   set and it is 0.
> * ip_set_dump_start() takes a reference on the set.
> * ip_set_swap() does the swap (even though it now has a non-zero
>   reference count).
> * ip_set_dump_start() gets the set from ip_set_list again which is now a
>   different set since it has been swapped.
> * ip_set_dump_start() calls __ip_set_put_netlink() and hits a BUG_ON due
>   to the reference count being 0.
> 
> Fix this race by extending the critical region in which the ref lock is
> held to include checking the ref counts.
> 
> The race can be reproduced with the following script:
>   while :; do
>     ipset destroy hash_ip1
>     ipset destroy hash_ip2
>     ipset create hash_ip1 hash:ip family inet hashsize 1024 \
>         maxelem 500000
>     ipset create hash_ip2 hash:ip family inet hashsize 300000 \
>         maxelem 500000
>     ipset create hash_ip3 hash:ip family inet hashsize 1024 \
>         maxelem 500000
>     ipset save &
>     ipset swap hash_ip3 hash_ip2
>     ipset destroy hash_ip3
>     wait
>   done
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>

Good catch, Pablo please apply in the nf tree. Thanks!

Best regards,
Jozsef

> ---
>  net/netfilter/ipset/ip_set_core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
> index ba6a551..cae3e4a 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -1185,14 +1185,17 @@ static int ip_set_swap(struct net *net, struct sock *ctnl, struct sk_buff *skb,
>  	      from->family == to->family))
>  		return -IPSET_ERR_TYPE_MISMATCH;
>  
> -	if (from->ref_netlink || to->ref_netlink)
> +	write_lock_bh(&ip_set_ref_lock);
> +
> +	if (from->ref_netlink || to->ref_netlink) {
> +		write_unlock_bh(&ip_set_ref_lock);
>  		return -EBUSY;
> +	}
>  
>  	strncpy(from_name, from->name, IPSET_MAXNAMELEN);
>  	strncpy(from->name, to->name, IPSET_MAXNAMELEN);
>  	strncpy(to->name, from_name, IPSET_MAXNAMELEN);
>  
> -	write_lock_bh(&ip_set_ref_lock);
>  	swap(from->ref, to->ref);
>  	ip_set(inst, from_id) = to;
>  	ip_set(inst, to_id) = from;
> -- 
> 2.9.5
> 
> --
> 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
> 

-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] netfilter: ipset: Fix race between dump and swap

[Pablo Neira Ayuso]

On Thu, Sep 28, 2017 at 12:31:02PM +0200, Jozsef Kadlecsik wrote:
> On Wed, 27 Sep 2017, Ross Lagerwall wrote:
> 
> > Fix a race between ip_set_dump_start() and ip_set_swap().
> > The race is as follows:
> > * Without holding the ref lock, ip_set_swap() checks ref_netlink of the
> >   set and it is 0.
> > * ip_set_dump_start() takes a reference on the set.
> > * ip_set_swap() does the swap (even though it now has a non-zero
> >   reference count).
> > * ip_set_dump_start() gets the set from ip_set_list again which is now a
> >   different set since it has been swapped.
> > * ip_set_dump_start() calls __ip_set_put_netlink() and hits a BUG_ON due
> >   to the reference count being 0.
> > 
> > Fix this race by extending the critical region in which the ref lock is
> > held to include checking the ref counts.
> > 
> > The race can be reproduced with the following script:
> >   while :; do
> >     ipset destroy hash_ip1
> >     ipset destroy hash_ip2
> >     ipset create hash_ip1 hash:ip family inet hashsize 1024 \
> >         maxelem 500000
> >     ipset create hash_ip2 hash:ip family inet hashsize 300000 \
> >         maxelem 500000
> >     ipset create hash_ip3 hash:ip family inet hashsize 1024 \
> >         maxelem 500000
> >     ipset save &
> >     ipset swap hash_ip3 hash_ip2
> >     ipset destroy hash_ip3
> >     wait
> >   done
> > 
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> 
> Good catch, Pablo please apply in the nf tree. Thanks!

Thanks a lot Jozsef. This is applied in the nf tree.