Re: [PATCH] IPSec fixes

On Sun, 18 Jan 2004, Hajimu UMEMOTO wrote:

> bzeeb> It will work *argl* but is _not_ considered to be a correct solution
> bzeeb> so please do not apply. I still consider my original patch to be
> bzeeb> correct.
>
> I'll follow KAME's decision.  Since IPsec is from KAME, it is good to
> sync with KAME.

That's good.


Ok, _longer_ explanation (please strip when replying !):

I said the patch suggested by itojun works. So why don't I consider it
being good ?

I think key_sp_unlink() should be called
A) only once per SP
B) only for SPs actualy in sptree

A) is pretty clear with current code. sp will always be marked
dead before calling key_sp_unlink() apart from within key_timehandler()
which will only call key_sp_unlink() if SP is marked dead. This
doesn't matter there because key_timehandler()  walks sptree[]
and if it would have been unlinked before key_timehandler() would no
longer find it.  [note: this can only happen if key_timehandler()
has expired the SP before ]

B) shouldn't be much harder to do: let's see where key_sp_unlink()
is called from:
- key_spdadd() on a SP returned from either key_getspbyid or key_getsp.
  + key_getsp walks sptree so no problem here
  + key_getspbyid() is walking sptailq comparing ids. So it could
    potentially return an sp not in sptree but the id extracted in
    key_spdadd() should be from a SP from SPD so this is no problem.
    [ remark: unsure if a user could pass an id not in sptree but in
      sptailq to an SADB_X_SPDUPDATE and cause problems; should be
      investigated seperatly. ]
- key_spddelete(): sp from key_getsp thus from sptree.
- key_spddelete2(): sp from key_getspbyid() thus see remark in
	key_spdadd() but for SADB_X_SPDDELETE2 this time.
- key_spdflush(): will talk about this in a minute
- key_timehandler(): walks sptree[] so no problem

ok back to key_spdflush():

key_spdflush() walks sptailq and will call key_sp_unlink() on every
non-persistent, not dead sp. Thus it will also call key_sp_unlink()
on entries not in sptree[] leading to a decrement of the refcnt which
will result in a too early free(9) call and thus the panic.

This is the point where the patch from itojun comes into play.
when key_sp_unlink() gets called for an sp not in sptree the forward
and back pointers will be NULL and thus (with the patch)
key_sp_unlink() will not try to remove sp from the sptree list and
even more it will not call key_freesp().

So everything is fine - no not really. This isn't pretty clean,
not straight forward, somewhat cheating and confusing and a lot
harder to understand from my point of view.
Thus at this point I would argue for key_sp_unlink only be called
for SPs in sptree[] and thus the key_sp_unlink() call would have to be
removed from key_spdflush().

[ this paragraph is hypotethical with current code but I would like
  to mention this to show why the patch is somewhat not clean enough
  for me:
   If an SP would not be marked dead there would be another potential
  flaw  if key_sp_unlink() is not removed from key_spdflush():
   The first call to key_sp_unlink() will remove a SP from the sptree[]
  but the back and forward pointers for this SP are left untouched by
  LIST_REMOVE(). As key_spdflush() walks sptailq this already unlinked
  sp could be passed another time to key_sp_unlink() and the
  LIST_REMOVE() could mix up sptree[]; and of course the refcnt of
  this sp would be decremented once again leading to other already
  known problems.
   One could also work against this be explicitly setting the le_next
  and le_prev = NULL.
   But as said this is hypothetical as the checks for IPSEC_SPSTATE_DEAD
  prevent this from happen with the current code.
]




Then somewhen this afternoon I remembered this:

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

This has not been fully correct the time of writing (this had been before
coming up with the patch) and most likely has made me thinking in the
wrong direction the last days. The results would have not been the
same for every entry _not_ in sptree[].

That made me thinking why with key_spdflush() we do s.th. we don't
do with neither spddelete() or spddelete2() nor do in key_timehandler():

why do we mark all the other SPs not in sptree[] dead ? I think this
is not correct. We shouldn't walk sptailq but sptree in
key_spdflush(). So at this point forget the above and have a look
at the following patch (you may ingnore the inlining)
(can also be found at
http://sources.zabbadoz.net/freebsd/patchset/118-ipsec-flush-fix.diff
)

--- src-20040117-01/sys/netkey/key.c.orig	Sat Jan 17 16:31:49 2004
+++ src-20040117-01/sys/netkey/key.c	Sat Jan 17 17:02:21 2004
_at__at_ -496,7 +496,7 _at__at_ static const char *key_getuserfqdn(void)
 #endif
 static void key_sa_chgstate(struct secasvar *, u_int8_t);
 static void key_sp_dead(struct secpolicy *);
-static void key_sp_unlink(struct secpolicy *);
+static __inline void key_sp_unlink(struct secpolicy *);
 static struct mbuf *key_alloc_mbuf(int);
 static struct callout key_timehandler_ch;

_at__at_ -2398,6 +2398,7 _at__at_ key_spdflush(so, m, mhp)
 {
 	struct sadb_msg *newmsg;
 	struct secpolicy *sp, *nextsp;
+	u_int dir;

 	/* sanity check */
 	if (so == NULL || m == NULL || mhp == NULL || mhp->msg == NULL)
_at__at_ -2406,15 +2407,16 _at__at_ key_spdflush(so, m, mhp)
 	if (m->m_len != PFKEY_ALIGN8(sizeof(struct sadb_msg)))
 		return key_senderror(so, m, EINVAL);

-	for (sp = TAILQ_FIRST(&sptailq); sp; sp = nextsp) {
-		nextsp = TAILQ_NEXT(sp, tailq);
-		if (sp->persist)
-			continue;
-		if (sp->state == IPSEC_SPSTATE_DEAD)
-			continue;
-		key_sp_dead(sp);
-		key_sp_unlink(sp);
-		sp = NULL;
+	for (dir = 0; dir < IPSEC_DIR_MAX; dir++) {
+		for (sp = LIST_FIRST(&sptree[dir]); sp != NULL; sp = nextsp) {
+			nextsp = LIST_NEXT(sp, chain);
+			if (sp->persist)
+				continue;
+			if (sp->state == IPSEC_SPSTATE_DEAD)
+				continue;
+			key_sp_dead(sp);
+			key_sp_unlink(sp);
+		}
 	}

 	/* invalidate all cached SPD pointers on pcb */
_at__at_ -7585,14 +7587,18 _at__at_ key_sp_dead(sp)
 	sp->state = IPSEC_SPSTATE_DEAD;
 }

-static void
+static __inline void
 key_sp_unlink(sp)
 	struct secpolicy *sp;
 {
-
+	KASSERT((sp->chain.le_next != NULL || sp->chain.le_prev != NULL),
+		("key_sp_unlink called on unlinked sp"));
+
 	/* remove from SP index */
-	if (__LIST_CHAINED(sp))
-		LIST_REMOVE(sp, chain);
+	LIST_REMOVE(sp, chain);
+	/* clear forward and back pointers (not done by LIST_REMOVE) */
+	sp->chain.le_next = NULL;
+	sp->chain.le_prev = NULL;
 	key_freesp(sp);
 }



This patch does mostly so three things:

1st: it makes key_spdflush() walk sptree[] instead of sptailq. With
     this we can always call key_sp_unlink() as the SP is in the sptree.

2nd: it will leave all SPs not in sptree[] untouched. I have not found
     any reason why we would want to mark all the others dead in spdflush ?
     I have not found my kernel leaking any SPs with this patch (this
     means being malloced but not free'ed apart from the 4 persistent
     SPs (ip4 and ip6 default, PCB in and out)

3rd: it removes the __LIST_CHAINED from key_sp_unlink() and replaces
     it with a KASSERT() so that we can be sure that key_sp_unlink()
     is only called once per SP (one can only remove s.th. once from a
     list; if it's not in there you cannot remove it and some other
     logic in the code is b0rken). But this is not enough: to ensure
     that the KASSERT() gets enforced one has to NULL the forward and
     and back pointers as stated somewhere in the above part you have
     been told to forget ;-)

NB: one may replace the KASSERT with an
	if(!__LIST_CHAINED(sp)) {
		/* either */
		panic("...");
		/* or */
		printf("s.th. very bad happend ... \n");
		return;
	 }
    then this would also make the two (unsure if can happen) flaws with
    key_getspbyid() from B) above being noted if kernel had been
    built without INVARIANTS.


*uff*

Comments ?

-- 
Bjoern A. Zeeb				bzeeb at Zabbadoz dot NeT
56 69 73 69 74				http://www.zabbadoz.net/