Correct SA refcounting. Fixes a bug where isakmpd could die when a peer was

discovered to have rebooted, and old now invalid SAs had to be garbage-
collected.

6 files changed, 44 insertions, 50 deletions

diff --git a/sbin/isakmpd/exchange.c b/sbin/isakmpd/exchange.c
index cf4d5e72eb5..26fb2734dea 100644
--- a/sbin/isakmpd/exchange.c
+++ b/sbin/isakmpd/exchange.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: exchange.c,v 1.44 2001/04/09 22:09:51 ho Exp $	*/
+/*	$OpenBSD: exchange.c,v 1.45 2001/04/24 07:27:36 niklas Exp $	*/
 /*	$EOM: exchange.c,v 1.143 2000/12/04 00:02:25 angelos Exp $	*/
 
 /*
@@ -817,8 +817,7 @@ exchange_establish_p1 (struct transport *t, u_int8_t type, u_int32_t doi,
 	  exchange_free (exchange);
 	  return;
 	}
-      else
-        sa_reference (msg->isakmp_sa);
+      sa_reference (msg->isakmp_sa);
     }
 
   msg->extra = args;
@@ -932,8 +931,8 @@ exchange_establish_p2 (struct sa *isakmp_sa, u_int8_t type, char *name,
     }
 
   msg = message_alloc (isakmp_sa->transport, 0, ISAKMP_HDR_SZ);
-  sa_reference (isakmp_sa);
   msg->isakmp_sa = isakmp_sa;
+  sa_reference (isakmp_sa);
   
   msg->extra = args;
 
@@ -1454,10 +1453,7 @@ exchange_finalize (struct message *msg)
 	}
 
       TAILQ_REMOVE (&exchange->sa_list, sa, next);
-
-      /* Only release the SA if it is time-expired */
-      if (sa->refcnt > 1)
-	sa_release (sa);
+      sa_release (sa);
     }
 
   /* If we have nothing to retransmit we can safely remove ourselves.  */
diff --git a/sbin/isakmpd/ipsec.c b/sbin/isakmpd/ipsec.c
index c111f58300b..0baa7c10e42 100644
--- a/sbin/isakmpd/ipsec.c
+++ b/sbin/isakmpd/ipsec.c
@@ -1,8 +1,8 @@
-/*	$OpenBSD: ipsec.c,v 1.43 2001/04/15 16:09:16 ho Exp $	*/
+/*	$OpenBSD: ipsec.c,v 1.44 2001/04/24 07:27:37 niklas Exp $	*/
 /*	$EOM: ipsec.c,v 1.143 2000/12/11 23:57:42 niklas Exp $	*/
 
 /*
- * Copyright (c) 1998, 1999, 2000 Niklas Hallqvist.  All rights reserved.
+ * Copyright (c) 1998, 1999, 2000, 2001 Niklas Hallqvist.  All rights reserved.
  * Copyright (c) 2001 Angelos D. Keromytis.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -805,7 +805,6 @@ ipsec_delete_spi_list (struct sockaddr *addr, u_int8_t proto,
 	       "%s made us delete SA %p (%d references) for proto %d",
 	       type, sa, sa->refcnt, proto));
 
-      sa_reference (sa);
       sa_free (sa);
     }
 }
@@ -1394,7 +1393,7 @@ ipsec_handle_leftover_payload (struct message *msg, u_int8_t type,
   u_int32_t spisz, nspis;
   struct sockaddr *dst;
   socklen_t dstlen;
-  int flag = 0;
+  int reenter = 0;
   u_int8_t *spis, proto;
   struct sa *sa;
 
@@ -1416,19 +1415,17 @@ ipsec_handle_leftover_payload (struct message *msg, u_int8_t type,
       if ((proto == ISAKMP_PROTO_ISAKMP && spisz != ISAKMP_HDR_COOKIES_LEN)
           || (proto != ISAKMP_PROTO_ISAKMP && spisz != sizeof (u_int32_t)))
         {
-	    LOG_DBG ((LOG_SA, 50,
-	             "ipsec_handle_leftover_payload: invalid SPI size %d "
-                     "for proto %d in DELETE payload", spisz, proto));
-	    return -1;
+	  log_print ("ipsec_handle_leftover_payload: "
+		     "invalid SPI size %d for proto %d in DELETE payload",
+		     spisz, proto);
+	  return -1;
         }
 
-      spis = (u_int8_t *) malloc (nspis * spisz);
-      if (spis == NULL)
+      spis = (u_int8_t *)malloc (nspis * spisz);
+      if (!spis)
         {
-	  LOG_DBG ((LOG_SA, 50,
-		    "ipsec_handle_leftover_payload: "
-		    "DELETE failed to allocate %d SPIs of %d bytes each",
-		    nspis, spisz));
+	  log_error ("ipsec_handle_leftover_payload: malloc (%d) failed",
+		     nspis * spisz);
 	  return -1;
 	}
 
@@ -1441,6 +1438,7 @@ ipsec_handle_leftover_payload (struct message *msg, u_int8_t type,
       free (spis);
       payload->flags |= PL_MARK;
       return 0;
+
     case ISAKMP_PAYLOAD_NOTIFY:
       switch (GET_ISAKMP_NOTIFY_MSG_TYPE (payload->p))
 	{
@@ -1456,13 +1454,14 @@ ipsec_handle_leftover_payload (struct message *msg, u_int8_t type,
 	      /*
 	       * Don't delete the current SA -- we received the notification
 	       * over it, so it's obviously still active. We temporarily need
-               * to remove the SA from the list to avoid an endless loop.
+               * to remove the SA from the list to avoid an endless loop,
+	       * but keep a reference so it won't disappear meanwhile.
 	       */
-
 	      if (sa == msg->isakmp_sa)
 	        {
-                  LIST_REMOVE (sa, link);
-                  flag = 1;
+		  sa_reference (sa);
+                  sa_remove (sa);
+                  reenter = 1;
 		  continue;
 		}
 
@@ -1473,8 +1472,11 @@ ipsec_handle_leftover_payload (struct message *msg, u_int8_t type,
 	      sa_delete (sa, 0);
 	    }
 
-          if (flag)
-            sa_enter (msg->isakmp_sa);
+          if (reenter)
+	    {
+	      sa_enter (msg->isakmp_sa);
+	      sa_release (msg->isakmp_sa);
+	    }
 	  payload->flags |= PL_MARK;
 	  return 0;
 	}
diff --git a/sbin/isakmpd/message.c b/sbin/isakmpd/message.c
index 458e151929b..a8429a4b743 100644
--- a/sbin/isakmpd/message.c
+++ b/sbin/isakmpd/message.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: message.c,v 1.41 2001/04/14 01:25:34 ho Exp $	*/
+/*	$OpenBSD: message.c,v 1.42 2001/04/24 07:27:37 niklas Exp $	*/
 /*	$EOM: message.c,v 1.156 2000/10/10 12:36:39 provos Exp $	*/
 
 /*
@@ -173,9 +173,9 @@ message_alloc_reply (struct message *msg)
 
   reply = message_alloc (msg->transport, 0, ISAKMP_HDR_SZ);
   reply->exchange = msg->exchange;
+  reply->isakmp_sa = msg->isakmp_sa;
   if (msg->isakmp_sa)
     sa_reference (msg->isakmp_sa);
-  reply->isakmp_sa = msg->isakmp_sa;
   return reply;
 }
 
diff --git a/sbin/isakmpd/pf_key_v2.c b/sbin/isakmpd/pf_key_v2.c
index a8cd801c06c..b61f44f0aca 100644
--- a/sbin/isakmpd/pf_key_v2.c
+++ b/sbin/isakmpd/pf_key_v2.c
@@ -1,4 +1,4 @@
-/*      $OpenBSD: pf_key_v2.c,v 1.49 2001/04/19 20:12:44 niklas Exp $  */
+/*      $OpenBSD: pf_key_v2.c,v 1.50 2001/04/24 07:27:37 niklas Exp $  */
 /*	$EOM: pf_key_v2.c,v 1.79 2000/12/12 00:33:19 niklas Exp $	*/
 
 /*
@@ -2025,13 +2025,6 @@ pf_key_v2_expire (struct pf_key_v2_msg *pmsg)
 
   if (life->sadb_lifetime_exttype == SADB_EXT_LIFETIME_HARD)
     {
-      /*
-       * This SA is still referenced by the software timeout.
-       * However, sa_free will clean up all timeouts and
-       * decrement reference counters.  We need to reference it
-       * so that sa_release() works.
-       */
-      sa_reference (sa);
       /* Remove the old SA, it isn't useful anymore.  */
       sa_free (sa);
     }
diff --git a/sbin/isakmpd/sa.c b/sbin/isakmpd/sa.c
index 910017983bd..b330a699ef1 100644
--- a/sbin/isakmpd/sa.c
+++ b/sbin/isakmpd/sa.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: sa.c,v 1.40 2001/04/09 22:09:52 ho Exp $	*/
+/*	$OpenBSD: sa.c,v 1.41 2001/04/24 07:27:37 niklas Exp $	*/
 /*	$EOM: sa.c,v 1.112 2000/12/12 00:22:52 niklas Exp $	*/
 
 /*
@@ -290,6 +290,7 @@ sa_enter (struct sa *sa)
     }
   bucket &= bucket_mask;
   LIST_INSERT_HEAD (&sa_tab[bucket], sa, link);
+  sa_reference (sa);
   LOG_DBG ((LOG_SA, 70, "sa_enter: SA %p added to SA list", sa));
   return 1;
 }
@@ -502,17 +503,18 @@ sa_free (struct sa *sa)
       sa->soft_death = 0;
       sa->refcnt--;
     }
-  sa_free_aux (sa);
+  sa_remove (sa);
 }
 
-/* Release all resources this SA is using except the death timers.  */
+/* Remove the SA from the hash table of live SAs.  */
 void
-sa_free_aux (struct sa *sa)
+sa_remove (struct sa *sa)
 {
   LIST_REMOVE (sa, link);
-  LOG_DBG ((LOG_SA, 70, "sa_free_aux: SA %p removed from SA list", sa));
+  LOG_DBG ((LOG_SA, 70, "sa_remove: SA %p removed from SA list", sa));
   sa_release (sa);
 }
+
 /* Raise the reference count of SA.  */
 void
 sa_reference (struct sa *sa)
@@ -581,7 +583,7 @@ sa_isakmp_upgrade (struct message *msg)
 {
   struct sa *sa = TAILQ_FIRST (&msg->exchange->sa_list);
 
-  LIST_REMOVE (sa, link);
+  sa_remove (sa);
   GET_ISAKMP_HDR_RCOOKIE (msg->iov[0].iov_base,
 			  sa->cookies + ISAKMP_HDR_ICOOKIE_LEN);
 
@@ -679,7 +681,7 @@ void
 sa_delete (struct sa *sa, int notify)
 {
   /* Don't bother notifying of Phase 1 SA deletes.  */
-  if (sa->phase != 1)
+  if (sa->phase != 1 && notify)
     message_send_delete (sa);
   sa_free (sa);
 }
@@ -691,7 +693,9 @@ static void
 sa_soft_expire (void *v_sa)
 {
   struct sa *sa = v_sa;
+
   sa->soft_death = 0;
+  sa_release (sa);
 
   if ((sa->flags & (SA_FLAG_STAYALIVE | SA_FLAG_REPLACED))
       == SA_FLAG_STAYALIVE)
@@ -702,8 +706,6 @@ sa_soft_expire (void *v_sa)
      * happen as soon as it is shown to be alive.
      */
     sa->flags |= SA_FLAG_FADING;
-
-  sa_release (sa);
 }
 
 /* SA has passed its best before date.  */
@@ -713,6 +715,7 @@ sa_hard_expire (void *v_sa)
   struct sa *sa = v_sa;
 
   sa->death = 0;
+  sa_release (sa);
 
   if ((sa->flags & (SA_FLAG_STAYALIVE | SA_FLAG_REPLACED))
       == SA_FLAG_STAYALIVE)
diff --git a/sbin/isakmpd/sa.h b/sbin/isakmpd/sa.h
index cfe8759de5c..0622b3a5a08 100644
--- a/sbin/isakmpd/sa.h
+++ b/sbin/isakmpd/sa.h
@@ -1,8 +1,8 @@
-/*	$OpenBSD: sa.h,v 1.20 2001/01/27 12:03:36 niklas Exp $	*/
+/*	$OpenBSD: sa.h,v 1.21 2001/04/24 07:27:37 niklas Exp $	*/
 /*	$EOM: sa.h,v 1.58 2000/10/10 12:39:01 provos Exp $	*/
 
 /*
- * Copyright (c) 1998, 1999 Niklas Hallqvist.  All rights reserved.
+ * Copyright (c) 1998, 1999, 2001 Niklas Hallqvist.  All rights reserved.
  * Copyright (c) 1999, 2001 Angelos D. Keromytis.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -190,7 +190,6 @@ extern void sa_delete (struct sa *, int);
 extern struct sa *sa_find (int (*) (struct sa *, void *), void *);
 extern int sa_flag (char *);
 extern void sa_free (struct sa *);
-extern void sa_free_aux (struct sa *);
 extern void sa_init (void);
 extern struct sa *sa_isakmp_lookup_by_peer (struct sockaddr *, socklen_t);
 extern void sa_isakmp_upgrade (struct message *);
@@ -203,6 +202,7 @@ extern struct sa *sa_lookup_isakmp_sa (struct sockaddr *, u_int8_t *);
 extern void sa_mark_replaced (struct sa *);
 extern void sa_reference (struct sa *);
 extern void sa_release (struct sa *);
+extern void sa_remove (struct sa *);
 extern void sa_report (void);
 extern int sa_setup_expirations (struct sa *);
 #endif /* _SA_H_ */