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--