Subject: Re: kern/37037: ipnat: Data modified on freelist
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: Martin Husemann <martin@duskware.de>
List: netbsd-bugs
Date: 10/03/2007 10:05:09
The following reply was made to PR kern/37037; it has been noted by GNATS.
From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc: stix@stix.id.au, Michael van Elst <mlelstv@serpens.de>
Subject: Re: kern/37037: ipnat: Data modified on freelist
Date: Wed, 3 Oct 2007 12:03:11 +0200
--5vNYLRcllDrimb99
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Could you try if this patch alone fixes the write after free problem
for you too? Restore the original RC2 userland binaries/headers and just
try a kernel build with this patch (against RC2, without Martti's patch).
It won't touch the tcp rdr timeouts, but that should be a different PR
(and IMHO is not fixed in IPF 4.1.27, but maybe I overlooked something).
Martin
--5vNYLRcllDrimb99
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=patch
Index: ip_nat.c
===================================================================
RCS file: /cvsroot/src/sys/dist/ipf/netinet/ip_nat.c,v
retrieving revision 1.19.2.7
diff -c -u -p -r1.19.2.7 ip_nat.c
--- ip_nat.c 16 Sep 2007 15:46:48 -0000 1.19.2.7
+++ ip_nat.c 3 Oct 2007 09:57:44 -0000
@@ -5013,10 +5013,11 @@ ipfgeniter_t *itp;
ipnat_t *ipn, *nextipnat = NULL, zeroipn;
nat_t *nat, *nextnat = NULL, zeronat;
int error = 0, count;
- ipftoken_t *freet;
char *dst;
- freet = NULL;
+ count = itp->igi_nitems;
+ if (count < 1)
+ return ENOSPC;
READ_ENTER(&ipf_nat);
@@ -5054,61 +5055,52 @@ ipfgeniter_t *itp;
}
dst = itp->igi_data;
- for (count = itp->igi_nitems; count > 0; count--) {
+ for (;;) {
switch (itp->igi_type)
{
case IPFGENITER_HOSTMAP :
if (nexthm != NULL) {
- if (nexthm->hm_next == NULL) {
- freet = t;
- count = 1;
- hm = NULL;
- }
if (count == 1) {
ATOMIC_INC32(nexthm->hm_ref);
+ t->ipt_data = nexthm;
}
} else {
bzero(&zerohm, sizeof(zerohm));
nexthm = &zerohm;
count = 1;
+ t->ipt_data = NULL;
}
break;
case IPFGENITER_IPNAT :
if (nextipnat != NULL) {
- if (nextipnat->in_next == NULL) {
- freet = t;
- count = 1;
- ipn = NULL;
- }
if (count == 1) {
MUTEX_ENTER(&nextipnat->in_lock);
nextipnat->in_use++;
MUTEX_EXIT(&nextipnat->in_lock);
+ t->ipt_data = nextipnat;
}
} else {
bzero(&zeroipn, sizeof(zeroipn));
nextipnat = &zeroipn;
count = 1;
+ t->ipt_data = NULL;
}
break;
case IPFGENITER_NAT :
if (nextnat != NULL) {
- if (nextnat->nat_next == NULL) {
- count = 1;
- freet = t;
- nat = NULL;
- }
if (count == 1) {
MUTEX_ENTER(&nextnat->nat_lock);
nextnat->nat_ref++;
MUTEX_EXIT(&nextnat->nat_lock);
+ t->ipt_data = nextnat;
}
} else {
bzero(&zeronat, sizeof(zeronat));
nextnat = &zeronat;
count = 1;
+ t->ipt_data = NULL;
}
break;
default :
@@ -5116,20 +5108,12 @@ ipfgeniter_t *itp;
}
RWLOCK_EXIT(&ipf_nat);
- if (freet != NULL) {
- ipf_freetoken(freet);
- freet = NULL;
- }
-
+ /*
+ * Copying out to user space needs to be done without the lock.
+ */
switch (itp->igi_type)
{
case IPFGENITER_HOSTMAP :
- if (hm != NULL) {
- WRITE_ENTER(&ipf_nat);
- fr_hostmapdel(&hm);
- RWLOCK_EXIT(&ipf_nat);
- }
- t->ipt_data = nexthm;
error = COPYOUT(nexthm, dst, sizeof(*nexthm));
if (error != 0)
error = EFAULT;
@@ -5138,9 +5122,6 @@ ipfgeniter_t *itp;
break;
case IPFGENITER_IPNAT :
- if (ipn != NULL)
- fr_ipnatderef(&ipn);
- t->ipt_data = nextipnat;
error = COPYOUT(nextipnat, dst, sizeof(*nextipnat));
if (error != 0)
error = EFAULT;
@@ -5149,9 +5130,6 @@ ipfgeniter_t *itp;
break;
case IPFGENITER_NAT :
- if (nat != NULL)
- fr_natderef(&nat);
- t->ipt_data = nextnat;
error = COPYOUT(nextnat, dst, sizeof(*nextnat));
if (error != 0)
error = EFAULT;
@@ -5163,29 +5141,52 @@ ipfgeniter_t *itp;
if ((count == 1) || (error != 0))
break;
+ count--;
+
READ_ENTER(&ipf_nat);
+ /*
+ * We need to have the lock again here to make sure that
+ * using _next is consistent.
+ */
switch (itp->igi_type)
{
case IPFGENITER_HOSTMAP :
- hm = nexthm;
- nexthm = hm->hm_next;
+ nexthm = nexthm->hm_next;
break;
-
case IPFGENITER_IPNAT :
- ipn = nextipnat;
- nextipnat = ipn->in_next;
+ nextipnat = nextipnat->in_next;
break;
-
case IPFGENITER_NAT :
- nat = nextnat;
- nextnat = nat->nat_next;
- break;
- default :
+ nextnat = nextnat->nat_next;
break;
}
}
+
+ switch (itp->igi_type)
+ {
+ case IPFGENITER_HOSTMAP :
+ if (hm != NULL) {
+ WRITE_ENTER(&ipf_nat);
+ fr_hostmapdel(&hm);
+ RWLOCK_EXIT(&ipf_nat);
+ }
+ break;
+ case IPFGENITER_IPNAT :
+ if (ipn != NULL) {
+ fr_ipnatderef(&ipn);
+ }
+ break;
+ case IPFGENITER_NAT :
+ if (nat != NULL) {
+ fr_natderef(&nat);
+ }
+ break;
+ default :
+ break;
+ }
+
return error;
}
--5vNYLRcllDrimb99--