From owner-freebsd-current@FreeBSD.ORG Wed Jan 14 00:45:52 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 8DCC816A4CE; Wed, 14 Jan 2004 00:45:52 -0800 (PST) Received: from coconut.itojun.org (coconut.itojun.org [219.101.47.130]) by mx1.FreeBSD.org (Postfix) with ESMTP id 42D5143D1D; Wed, 14 Jan 2004 00:45:51 -0800 (PST) (envelope-from itojun@itojun.org) Received: by coconut.itojun.org (Postfix, from userid 1001) id 8B358A0; Wed, 14 Jan 2004 17:45:50 +0900 (JST) To: bzeeb-lists@lists.zabbadoz.net In-Reply-To: Your message of "Wed, 14 Jan 2004 17:43:56 +0900 (JST)" <20040114084356.C894EA0@coconut.itojun.org> References: <20040114084356.C894EA0@coconut.itojun.org> X-Mailer: Cue version 0.6 (031125-1130/itojun) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Message-Id: <20040114084550.8B358A0@coconut.itojun.org> Date: Wed, 14 Jan 2004 17:45:50 +0900 (JST) From: itojun@itojun.org (Jun-ichiro itojun Hagino) X-Mailman-Approved-At: Wed, 14 Jan 2004 05:28:04 -0800 cc: core@kame.net cc: current@freebsd.org cc: bzeeb+freebsd@zabbadoz.net cc: ume@freebsd.org Subject: Re: [PATCH] IPSec fixes X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Jan 2004 08:45:52 -0000 > > > as for key_sp_unlink(), i don't think the patch is correct. > > > even if you do not call key_sp_unlink() in key_spdflush(), spd entries > > > will get unlink'ed in key_timehandler(). therefore the end result > > > will be the same. > > > > No ! calling key_sp_unlink() from key_spdflush() will result in an > > _extra_ call of key_freesp() and thus refcnt will be decremented > > though it shouldn't. > > This will result in a refcnt being 0 too early and with valid > > pointers to that secpolicy and will further lead to "Memory accessed > > and/or modified after free" situations somewhen after the first and > > all successive flushes of the SPD. > > Each part of the code checks for the state == .._DEAD when getting an > > sp from sptree so the comment above key_spdflush() is correct. Only > > mark the sp as dead. > > > > Hope this explains the problem a bit better. > > the refcnt decremented in key_sp_unlink() is for the link from > sptree[]. i guess this is the proper fix. does it fix your situation? oops. itojun Index: key.c =================================================================== RCS file: /cvsroot/kame/kame/kame/sys/netkey/key.c,v retrieving revision 1.324 diff -u -r1.324 key.c --- key.c 14 Jan 2004 04:10:24 -0000 1.324 +++ key.c 14 Jan 2004 08:45:36 -0000 @@ -2092,9 +2092,9 @@ newsp->lifetime = lft ? lft->sadb_lifetime_addtime : 0; newsp->validtime = lft ? lft->sadb_lifetime_usetime : 0; - newsp->refcnt = 1; /* do not reclaim until I say I do */ newsp->state = IPSEC_SPSTATE_ALIVE; LIST_INSERT_TAIL(&sptree[newsp->dir], newsp, secpolicy, chain); + newsp->refcnt++; /* delete the entry in spacqtree */ if (mhp->msg->sadb_msg_type == SADB_X_SPDUPDATE && @@ -8080,9 +8080,10 @@ { /* remove from SP index */ - if (__LIST_CHAINED(sp)) + if (__LIST_CHAINED(sp)) { LIST_REMOVE(sp, chain); - key_freesp(sp); + key_freesp(sp); + } } /* XXX too much? */